Repository navigation
Fixes CI base image pulls when NGC refuses the inherited credential - #7332
Conversation
|
Default branch. PRs opened with the default base land here, which is how the failure arrived. Credential kept. Removing it would break fork PRs on Verified. Two GPU runners failed identically here, ruling out host state. YAML parsed; changed shell passed Live test. This PR is from a fork, so Untouched. |
Greptile SummaryThis PR makes the composite Docker action retry public NGC base-image access anonymously when inherited credentials are refused, improves registry-login secret handling, and fails early when the base-image digest cannot be resolved.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking cache-key robustness issue if buildx emits diagnostics during a successful manifest inspection. The credential fallback is constrained to a base image proven readable anonymously, and later ECR authentication is restored explicitly; only the unvalidated combined stdout/stderr digest capture needs attention. Files Needing Attention: .github/actions/ecr-build-push-pull/action.yml Important Files Changed
Reviews (1): Last reviewed commit: "Fall back to anonymous pulls when NGC re..." | Re-trigger Greptile |
4e6b6db to
ded7a06
Compare
There was a problem hiding this comment.
Isaac Lab Review Bot
The action correctly probes inherited nvcr.io credentials and falls back to anonymous access when needed, while making an unreadable base-image digest fatal rather than weakening the dependency-cache identity. One digest-capture issue should be fixed before merge: stderr is currently incorporated into cache-key data.
- Design and architecture: Credential selection remains contained in the setup step, and exporting the final DOCKER_CONFIG after probing preserves its use by later ECR and build steps. Requiring a resolved manifest digest is consistent with maintaining a reliable dependency-cache identity.
- API: No action inputs or outputs were renamed or removed. Operational behavior intentionally changes by allowing anonymous base-image access after credential refusal and by failing when the base-image digest cannot be resolved.
- Implementation: The credential probe, anonymous fallback, and temporary-directory handling are coherent. However, the digest command redirects stderr into INSPECT_OUT; on a successful command that emits a warning or diagnostic, this text is concatenated with stdout and then used in BASE_IMAGE_UNIQ_ID and DEPS_HASH. Capture failure diagnostics separately while deriving the digest from stdout only.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
|
Live proof on this PR. The third line is the bug and the fix in one place. The runner's inherited credentials genuinely cannot read the public base image, which is the Two details worth noting. The run hit a deps-cache miss, so it resolved the base image and completed a full build rather than short-circuiting on a cached layer — the strongest form of the test available here. And the |
|
Still failing on this branch. #7342, a backport opened against That log also shows the second defect this PR closes: the Ready for a decision. 33 checks pass on Review. @kellyguo11 you own |
b25387f
into
isaac-sim:release/3.0.0-beta2
Currently
Build Base Docker ImageandBuild cuRobo Docker Imagefail withdenied: Access Deniedwhile resolving the Isaac Sim base image from public ("nvidia") NGC org.Cause.
ecr-build-push-pullcopies the runner's persistentnvcr.iocredentials for a private NGC org into the job. NGC declines that identity a pull token for the publicnvidia/*catalog, so an image that needs no credentials becomes unpullable.Fix. The setup step now confirms those credentials can read the configured base image, and retries anonymously when they cannot. The deps-hash step fails loudly on an unreadable digest, which until now corrupted the cache key in silence.
docker-buildreceives the same fallback, and its login moves to--password-stdin.Why this branch. Only
release/3.0.0-beta2pins a public base image, so the bug lives here alone.developkeeps this logic in.github/actions/_lib/, a path this branch does not have, so a fix there could never run here.denied: Access DeniedType of change
Release backport
develop(not applicable: this targets a release branch directly, and the equivalent code ondevelopis already correct)Screenshots
Not applicable; the change affects CI credential handling only.
Checklist
pre-commitchecks with./isaaclab.sh --format(not run locally, no dev setup on this machine; this PR's ownpre-commitcheck passes)source/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that) (no package undersource/touched)CONTRIBUTORS.mdor my name already exists there