Warn when staging image lacks commit suffix#360
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds an else branch that logs a warning when a staging image tag lacks the expected ChangesStaging Image Suffix Validation
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — the change is a purely additive else branch that emits a warning annotation and does not alter the promotion logic or any exit paths. The warning fires only when staging_tag does not contain _, leaving the existing happy path and all exit-1 error paths completely untouched. staging_commit is still set to empty string in that branch, so the downstream production_image construction behaves identically to before. The two new spec assertions are straightforward substring checks consistent with the existing test style, and both strings match verbatim content in the workflow file. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[staging_tag extracted from staging_image] --> B{staging_tag contains '_'?}
B -- Yes --> C[staging_commit = suffix after last '_']
B -- No --> D["::warning:: Staging image did not include '_<commit>' suffix"]
D --> E[staging_commit remains empty string]
C --> F{staging_commit non-empty?}
E --> F
F -- Yes --> G["production_image = name:number_commit"]
F -- No --> H["production_image = name:number (no commit suffix)"]
Reviews (1): Last reviewed commit: "Warn when staging image lacks commit suf..." | Re-trigger Greptile |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| staging_commit="" | ||
| if [[ "${staging_tag}" == *_* ]]; then | ||
| staging_commit="${staging_tag##*_}" | ||
| else |
There was a problem hiding this comment.
The warning references ${STAGING_IMAGE} (the original, unnormalized input) while every surrounding message in this step uses ${staging_image} (the local, normalized variable). If the caller passes a digest-based ref like app:tag@sha256:abc, the warning will show the full @sha256:... portion rather than the processed value — fine for raw input visibility, but inconsistent with the rest of the error/warning messages in this block.
Consider using ${staging_image} to match the surrounding style:
| else | |
| echo "::warning::Staging image '${staging_image}' did not include a '_<commit>' suffix; production image tag will omit the commit suffix." |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| expect(contents).to include('if [[ "${staging_tag}" == *_* ]]; then') | ||
| expect(contents).to include('staging_commit="${staging_tag##*_}"') | ||
| expect(contents).to include("workflow-level concurrency group serializes this sequence") | ||
| expect(contents).to include("top-level concurrency group: cpflow-promote-staging-to-production") |
There was a problem hiding this comment.
reusable_promote_workflow_path resolves to Cpflow.root_path.join(".github/workflows/cpflow-promote-staging-to-production.yml") — the repo's own reusable workflow — not a file generated from the template at lib/github_flow_templates/. The generator template at lib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.yml has the same staging_commit code path but was not updated with the new warning or the concurrency-group comment, and there is no spec assertion covering that template path.
This means users who run cpflow generate_github_actions will receive a template that silently omits commit traceability without any warning — exactly the observability gap this PR aims to close.
Suggested fix: apply the same two-line change to lib/github_flow_templates/.github/workflows/cpflow-promote-staging-to-production.yml and add a parallel expect(contents).to include(...) assertion that reads from the generated (promote_workflow_path) file.
Code ReviewOverviewThis PR adds a The change is low-risk and the intent is clear. One functional gap and one minor style nit are noted below. Functional Gap — generator template not updatedThe most significant issue: This template is what Required fix: apply the same two additions to Minor Style Nit — variable reference in warning messageThe new warning emits Everything else looks good
|
65f3eb0 to
8ea704c
Compare
Code ReviewOverall: Clean, focused observability improvement. Low risk, appropriate scope. What this doesAdds a Strengths
Observations1. Warning fires when The outer
That message implies a tag exists but is just missing the suffix — the more accurate cause here is that the image has no recognisable tag at all. In practice CPLN images always carry a tag, so this is an unlikely edge case, but a guard like 2. Minor: The new SummaryApproving — the change is correct and improves debuggability. The empty-tag edge case is worth a follow-up if it ever surfaces in practice. |
| if [[ "${staging_tag}" == *_* ]]; then | ||
| staging_commit="${staging_tag##*_}" | ||
| else | ||
| echo "::warning::Staging image '${staging_image}' did not include a '_<commit>' suffix; production image tag will omit the commit suffix." |
There was a problem hiding this comment.
This else also fires when staging_tag="" — i.e. when staging_image contains neither : nor @, so the outer if/elif never assigns a value. An empty string does not match *_*, so the warning message ("did not include a _<commit> suffix") triggers, which implies a tag was present but malformed rather than absent entirely.
If the distinction matters, a dedicated guard before this block would be clearer:
| echo "::warning::Staging image '${staging_image}' did not include a '_<commit>' suffix; production image tag will omit the commit suffix." | |
| else | |
| if [[ -z "${staging_tag}" ]]; then | |
| echo "::warning::Staging image '${staging_image}' has no recognisable tag; production image tag will omit the commit suffix." | |
| else | |
| echo "::warning::Staging image '${staging_image}' did not include a '_<commit>' suffix; production image tag will omit the commit suffix." | |
| fi |
Low-priority — in practice CPLN images always carry a tag, so this path is unlikely to be hit.
| expect(contents).to include('if [[ "${staging_tag}" == *_* ]]; then') | ||
| expect(contents).to include('staging_commit="${staging_tag##*_}"') | ||
| expect(contents).to include("workflow-level concurrency group serializes this sequence") | ||
| expect(contents).to include("top-level concurrency group: cpflow-promote-staging-to-production") |
There was a problem hiding this comment.
Minor readability nit: the wrapper assertions are interleaved between two existing contents assertions. Grouping all wrapper expectations together (e.g. after the contents block) would make it easier to scan which subject each line targets. Not a correctness issue.
1 participant
Summary
Validation
Note
Low Risk
Only adds workflow warnings and comments; production tagging behavior when the suffix is missing was already unchanged.
Overview
The staging → production image copy step now emits a GitHub Actions
::warning::when the staging image tag has no_<commit>suffix, so promotions that produce production tags without commit traceability show up clearly in logs instead of failing silently.A short comment in the same step points reviewers at the workflow-level concurrency group
cpflow-promote-staging-to-production, which serializes tag allocation. The same changes are applied to the checked-in workflow and the generator template.Generator specs now assert the warning text and concurrency comment appear in both the reusable and caller wrapper promote workflows.
Reviewed by Cursor Bugbot for commit 8ea704c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit