ci: concurrency control, precise Go caching, tighter retention, dedupe docs-PR logic - #2328
ci: concurrency control, precise Go caching, tighter retention, dedupe docs-PR logic#2328OmkarDeshpande7 wants to merge 3 commits into
Conversation
✅ Security Vulnerability SummaryNo change in security posture 📊 Overall Changes
🔍 Detailed Breakdown📦 Gosec (Static Analysis)
📦 Trivy (Dependency Scan)
📋 Baseline Methods
Only HIGH and CRITICAL severity vulnerabilities are tracked |
…ention, dedupe docs-PR logic - Add concurrency groups to stop stacked/duplicate CI runs on rapid pushes: workflow-level for security.yml and golangci-lint.yaml, job-level for packer.yml (push-images uses cancel-in-progress:false since it does real docker push, so it queues instead of being killed mid-push). Guard security-comment.yml against posting a false "0 vulnerabilities" comment when the scan run is cancelled. - Pin cache-dependency-path to k8s/migration/go.sum on the setup-go steps that build/lint that module specifically (packer.yml, pre_release.yaml, golangci-lint.yaml), so bumping a dependency in an unrelated Go module no longer invalidates their cache. - Set retention-days: 1 on packer.yml's qcow2/s3-urls/yamls artifacts to match the retention already used by the other build artifacts in the same file; Quay/S3 remain the durable copies. - Extract the byte-identical "archive old release notes" step and the near-identical "create PR" step (shared by 01_update_release_notes.yaml and 02_backfill_docs.yaml) into two composite actions under .github/actions/, referenced via owner/repo/path@main since these workflows check out gh-pages, not main. create-docs-pr takes a retry input so each caller keeps its existing behavior exactly (01: no retry, 02: 3 attempts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Step 6 launched the shared skill-creator plugin's eval-viewer, which has no GitHub-posting feature at all (confirmed by grepping every copy on disk) — so the skill's promised "Post to GitHub" checkboxes never existed. This repo already has its own working copy at .claude/eval-viewer/ with a real send_comments.sh parser and /api/post-comments endpoint; the skill just wasn't pointed at it. Repoints Step 6 there, drops the now-unused findings.json generation, and gitignores the skill's own workspace + eval-viewer's __pycache__. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1c62a85 to
774a045
Compare
🚨 Security Vulnerability SummarySecurity posture degraded 📊 Overall Changes
🔍 Detailed Breakdown📦 Gosec (Static Analysis)
📦 Trivy (Dependency Scan)
📋 Baseline Methods
🚨 Added VulnerabilitiesTrivy (Dependencies) - 3 AddedTarget: Target: Target: Only HIGH and CRITICAL severity vulnerabilities are tracked |
| runs-on: ubuntu-latest | ||
| concurrency: | ||
| group: ${{ github.workflow }}-determine-release-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
Nightly is not a separate workflow. nightly-build-trigger.yml:72 dispatches packer.yml with ref: 'main'. So nightly and push-to-main both resolve github.ref → refs/heads/main → same group.
What happens:
Merge to main during a nightly → nightly's build-* jobs cancelled → push-images sees cancelled (its if: accepts only success/skipped) → skipped → packer/qcow2 never runs → no nightly image that day, zero failures reported.
Reverse: 19:30 UTC nightly cancels an in-flight push run's builds → that commit's dev images never reach Quay.
push-images having cancel-in-progress: false doesn't save it — upstream builds already dead. And GitHub keeps only one pending entry per group, so a queued nightly push-images can also get evicted.
Add event discriminator to all seven group keys
| fi | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| uses: platform9/vjailbreak/.github/actions/create-docs-pr@main |
There was a problem hiding this comment.
01_update_release_notes.yaml:150,154 + 02_backfill_docs.yaml:94,99
These four uses: point at @main, an unpinned floating ref — every other action in this repo is pinned (actions/checkout@v4, oras-project/setup-oras@v1.2.1), so a bad commit to main breaks the release-notes workflow instantly. Also worth noting the actions don't exist on main until this lands: cherry-picking this onto a release-* branch first makes both workflows hard-fail at uses: resolution.
| git config --global user.name "GitHub Actions Bot" | ||
|
|
||
| TIMESTAMP=$(date +%s) | ||
| NEW_BRANCH="${{ inputs.branch-prefix }}-$TIMESTAMP" |
There was a problem hiding this comment.
.github/actions/create-docs-pr/action.yml:35,41,46,60
${{ inputs.branch-prefix }}, commit-message, pr-title and pr-body are interpolated straight into the shell, so a release tag containing " or $(...) becomes code. Not a regression (the old inline scripts did the same), but since env: is already there at line 28 for GH_TOKEN, add the four as env vars too and use "$COMMIT_MESSAGE" / "$PR_TITLE" / "$PR_BODY" / "$BRANCH_PREFIX".
| image_builder/deploy/version-checker.yaml | ||
| image_builder/deploy/vjailbreak-settings.yaml No newline at end of file | ||
| image_builder/deploy/vjailbreak-settings.yaml | ||
| retention-days: 1 No newline at end of file |
There was a problem hiding this comment.
retention-days: 1 is right for vjailbreak-qcow2 (601) and s3-urls (671) — both are gated on release_found || is_nightly and also land on Quay + S3. But vjailbreak-yamls here is uploaded unconditionally, downloaded by no workflow, and is the manifest set people fetch by hand off a release run. Suggest retention-days: 7
| # Only run for PRs (not pushes to main or releases) | ||
| if: github.event.workflow_run.event == 'pull_request' | ||
| # Only run for PRs (not pushes to main or releases), and only when the scan actually completed | ||
| if: github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.conclusion == 'success' |
There was a problem hiding this comment.
Good pairing with the new cancel-in-progress: true on security.yml — without it a cancelled scan would post a bogus "0 vulnerabilities". One side effect: gosec_scan and trivy_scan are independent jobs in the same workflow, so a flaky trivy_scan (DB fetch, go install ...@latest) now suppresses the gosec comment too even though its artifact uploaded fine. If you want comments on partially-failed scans, conclusion != 'cancelled' gets the cancel protection without that coupling — otherwise fine as-is, just confirming it's deliberate.
…tion
- packer.yml: add github.event_name into all 7 concurrency group keys.
nightly-build-trigger.yml dispatches packer.yml with ref: 'main', so a
nightly workflow_dispatch run and a push-to-main run both resolved
github.ref to refs/heads/main and shared a group — a merge during a
nightly window could cancel its build jobs (or vice versa), silently
skipping push-images since its if: only accepts success/skipped.
- create-docs-pr/action.yml: branch-prefix/commit-message/pr-title/pr-body
were interpolated directly into the shell via ${{ inputs.x }}, so a
release tag or PR title containing a shell metacharacter would execute
as code. Move them into env: alongside GH_TOKEN and reference via
"$VAR" instead.
- packer.yml: vjailbreak-yamls retention-days 1 -> 7. Unlike qcow2/s3-urls
(gated on release_found||is_nightly, also land on Quay+S3), this
artifact uploads unconditionally with no other durable copy and is
fetched by hand off release runs.
- security-comment.yml: narrow the cancellation guard from
conclusion == 'success' to conclusion != 'cancelled'. gosec_scan and
trivy_scan are independent jobs — requiring full success meant a flaky
trivy_scan suppressed the gosec comment too even though its artifact
uploaded fine.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🚨 Security Vulnerability SummarySecurity posture degraded 📊 Overall Changes
🔍 Detailed Breakdown📦 Gosec (Static Analysis)
📦 Trivy (Dependency Scan)
📋 Baseline Methods
🚨 Added VulnerabilitiesTrivy (Dependencies) - 3 AddedTarget: Target: Target: Only HIGH and CRITICAL severity vulnerabilities are tracked |
2 participants
Summary
security.yml/golangci-lint.yaml, job-level forpacker.yml(push-imagesqueues instead of being cancelled mid-docker push). Guardssecurity-comment.ymlagainst posting a false "0 vulnerabilities" comment when a scan run is cancelled.cache-dependency-path: k8s/migration/go.sumon the setup-go steps that build/lint that module (packer.yml,pre_release.yaml,golangci-lint.yaml) so an unrelated module's dependency bump no longer busts their Go cache.retention-days: 1on packer.yml'svjailbreak-qcow2/s3-urls/vjailbreak-yamlsartifacts, matching the retention already used by the other build artifacts in that file (Quay/S3 remain the durable copies).01_update_release_notes.yamland02_backfill_docs.yamlinto two composite actions under.github/actions/, referenced viaowner/repo/path@mainsince both workflows checkoutgh-pages, notmain.create-docs-prtakes aretryinput so each caller keeps its exact existing behavior (01: no retry, 02: 3 attempts).Scoped deliberately to changes with no behavior side effects — did not touch Docker layer caching, Trivy/gosec download caching, or the divergent release-notes-generation logic (02 has extra skopeo/multi-tag looping 01 lacks), all flagged as separate, riskier follow-ups.
Test plan
actionlintclean on all changed workflow files and the two new composite actions — zero new findings (2 pre-existing unrelated findings remain:01_update_release_notes.yaml:31input type,packer.yml:81untrusted expression)needs:/if:conditions by hand to confirm job graphs behave identicallypacker.ymlconcurrency groups and the composite-action wiring in01_update_release_notes.yaml/02_backfill_docs.yamlon review🤖 Generated with Claude Code