Pull paired moveit_pro PR images from the private ECR repository - #921
Pull paired moveit_pro PR images from the private ECR repository#921JWhitleyWork wants to merge 1 commit into
Conversation
moveit_pro #22374 stops publishing per-PR images to Docker Hub and retags each validated candidate into the private `moveit-pro-ci` ECR repository as `pr-<number>-<distro>-<arch><gpu>`. The `needs: moveit_pro/#N` path here composed a Docker Hub reference from the paired PR's branch name, which that change leaves pointing at an image nobody publishes. Resolve the paired image from ECR by PR number instead, normalizing the number so a zero-padded token cannot compose a tag the producer never writes, and mirror the two pairing checks moveit_pro applies from its side: the paired PR must be open and must target the same base branch. A cross-release-line pairing used to resolve against Docker Hub and now has no image at all, so it fails with both refs named rather than deep inside a container pull. Fail a `repository_dispatch` that carries no `image_ref` rather than falling back to a Docker Hub reference built from `image_tag`, which is now `pr-<number>`. Every failing path publishes `moveit_pro_sha` first: the rollup job posts the required `example_ws / integration` status from it alone, and on a dispatch that is its only target, so returning without it would leave the moveit_pro PR pending forever. Pin the composed reference to an approved repository before use, the way moveit_pro's lane-e2e.yaml does, since it becomes a `container:` image on a self-hosted GPU runner and arrives verbatim in the payload on the dispatch path. The pre-rename `moveit-studio` repository stays approved because v9.4 still dispatches under that name. The base-branch default stays on Docker Hub: it has to resolve for every base this file serves, including release branches, and moveit_pro's v9.4, v10.0 and v10.1 lines write no ECR branch tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JqMtkYqqzdXUEYRVbe3Xp
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: 5 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. 📝 SummarySummary by CodeRabbit
WalkthroughChangesThe CI workflow now validates dispatch and paired pull-request image inputs, supports private ECR references, centralizes pairing outputs, and restricts fallback images to approved repositories with CI image resolution
Suggested reviewers: Merge Risk: ⚪ Minimal · up to CI image resolution now uses validated paired-PR ECR references while retaining supported Docker Hub defaults. No merge-blocking risk is currently identified. Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (3 passed)
Full details: Human Review CheckExplanation The PR changes CI/CD configuration in
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
|
Consider whether the change should land upstream in Overlapping files
|
|
1 participant
[written by AI]
needs: moveit_pro/#22374
Paired PR for PickNikRobotics/moveit_pro#22374, the sixth and last PR of that CI overhaul series. Blocked on the same rollout prerequisite it is: the
moveit-pro-ciECR repository does not exist yet.Motivation
moveit_pro#22374 stops publishing a public Docker Hub tag for every pull request. Each validated candidate is retagged into the private ECR repository
moveit-pro-ciaspr-<number>-<distro>-<arch><gpu>, keyed on the PR number rather than the sanitized branch name.The
needs: moveit_pro/#Npath in this repository composes its image reference from the paired PR's branch name and Docker Hub. After that change it points at an image nobody publishes, so every paired PR would fail on an image pull with no hint as to why.The
repository_dispatchpath needs less than it looks like it does. It already takesimage_reffrom the payload verbatim, and moveit_pro sends the full ECR reference there, so it keeps working untouched. What it lacked was a floor: on an emptyimage_refit fell through to a Docker Hub reference built fromimage_tag, andimage_tagis nowpr-<number>.Brief description
needs:path resolves<registry>/moveit-pro-ci:pr-<N>-{0}-amd64, byte-identical up to the CUDA suffix to the templatebuildDispatchPayloadsends in moveit_pro's.github/scripts/dispatch_example_ws.cjs.sanitizeBranchgoes with it: nothing composes a branch-named tag any more. The PR number is normalized throughparseIntfirst, so a body reading#022374cannot composepr-022374against a producer that writespr-22374.mainnaming a moveit_pro PR that targetsv10.1) used to resolve, because Docker Hub carried a branch-named tag for every line. The release lines retag nothing into ECR, so it now has no image at all and is better caught by name than by a pull failure insidecontainer:.repository_dispatchcarrying noimage_reffails with that as the reason instead of 404ing later on a Docker Hub tag that was never published.moveit_pro_shabefore it returns.integration-statusbuilds its post-back targets from that output alone, and on a dispatch it is the only target there is, so a barereturnwould leave the moveit_pro PR sitting on thependingstatus its own dispatch job wrote, with the evidence in a repository its author has no reason to open.lane-e2e.yamlpins its own. It becomes thecontainer:image of a job on a self-hosted GPU runner, and on the dispatch path it arrives verbatim in the client payload.vars.CONTAINER_REGISTRY_URLwith a hardcoded default, mirroring howDOCKERHUB_USERNAMEis already handled a few lines above. This repository has no Actions variables defined at all, so without the default nothing resolves.container:in moveit_pro_ci'sworkspace_integration_test.yaml, which has nocredentials:block, so a login step in this workflow could not help even if one were added. The pull works because thepicknik-16-amd64-gpupod (ARC scale setarc-16-amd64-gpu) runs as thearc-runner-ecrservice account with amazon-ecr-credential-helper merged into~/.docker/config.json, and that IRSA role grants pull onrepository/*rather than an enumerated list, so the new repository is covered the day it is created.Why the base-branch default stays on Docker Hub
moveit_pro#22374 leaves this open: the default could stay on Docker Hub or move to the fresher ECR
branch-<base>image. It stays.The default has to resolve for every base this file serves, and that includes release branches, where it runs with
image_tag=v9.4.moveit-pro-ci:branch-<sanitized>exists only for branches whose pipeline has run under the newpublish-runtime.yaml; moveit_pro's v9.4, v10.0 and v10.1 lines write no ECR branch tag at all. Docker Hub is the only registry carrying a tag for every base, and full runs keep those current.Worth being explicit, because my first draft of this comment got it wrong: this is not about fork pull requests lacking credentials. The ECR pull rides the runner pod's identity, not the trigger's, so a fork run landing on
picknik-16-amd64-gpuwould pull from ECR fine. Thelfs_cache_s3_bucketgate further down is the one that genuinely turns on OIDC.Rollout
moveit-pro-cistill has to exist. Inmoveit_pro_ci_runner_config,terraform/ecr_ci_images.tf'sci_imagesmap holds onlymoveit-pro-source,moveit-studio-frontendandmoveit-pro-buildcaches. Addingmoveit-pro-cithere also extends the push role, whose policy is[for r in aws_ecr_repository.ci : r.arn], so moveit_pro'spr-imagegets push access from the same edit.MUTABLE, sincepr-<n>-jazzy-amd64-cuda13.2-cudnn9is rewritten on every push to the paired moveit_pro PR. And its retention window needs to exceed a realistic paired-PR lifetime: #22374 specifies 14 days forpr-*, while a paired example_ws PR can sit open for weeks re-running itsneeds:pull against a tag written once. Either the window grows or an aged-out image needs a better failure than a pull error.terraform/irsa_runner.tfgrants pull onrepository/*.CONTAINER_REGISTRY_URLas a repository or org variable visible here would let the hardcoded default go. It is the second copy of that account id in this file, so for now there is a comment tying it to thelfs_cache_s3_bucketliteral.pr-imageECR retag. There is no Docker Hub fallback left on theneeds:path. The comment beside the template says so.What happens when a guard fires
Nothing runs and the PR cannot merge, which is the intent. Worth spelling out because the mechanism is not the one the file's own comment implies.
build-statuscalls itself "the single aggregator job that branch protection requires" and it is not:mainhas no classic branch protection, and the Default ruleset requires four contexts, none of thembuild-status. Two of them are the nested reusable-workflow job namesintegration-test (lab_sim) / integration-test-in-studio-container (jazzy)and thehangar_simequivalent. Those check runs exist only whenintegration-testactually invokes the called workflow, so aresolvefailure skips the caller, the contexts are never created, they stay Expected, and the ruleset blocks. Verified against run 34015630299, where the skippedintegration-testappears as one bare job with no matrix expansion and no nested name, against run 31206499436 where it runs and both nested contexts appear.One honest gap: that run is an
if:-skip rather than a needs-failure skip. The mechanism is the same, since neither invokes the called workflow, but no run in this repository exercises the needs-failure path yet, becauseresolvehad no failure path before this PR. The first paired PR that trips a guard will close it for real.The distinction is fragile and worth knowing about: a check run whose conclusion is
skippeddoes satisfy a required context of that name. It does not here only because the required contexts are nested names that never come into existence. If anyone ever changes the required context to the caller job nameintegration-test, or turns that job into a plainsteps:job, a skip starts satisfying the requirement and these guards would go green while nothing ran.The Default ruleset also targets
refs/heads/v9.*andrefs/heads/v10.*, so the premature-backport hazard above is fail-closed on the release lines by the same mechanism.Follow-ups, deliberately not in this PR
build-statusgates nothing, and its comment tells you otherwise. It is required by no ruleset, yet it instructs "Add new gating jobs to theneeds:list", so anyone following that instruction believes they have made a job gating when they have not.validate-workspace-dependenciesandintegration-statusare already in that list and required by nothing. Untangling that is unrelated to this change, so it is not in here, but it is the reason I did not addresolveto that list either: the real gate already fails closed.The Default ruleset hardcodes the two matrix cells by name, while the
config_packagematrix comment says the list expands one PR at a time. Each added sim needs a matching ruleset entry or the new cell runs ungated.image_tagnow names no real image on the dispatch path, yet it still forms the reusable workflow's ccache key and every restore-key prefix, giving each moveit_pro PR its own namespace that can never fall back to the base branch's entries. Passing the base branch as theimage_taginput while keeping the payload value for the run log would align both paired paths on one namespace. That is a caching behavior change, so it belongs on its own.The
resolvescript is ~150 lines of JavaScript embedded in YAML with no test in the repository. moveit_pro extracted its equivalent to.github/scripts/dispatch_example_ws.cjswith anode --testsuite in this same series. Doing the same here is the obvious next step.How it was tested
resolvescript and ran it against 28 scenarios: the ECR reference and its suffix, PR-number normalization, the open and base-ref guards, a throwing paired-PR lookup, the empty-image_reffail-fast, that every failing path still publishesmoveit_pro_shaor the PR number, the currently-deployed Docker Hub payload shapes from main/v10.0/v10.1 and from v9.4, a digest-pinned reference being accepted, four prefix-collision vectors being refused,image_tagandgit_refon each path, and two scenarios with both env defaults deleted, since a repository with no Actions variables is the only path production takes. All 28 pass. The harness is not committed; see the follow-up above.picknikciuser/moveit-studio:...where the other two sendpicknikciuser/moveit-pro:.... A dispatch always runs this file from the default branch, so the allowlist carries the pre-rename name too; without it every v9.4 pairing would break.pre-commit run --files .github/workflows/ci.yamlclean.actionlint: one pre-existing shellcheck style note, in a job this PR does not touch. Nothing new.Release notes
None
Claude agent checks
platform-architect-botpulls.getthrow is fixed and thebuild-statusone was withdrawn by the reviewer after I showed the live ruleset. 8 suggestions applied, 3 recorded as follow-ups, 1 declined with the reason in the code. No blocking findings on the final candidate.security-auditorcode-reviewerroboticist-botfrontend-noah-botcompatibility-botsonar-botdocumentation-bottest-runner🤖 Generated with Claude Code
https://claude.ai/code/session_019JqMtkYqqzdXUEYRVbe3Xp