chore(docs): update docs around release and location of helm chart#239
Merged
Conversation
lockwobr
requested review from
ayuskauskas,
mskalka and
rice-riley
as code owners
ayuskauskas
approved these changes
May 19, 2026
Contributor
📝 WalkthroughWalkthroughThis PR implements a coordinated migration from Skyhook to NodeWright branding and shifts distribution to ghcr.io-only delivery. The Helm chart name changes to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/lint-ci.yaml:
- Line 153: The workflow currently accepts HTTP status 403 in the
curlcheck/accept range (--accept "200..=299,403,429,500..=599"), which can mask
real authorization failures; either remove 403 from the accept list or add a
short comment documenting why 403 is considered transient in this job. Locate
the --accept flag in the lint-ci.yaml step (the line containing --accept
"200..=299,403,429,500..=599") and update the string to exclude 403 (e.g. remove
the literal "403") or append an inline comment explaining the intentional
trade-off for noisy external services so future reviewers understand the
rationale.
- Around line 99-110: The cache step "Restore lychee cache" uses key: lychee-${{
github.run_id }} which prevents weekly rotation described in the comment; update
the step to use a date-based or weekly key instead (modify the key field and
keep restore-keys), e.g. replace key: lychee-${{ github.run_id }} with a
date-based key like key: lychee-${{ github.event.repository.updated_at }}-${{
hashFiles('**/*.md') }} or a simple weekly rotation key such as key:
lychee-week-${{ github.run_id }} paired with restore-keys: | lychee-week- so the
cache actually rotates on a weekly basis and dead links resurface as intended
(edit the "Restore lychee cache" step, changing the key and restore-keys
entries).
- Line 104: Replace the floating tag "uses: actions/cache@v4" with a SHA-pinned
reference to a specific v4 release commit (for example "uses:
actions/cache@0400d5f" or the actual commit SHA for the latest v4.x release);
update the workflow line that currently uses the actions/cache@v4 token so it
points to the exact commit SHA to satisfy the pinning policy and ensure
reproducible builds.
In `@docs/release-process.md`:
- Around line 20-25: Update the unscoped tags in the flow-diagram lines so they
use component-prefixed tags (e.g., change "v0.16.0-rc1", "v0.16.0-rc2", and
"v0.16.0" to the appropriate component-prefixed forms such as
"operator/v0.16.0-rc1", "operator/v0.16.0-rc2" or "chart/v0.16.0" as
applicable); modify the three entries that include tag: "v0.16.0-rc1", tag:
"v0.16.0-rc2", and commit tag: "v0.16.0" in the diff so they follow the
documented component-prefixed convention to prevent ambiguous release tagging.
- Around line 71-73: The step in docs/release-process.md that currently reads
"Cut the final release on the same commit as the last good RC." but then
immediately instructs to "Bump Chart.yaml to v0.16.0 (drop the -rcN suffix) and
commit." is contradictory; update the sentence starting with "Cut the final
release..." to instead say something like "Cut the final release immediately
after the last good RC." (or equivalent) so it no longer conflicts with the
following "Bump Chart.yaml..." and ensure the surrounding lines referencing
Chart.yaml and the git commit example remain consistent with the new wording.
In `@k8s-tests/chainsaw/helm/helm-chart-test/assert-no-schedule.yaml`:
- Line 52: Update the image-matching expression so it uses the trailing-colon
local registry pattern; replace the contains(...) call currently written as
contains(image, 'localhost:5005/skyhook-operator') with contains(image,
'localhost:5005/skyhook-operator:') in the assertion (the expression on line
containing (image == 'ghcr.io/nvidia/nodewright/operator:latest' ||
contains(...)) so the test only matches exact repository paths like
localhost:5005/skyhook-operator:tag and avoids false positives like
localhost:5005/skyhook-operator-foo:tag.
In `@README.md`:
- Around line 173-180: Update the Helm examples to use the OCI chart URI and the
recommended upgrade/install command: replace uses of "./chart" in the commands
(the helm install nodewright ./chart ... and the helm install nodewright ./chart
--namespace skyhook --set cleanup.jobTimeoutSeconds=180 example) with
"oci://ghcr.io/nvidia/nodewright/charts/nodewright" and prefer "helm upgrade
--install nodewright oci://ghcr.io/nvidia/nodewright/charts/nodewright
--namespace skyhook" (including the same --set flags like cleanup.enabled=false
and cleanup.jobTimeoutSeconds=180 as shown).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b72a4759-5a8e-4350-a623-5739329e3685
📒 Files selected for processing (15)
.claude/CLAUDE.md.github/workflows/lint-ci.yamlREADME.mdchart/Chart.yamlchart/README.mdchart/templates/_helpers.tplchart/values.yamldocs/release-process.mdk8s-tests/chainsaw/helm/helm-chart-test/assert-no-schedule.yamlk8s-tests/chainsaw/helm/helm-chart-test/assert-scheduled.yamlk8s-tests/chainsaw/helm/helm-node-affinity-test/assert-no-schedule.yamlk8s-tests/chainsaw/helm/helm-node-affinity-test/assert-scheduled.yamlk8s-tests/chainsaw/helm/helm-scale-test/assert-override-resources.yamlk8s-tests/chainsaw/helm/helm-scale-test/assert-scaled-resources.yamlk8s-tests/chainsaw/helm/helm-webhook-test/assert-scheduled.yaml
Comment on lines
+99
to
+110
| # Persist lychee's URL cache across runs so transient external | ||
| # failures don't re-fail every PR. Cache is keyed weekly so dead | ||
| # links still surface within ~7 days. | ||
| - name: Restore lychee cache | ||
| if: always() | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: .lycheecache | ||
| key: lychee-${{ github.run_id }} | ||
| restore-keys: | | ||
| lychee- | ||
|
|
Contributor
There was a problem hiding this comment.
Cache key strategy conflicts with documented intent.
The comment states the cache is "keyed weekly so dead links still surface within ~7 days," but the key uses github.run_id, which is unique for every workflow run. This means:
- The exact key
lychee-${{ github.run_id }}will never match on future runs - Cache hits rely entirely on the
restore-keys: lychee-fallback, which matches the most recent cache regardless of age - Dead links will not resurface within 7 days as intended — they'll remain cached indefinitely until the most recent cache expires (GitHub's 7-day unused cache eviction)
To implement weekly rotation as documented, use a date-based key:
key: lychee-${{ github.event.repository.updated_at }}-${{ hashFiles('**/*.md') }}Or simpler weekly rotation:
key: lychee-week-${{ github.run_id }}
restore-keys: |
lychee-week-then rely on GitHub's automatic cache eviction after 7 days of no access.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 104-104: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/lint-ci.yaml around lines 99 - 110, The cache step
"Restore lychee cache" uses key: lychee-${{ github.run_id }} which prevents
weekly rotation described in the comment; update the step to use a date-based or
weekly key instead (modify the key field and keep restore-keys), e.g. replace
key: lychee-${{ github.run_id }} with a date-based key like key: lychee-${{
github.event.repository.updated_at }}-${{ hashFiles('**/*.md') }} or a simple
weekly rotation key such as key: lychee-week-${{ github.run_id }} paired with
restore-keys: | lychee-week- so the cache actually rotates on a weekly basis and
dead links resurface as intended (edit the "Restore lychee cache" step, changing
the key and restore-keys entries).
| # links still surface within ~7 days. | ||
| - name: Restore lychee cache | ||
| if: always() | ||
| uses: actions/cache@v4 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the latest commit SHA for actions/cache v4 tag
gh api repos/actions/cache/git/ref/tags/v4 --jq '.object.sha'Repository: NVIDIA/nodewright
Length of output: 2043
🏁 Script executed:
# First, let's examine the lint-ci.yaml file to see the context around lines 72 and 104
cd .github/workflows && wc -l lint-ci.yamlRepository: NVIDIA/nodewright
Length of output: 78
🏁 Script executed:
# Read the lint-ci.yaml file, focusing on lines around 72 and 104
cat -n lint-ci.yaml | sed -n '65,115p'Repository: NVIDIA/nodewright
Length of output: 106
🌐 Web query:
actions/cache v4 commit SHA github
💡 Result:
For GitHub Actions’ actions/cache v4, the “commit SHA” you can pin depends on which exact v4.x release tag you mean. 1) actions/cache v4 (major tag) - The repository keeps v4 as a moving tag (points to a specific commit at the time you fetch it). The source for the v4 tag is the repo’s tag page: https://github.com/actions/cache/tree/v4 [1]. - If you want to pin to a commit SHA, you should pin to a specific release tag (e.g., v4.2.4) rather than the moving v4 tag. 2) Example: actions/cache v4.2.4 (specific release tag) - The v4.2.4 release page shows the tag resolves to commit SHA “0400d5f”. [2] How to pin safely - Use the full commit SHA shown for the exact v4.x release you want, in the workflow like: uses: actions/cache@0400d5f - GitHub’s own guidance for pinned SHAs indicates you should use the SHAs of specific versions (e.g., v4.2.0 or v3.4.0). [1][3] If you tell me which exact v4.x you want (or whether you really mean the moving “@v4” tag), I can provide the exact commit SHA for that version.
Citations:
- 1: https://github.com/actions/cache/tree/v4
- 2: https://redirect.github.com/actions/cache/releases/tag/v4.2.4
- 3: https://github.com/actions/cache/blob/main/RELEASES.md
🏁 Script executed:
# Find the lint-ci.yaml file
fd -name "lint-ci.yaml"Repository: NVIDIA/nodewright
Length of output: 292
🏁 Script executed:
# Read the file content focusing on lines around 72 and 104
fd -name "lint-ci.yaml" -exec cat -n {} \; | sed -n '65,115p'Repository: NVIDIA/nodewright
Length of output: 292
🏁 Script executed:
# Find the lint-ci.yaml file
fd -name "lint-ci.yaml"Repository: NVIDIA/nodewright
Length of output: 292
🏁 Script executed:
# Read the file with cat -n and sed for lines 65-115
cat -n .github/workflows/lint-ci.yaml | sed -n '65,115p'Repository: NVIDIA/nodewright
Length of output: 2639
Pin the action to a commit SHA.
The actions/cache@v4 reference is not pinned to a SHA, which violates NVIDIA's enterprise policy as evidenced by the SHA-pinned action at line 72. GitHub Actions best practice recommends pinning to a specific release version or commit SHA rather than using the moving v4 tag.
Pin to a specific version's commit SHA, such as v4.2.4 (commit SHA: 0400d5f) or the latest v4.x release available at https://github.com/actions/cache/releases.
Example fix for v4.2.4
- - uses: actions/cache@v4
+ - uses: actions/cache@0400d5f # v4.2.4Verify the current latest v4.x release and use its corresponding commit SHA.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| uses: actions/cache@v4 | |
| uses: actions/cache@0400d5f # v4.2.4 |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 104-104: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/lint-ci.yaml at line 104, Replace the floating tag "uses:
actions/cache@v4" with a SHA-pinned reference to a specific v4 release commit
(for example "uses: actions/cache@0400d5f" or the actual commit SHA for the
latest v4.x release); update the workflow line that currently uses the
actions/cache@v4 token so it points to the exact commit SHA to satisfy the
pinning policy and ensure reproducible builds.
| --max-retries 8 \ | ||
| --retry-wait-time 5 \ | ||
| --timeout 20 \ | ||
| --accept "200..=299,403,429,500..=599" \ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider whether accepting 403 status masks real permission issues.
The accepted status range includes 403 (Forbidden), which the comment describes as "commonly transient." However, 403 typically indicates a permanent authorization failure rather than a transient error:
- Unlike 429 (rate limit) or 5xx (server error), a 403 usually means the resource requires authentication, has moved to a private repository, or is otherwise access-controlled
- Including it may hide genuinely broken external links that have become restricted
Since this pass is already non-blocking (continue-on-error: true) and failures appear as ❌ in the log, the current approach may be an intentional trade-off to reduce noise from aggressive bot-detection or overly strict external services.
If the 403 inclusion is causing real broken links to go unnoticed, consider removing it from the accepted range. Otherwise, document the rationale in the comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/lint-ci.yaml at line 153, The workflow currently accepts
HTTP status 403 in the curlcheck/accept range (--accept
"200..=299,403,429,500..=599"), which can mask real authorization failures;
either remove 403 from the accept list or add a short comment documenting why
403 is considered transient in this job. Locate the --accept flag in the
lint-ci.yaml step (the line containing --accept "200..=299,403,429,500..=599")
and update the string to exclude 403 (e.g. remove the literal "403") or append
an inline comment explaining the intentional trade-off for noisy external
services so future reviewers understand the rationale.
Comment on lines
+20
to
+25
| cherry-pick id: "X" tag: "v0.16.0-rc1" | ||
| checkout main | ||
| commit id: "Y" | ||
| checkout release/v0.16.x | ||
| cherry-pick id: "Y" tag: "v0.16.0-rc2" | ||
| commit tag: "v0.16.0" |
Contributor
There was a problem hiding this comment.
Align flow-diagram tags with the documented component-prefixed format.
Line 20, Line 24, and Line 25 use unscoped tags (v...), but this doc later defines component-prefixed tags as the accepted format. Update the diagram labels to operator/v... and/or chart/v... to avoid release-tagging mistakes.
Based on learnings: Components are released independently with version-prefixed tags (operator/v..., agent/v..., chart/v...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/release-process.md` around lines 20 - 25, Update the unscoped tags in
the flow-diagram lines so they use component-prefixed tags (e.g., change
"v0.16.0-rc1", "v0.16.0-rc2", and "v0.16.0" to the appropriate
component-prefixed forms such as "operator/v0.16.0-rc1", "operator/v0.16.0-rc2"
or "chart/v0.16.0" as applicable); modify the three entries that include tag:
"v0.16.0-rc1", tag: "v0.16.0-rc2", and commit tag: "v0.16.0" in the diff so they
follow the documented component-prefixed convention to prevent ambiguous release
tagging.
Comment on lines
+71
to
+73
| # 6. Cut the final release on the same commit as the last good RC. | ||
| # Bump Chart.yaml to v0.16.0 (drop the -rcN suffix) and commit. | ||
| git commit -am "release: v0.16.0" |
Contributor
There was a problem hiding this comment.
Fix the contradictory “same commit as last good RC” instruction.
Line 71 says final release is cut on the same commit as the last good RC, but Line 72-Line 73 immediately require a new Chart.yaml bump commit. Reword this step to “immediately after the last good RC” (or equivalent) to remove ambiguity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/release-process.md` around lines 71 - 73, The step in
docs/release-process.md that currently reads "Cut the final release on the same
commit as the last good RC." but then immediately instructs to "Bump Chart.yaml
to v0.16.0 (drop the -rcN suffix) and commit." is contradictory; update the
sentence starting with "Cut the final release..." to instead say something like
"Cut the final release immediately after the last good RC." (or equivalent) so
it no longer conflicts with the following "Bump Chart.yaml..." and ensure the
surrounding lines referencing Chart.yaml and the git commit example remain
consistent with the new wording.
| - /manager | ||
| ((env[?name == 'RUNTIME_REQUIRED_TAINT'].value)[0] == 'skyhook.nvidia.com=runtime-required:NoSchedule'): true | ||
| (image == 'ghcr.io/nvidia/skyhook/operator:latest' || contains(image, 'localhost:5005/skyhook-operator')): true | ||
| (image == 'ghcr.io/nvidia/nodewright/operator:latest' || contains(image, 'localhost:5005/skyhook-operator')): true |
Contributor
There was a problem hiding this comment.
Inconsistent local registry image pattern across test files.
Line 52 uses contains(image, 'localhost:5005/skyhook-operator') without a trailing colon, while assert-scheduled.yaml at line 52 uses contains(image, 'localhost:5005/skyhook-operator:') with a trailing colon. These patterns should match for consistency.
The pattern without the colon will match images like localhost:5005/skyhook-operator-foo:tag, while the pattern with the colon will only match localhost:5005/skyhook-operator:*. Use the trailing-colon pattern in both files to avoid false matches.
🔧 Proposed fix
- (image == 'ghcr.io/nvidia/nodewright/operator:latest' || contains(image, 'localhost:5005/skyhook-operator')): true
+ (image == 'ghcr.io/nvidia/nodewright/operator:latest' || contains(image, 'localhost:5005/skyhook-operator:')): true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (image == 'ghcr.io/nvidia/nodewright/operator:latest' || contains(image, 'localhost:5005/skyhook-operator')): true | |
| (image == 'ghcr.io/nvidia/nodewright/operator:latest' || contains(image, 'localhost:5005/skyhook-operator:')): true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@k8s-tests/chainsaw/helm/helm-chart-test/assert-no-schedule.yaml` at line 52,
Update the image-matching expression so it uses the trailing-colon local
registry pattern; replace the contains(...) call currently written as
contains(image, 'localhost:5005/skyhook-operator') with contains(image,
'localhost:5005/skyhook-operator:') in the assertion (the expression on line
containing (image == 'ghcr.io/nvidia/nodewright/operator:latest' ||
contains(...)) so the test only matches exact repository paths like
localhost:5005/skyhook-operator:tag and avoids false positives like
localhost:5005/skyhook-operator-foo:tag.
Comment on lines
+173
to
180
| helm install nodewright ./chart --namespace skyhook --set cleanup.enabled=false | ||
| ``` | ||
|
|
||
| To adjust the job timeout: | ||
|
|
||
| ```bash | ||
| helm install skyhook ./chart --namespace skyhook \ | ||
| helm install nodewright ./chart --namespace skyhook \ | ||
| --set cleanup.jobTimeoutSeconds=180 |
Contributor
There was a problem hiding this comment.
Use OCI chart source in the cleanup configuration commands.
Line 173 and Line 179 still use ./chart, which conflicts with the OCI-first install flow above and fails for users who didn’t clone the repo. Use oci://ghcr.io/nvidia/nodewright/charts/nodewright (ideally with helm upgrade --install) in these examples for consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 173 - 180, Update the Helm examples to use the OCI
chart URI and the recommended upgrade/install command: replace uses of "./chart"
in the commands (the helm install nodewright ./chart ... and the helm install
nodewright ./chart --namespace skyhook --set cleanup.jobTimeoutSeconds=180
example) with "oci://ghcr.io/nvidia/nodewright/charts/nodewright" and prefer
"helm upgrade --install nodewright
oci://ghcr.io/nvidia/nodewright/charts/nodewright --namespace skyhook"
(including the same --set flags like cleanup.enabled=false and
cleanup.jobTimeoutSeconds=180 as shown).
Coverage Report for CI Build 26115284720Warning No base build found for commit Coverage: 81.905%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
lockwobr
added a commit
that referenced
this pull request
May 22, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
pushed a commit
that referenced
this pull request
Jun 9, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
pushed a commit
that referenced
this pull request
Jun 9, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
pushed a commit
that referenced
this pull request
Jun 9, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
pushed a commit
that referenced
this pull request
Jun 9, 2026
* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
rice-riley
added a commit
that referenced
this pull request
Jun 9, 2026
* feat(agent): port enums and steps for agent go rewrite Signed-off-by: Riley Rice <[email protected]> * chore: Cherry pick (#244) * ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]> --------- Signed-off-by: Riley Rice <[email protected]> Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Brian Lockwood <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
cherry pick c004c13 from main
Checklist
git commit -s) per the DCO.