Skip to content

test(deploy): assert production helm upgrade keeps --atomic (BLO-21492) - #1084

Closed
allyblockcast[bot] wants to merge 1 commit into
masterfrom
platformsre/blo-21492-atomic-flag-test-coverage
Closed

test(deploy): assert production helm upgrade keeps --atomic (BLO-21492)#1084
allyblockcast[bot] wants to merge 1 commit into
masterfrom
platformsre/blo-21492-atomic-flag-test-coverage

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown

Thinking Path

Linked Issues or Issue Description

Follow-up to Blockcast/paperclip#998, refs BLO-21492. No new GitHub issue — same tracker issue, addressing a review suggestion left open on the merged PR.

  • I searched the GitHub PR list (open + recently closed) for similar PRs and confirmed this is not a duplicate.

What Changed

  • Added a test in scripts/check-docker-deploy-timeout.test.js asserting the production helm upgrade "${RELEASE}" invocation retains --atomic between the helm upgrade call and where helm_pid=$! captures its PID (i.e. on the actual upgrade command, not just anywhere in the job).

Verification

  • node --test ./scripts/check-docker-deploy-timeout.test.js — 13/13 pass (was 12; new test included).
  • Manually verified the new assertion actually fails: temporarily stripped --atomic from .github/workflows/docker.yml locally, reran the test suite, confirmed the new test fails with the expected message, then reverted (not committed).

Risks

None — test-only change, no workflow behavior modified.

Model Used

Claude, claude-sonnet-5[1m] (Sonnet 5, 1M context), via Claude Code — no extended thinking mode, standard tool use.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)

#998 added --atomic to the production helm upgrade so a resource denied
mid-upgrade (e.g. by an admission policy) rolls the whole release back
instead of leaving tiers on different image digests. Ally's review on
that PR flagged that no test would catch a future regression dropping
the flag -- the existing timeout-margin test only checks the numeric
--wait --timeout adjacency, not that --atomic is present at all.

Add a dedicated assertion; verified it fails when --atomic is removed.
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-21492

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-21492

@kkroo

kkroo commented Aug 6, 2026

Copy link
Copy Markdown

Superseded by #1099, which carries the exact same head under independent PR author kkroo so Ally can provide the required App approval.

@kkroo kkroo closed this Aug 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a manual request Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants