feat(ci): build a dev image automatically on a labelled PR - #677
feat(ci): build a dev image automatically on a labelled PR#677balajinvda wants to merge 2 commits into
Conversation
Getting a dev image to test on staging currently means opening Actions, selecting the workflow, typing the service subtree by hand and clicking run, once per attempt. That friction is why people ask for dev tags instead, but a dev tag does not solve it: publish_dev_prerelease runs on a main push, so the tag only exists after the merge, which is too late to test before merging. Label a PR with deploy-to-stg and every push to it builds and pushes a dev image. Removing the label stops it. The image lands in ncp-dev as gh.<run>-<sha8>, the same convention workflow_dispatch already uses, so nothing downstream changes. A PR event carries no service_path input, so a resolve job derives it from the changed files: take the src/<plane>/<service> prefix of everything that changed and require exactly one. Changing two services skips with a notice rather than guessing, since a dev image targets one service and picking for the user would be wrong. workflow_dispatch still passes the path explicitly and is unchanged. No release machinery is touched and no git tag is created, so this cannot reach the production promotion path. Fork PRs receive no secrets and fail closed rather than leaking the registry token; same-repo PRs get them, matching how the bazel matrix already handles the cache token. Resolution logic verified against six cases before pushing: one service, two services, docs only, a root file only, a service plus a root file, and a top-level src file. Only the single-service case builds. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
📝 WalkthroughWalkthroughThe workflow supports labeled pull request image builds. A ChangesImage push workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PullRequestEvent
participant resolve
participant push
PullRequestEvent->>resolve: Start on labeled, synchronize, or reopened event
resolve->>resolve: Check deploy-to-stg label and resolve service subtree
resolve->>push: Emit resolved service path
push->>push: Use path for cache, Bazel layout, and image publishing
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/image-push-manual.yml:
- Around line 80-85: Update the subtree detection command in the workflow to use
the merge-base diff form between BASE_SHA and HEAD_SHA, so only changes
introduced by the PR populate subtrees; leave the subsequent single-subtree
BUILD.bazel resolution logic unchanged.
- Around line 18-25: Update the workflow trigger/resolve logic so a labeled
event proceeds only when github.event.label.name is deploy-to-stg. Preserve the
existing label-presence check for synchronize and reopened events in the resolve
job condition, without changing their behavior.
- Around line 42-43: Update the concurrency group expression in the workflow to
use github.run_id for pull request runs while retaining
github.event.inputs.service_path for manual dispatches; preserve the existing
github.ref fallback where applicable and leave cancel-in-progress unchanged.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: 820e2b07-a354-4ab7-ab9a-a27819a4ac60
📒 Files selected for processing (1)
.github/workflows/image-push-manual.yml
| group: image-push-${{ github.event.inputs.service_path || github.ref }} | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/image-push-manual.yml
printf '%s\n' '--- concurrency references ---'
rg -n -C 3 'concurrency|github\.ref|github\.run_id|workflow_dispatch|pull_request|labeled' .github/workflowsRepository: NVIDIA/nvcf
Length of output: 50367
🌐 Web query:
site:docs.github.com/en/actions control workflow concurrency one pending run group cancel-in-progress false github.run_id concurrency expression
💡 Result:
In GitHub Actions, the concurrency key is used to limit the number of workflow runs or jobs that execute simultaneously within a defined concurrency group [1][2]. When a new workflow run is triggered, if another run within the same concurrency group is already in progress, the new run is placed in a pending state [1][3]. Regarding your specific scenario: 1. cancel-in-progress: false: This is the default behavior [1][3]. When set to false (or omitted), any existing pending runs within the same concurrency group are canceled, and the new run becomes the single pending run [1][3]. The currently running job or workflow is not canceled; it completes its execution [1][3][4]. 2. github.run_id as a concurrency group: If you use the expression group: ${{ github.run_id }}, you are effectively assigning a unique concurrency group to every individual workflow run [5]. Since every run has a unique ID, the concurrency group will never overlap with another run. Consequently, this configuration will have no effect on concurrency control, as no two runs will ever share the same group, and no run will ever be placed in a pending state or cause another to be canceled [1][2]. If your goal is to manage concurrency while preventing the cancellation of in-progress runs, you should omit cancel-in-progress or explicitly set it to false and ensure your concurrency group identifier is shared across the runs you intend to throttle (e.g., based on branch name or workflow name rather than a unique run ID) [1][4].
Citations:
- 1: https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency
- 2: https://docs.github.com/en/actions/concepts/workflows-and-actions/concurrency
- 3: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 4: https://docs.github.com/en/actions/how-tos/deploy/configure-and-manage-deployments/control-deployments
- 5: https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
expr = "github.event_name == 'workflow_dispatch' && github.event.inputs.service_path || github.run_id"
cases = [
("workflow_dispatch", "src/a/b", "refs/heads/main", 101),
("workflow_dispatch", "src/a/b", "refs/heads/feature", 102),
("pull_request", "", "refs/pull/7/merge", 103),
("pull_request", "", "refs/pull/7/merge", 104),
]
for event, path, ref, run_id in cases:
# Evaluate the equivalent of GitHub's &&/|| conditional idiom.
value = path if event == "workflow_dispatch" and path else run_id
print(f"{event:18} path={path or '-':12} ref={ref:24} run_id={run_id} -> image-push-{value}")
PYRepository: NVIDIA/nvcf
Length of output: 537
Use a unique concurrency group for pull request runs.
cancel-in-progress: false keeps the running run but allows a newer run to replace the existing pending run. Use github.run_id for pull request runs and keep github.event.inputs.service_path for manual dispatches.
🤖 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/image-push-manual.yml around lines 42 - 43, Update the
concurrency group expression in the workflow to use github.run_id for pull
request runs while retaining github.event.inputs.service_path for manual
dispatches; preserve the existing github.ref fallback where applicable and leave
cancel-in-progress unchanged.
| mapfile -t subtrees < <( | ||
| git diff --name-only "${BASE_SHA}" "${HEAD_SHA}" \ | ||
| | grep -E '^src/[^/]+/[^/]+/' \ | ||
| | cut -d/ -f1-3 | sort -u | ||
| ) | ||
| if [ "${#subtrees[@]}" -eq 1 ] && [ -f "${subtrees[0]}/BUILD.bazel" ]; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Diff from the PR merge base.
git diff "${BASE_SHA}" "${HEAD_SHA}" compares two endpoint trees. If the base branch advances in another service after the PR branches, that service enters subtrees. The resolver then treats a single-service PR as multi-service and skips its image.
Use the merge-base form. git diff A...B compares the merge base of A and B with B. (git-scm.com)
Proposed fix
- git diff --name-only "${BASE_SHA}" "${HEAD_SHA}" \
+ git diff --name-only "${BASE_SHA}...${HEAD_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.
| mapfile -t subtrees < <( | |
| git diff --name-only "${BASE_SHA}" "${HEAD_SHA}" \ | |
| | grep -E '^src/[^/]+/[^/]+/' \ | |
| | cut -d/ -f1-3 | sort -u | |
| ) | |
| if [ "${#subtrees[@]}" -eq 1 ] && [ -f "${subtrees[0]}/BUILD.bazel" ]; then | |
| mapfile -t subtrees < <( | |
| git diff --name-only "${BASE_SHA}...${HEAD_SHA}" \ | |
| | grep -E '^src/[^/]+/[^/]+/' \ | |
| | cut -d/ -f1-3 | sort -u | |
| ) | |
| if [ "${`#subtrees`[@]}" -eq 1 ] && [ -f "${subtrees[0]}/BUILD.bazel" ]; then |
🤖 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/image-push-manual.yml around lines 80 - 85, Update the
subtree detection command in the workflow to use the merge-base diff form
between BASE_SHA and HEAD_SHA, so only changes introduced by the PR populate
subtrees; leave the subsequent single-subtree BUILD.bazel resolution logic
unchanged.
Four problems found by reviewing and exercising the first version rather than assuming it worked. Fork PRs fired the trigger, took a runner and died at the push step with an auth error, because secrets are withheld from forks. They are now filtered in the job condition, so they never start. `labeled` fires for every label. Adding an unrelated label to a PR that already carried deploy-to-stg triggered a full rebuild. The condition now also requires that the label being added is the one we care about; on `synchronize` and `reopened` there is no github.event.label, so that clause is true and the label-presence check governs. Pushing twice to a PR queued two builds when the first was already obsolete. PR builds now cancel superseded runs. Dispatch runs still queue, since each is deliberate and someone is waiting on its output. The skip message was wrong for a subtree that resolves but has no BUILD.bazel: it reported the multiple-subtree case and pointed at workflow_dispatch, which would not have helped. A change under src/libraries/go/lib resolves to src/libraries/go, which is not a buildable service. All three skip reasons now say what actually happened. Verified by extracting the resolve script from the workflow and running it against dispatch, one service, two services, docs only, and a library path. actionlint is clean across every workflow in the repository. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/image-push-manual.yml (1)
95-100: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftResolve the canonical service root.
When a PR changes only
src/libraries/rust/stargate, line 98 resolves it tosrc/libraries/rust. That directory has noBUILD.bazel, so the workflow setsfound=falseand skips the image push. Use the service-root contract from.github/workflows/bazel.ymlbefore theBUILD.bazelcheck.🤖 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/image-push-manual.yml around lines 95 - 100, Update the subtree resolution using the canonical service-root contract from bazel.yml before the BUILD.bazel check. Ensure changes under src/libraries/rust/stargate resolve to their service root rather than src/libraries/rust, then use that resolved root in the existing subtrees and BUILD.bazel validation so the image push is detected.
🤖 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.
Outside diff comments:
In @.github/workflows/image-push-manual.yml:
- Around line 95-100: Update the subtree resolution using the canonical
service-root contract from bazel.yml before the BUILD.bazel check. Ensure
changes under src/libraries/rust/stargate resolve to their service root rather
than src/libraries/rust, then use that resolved root in the existing subtrees
and BUILD.bazel validation so the image push is detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 543becb8-e777-4b14-959a-f20b27e9ecd3
📒 Files selected for processing (1)
.github/workflows/image-push-manual.yml
|
Moving to draft until the NGC token scope is confirmed. This PR auto-fires Auto-triggering on a label is only safe if that key is stg-scoped. Holding until |
3 participants
Why
Automatic dev image creation for developers, so a change can be deployed to
staging and tested before the PR merges.
What changed
Label a PR
deploy-to-stgand every push to it builds and pushes a dev image.Remove the label to stop. The image lands in ncp-dev as
gh.<run>-<sha8>, thesame convention
workflow_dispatchalready uses, so nothing downstream changes.A PR event carries no
service_pathinput, so a newresolvejob derives itfrom the changed files: take the
src/<plane>/<service>prefix of everythingchanged and require exactly one. Two services skips with a notice rather than
guessing.
workflow_dispatchstill passes the path explicitly and behavesexactly as before.
Customer Release Notes
Not customer visible.
Plan Summary
Not applicable. No release machinery is touched and no git tag is created, so
this cannot reach the production promotion path.
Usage
Label the PR, then deploy the image.
In the GitHub UI: open the PR, click the gear next to
Labelsin the rightsidebar, and select
deploy-to-stg.From the CLI:
The build starts as soon as the label is applied, and again on every push to
that PR. Find the resulting tag under the
image-push (manual)run, or deriveit:
gh.<run-number>-<first 8 of the head SHA>. Pull it from ncp-dev and deploythat tag on staging.
To stop the builds, remove the label:
Manual dispatch is unchanged for the cases this does not cover, such as a PR
touching two services or building from a branch with no PR.
Testing
The resolution logic was exercised against six cases before pushing: one
service, two services, docs only, a root file only, a service plus a root file,
and a top-level
srcfile. Only the single-service case builds; the rest skipwith a notice.
Not yet exercised end to end on a real labelled PR. The label does not exist in
the repository yet, so it needs creating before the trigger can fire, and the
first labelled PR is the real test.
Notes
Security: fork PRs receive no secrets, so the push fails closed rather than
leaking the registry token. Same-repo PRs get them, matching how the bazel
matrix already handles the cache token.
pull_requestis used rather thanpull_request_target, so untrusted code never runs with secrets in scope.The
concurrencygroup previously keyed ongithub.event.inputs.service_path,which is empty on a PR event and would have collapsed every PR into one group.
It now falls back to the ref.
Open question for the reviewer:
deploy-to-stgis my choice of label name andis easy to change.
References
None
Related Merge Requests/Pull Requests
None
Dependencies
None
Github commit:
feat(ci): build a dev image automatically on a labelled PR
Co-authored-by: Balaji Ganesan bganesan@nvidia.com
Summary by CodeRabbit