refactor(platform): factor the cozystack-version stamp into a shared migration helper - #2980
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughIntroduces ChangesShared version-stamp helper and migration refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the version stamping logic for the Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the version-stamping logic in the migration scripts by centralizing the creation of the cozystack-version ConfigMap into a shared helper script (cozystack-version.sh). This ensures that the platform.cozystack.io/no-delete label is consistently applied across all migrations to prevent configuration drift. Additionally, unit tests for this helper script are introduced in hack/cozystack-version-stamp.bats. The review feedback suggests adding a parameter guard to the stamp_cozystack_version function to ensure robust error handling and prevent executing kubectl with empty input.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| stamp_cozystack_version() { | ||
| render_cozystack_version_manifest "$1" | kubectl apply --filename - | ||
| } |
There was a problem hiding this comment.
To ensure robust error handling and prevent executing kubectl with empty input, add a parameter guard to stamp_cozystack_version similar to the one in render_cozystack_version_manifest. This is especially important because if pipefail is not supported or enabled in the executing shell, a failure in render_cozystack_version_manifest might not be propagated correctly through the pipeline, potentially leading to unexpected behavior.
| stamp_cozystack_version() { | |
| render_cozystack_version_manifest "$1" | kubectl apply --filename - | |
| } | |
| stamp_cozystack_version() { | |
| : "${1:?stamp_cozystack_version: version argument required}" | |
| render_cozystack_version_manifest "$1" | kubectl apply --filename - | |
| } |
There was a problem hiding this comment.
Added the guard in 3117d02 — stamp_cozystack_version now fails fast on a missing version (: "${1:?...}") before the kubectl pipe, so it aborts regardless of whether the calling shell has pipefail enabled. Covered by a new unittest in the cozystack-version-stamp suite.
423b282 to
b20e934
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hack/cozystack-version-stamp.bats`:
- Around line 107-127: The grep patterns on lines 117 and 122 only check for the
explicit "create configmap" pattern but migrations could bypass the helper by
using "kubectl apply" instead. Expand the grep patterns to also detect kubectl
apply commands that could create or modify the cozystack-version configmap.
Either add an additional grep check for the kubectl apply pattern or modify the
existing grep expression to match both "create configmap" and "apply" approaches
to ensure the test properly enforces that migrations 42 and onward must use the
helper function.
🪄 Autofix (Beta)
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: Pro
Run ID: 3cad885e-4179-446f-8e42-f379a0c9b7f2
📒 Files selected for processing (6)
hack/cozystack-version-stamp.batspackages/core/platform/images/migrations/migrations/42packages/core/platform/images/migrations/migrations/43packages/core/platform/images/migrations/migrations/44packages/core/platform/images/migrations/migrations/lib/cozystack-version.shpackages/core/platform/images/migrations/run-migrations.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/platform/images/migrations/migrations/44
- packages/core/platform/images/migrations/migrations/43
- packages/core/platform/images/migrations/migrations/lib/cozystack-version.sh
- packages/core/platform/images/migrations/migrations/42
b20e934 to
46e7125
Compare
There was a problem hiding this comment.
Code Review
This pull request centralizes the creation and stamping of the cozystack-version ConfigMap by introducing a shared helper script cozystack-version.sh. This ensures that the ConfigMap consistently carries the platform.cozystack.io/no-delete: "true" label across migrations (42, 43, 44) and the migration runner. A new bats unit test suite is also added to verify the helper's functionality. The feedback suggests a minor improvement to the test script to strip comments before performing a grep check, preventing potential false positives.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| echo "$f does not call the cozystack-version helper API" >&2 | ||
| exit 1 | ||
| fi | ||
| if grep -Eq 'kubectl[[:space:]].*(create[[:space:]]+configmap|apply).*cozystack-version' "$f"; then |
There was a problem hiding this comment.
To prevent false positives from comments (e.g., if a developer writes a comment explaining why they are not using kubectl apply directly), we can strip comment lines before running the grep check.
if grep -v '^[[:space:]]*#' "$f" | grep -Eq 'kubectl[[:space:]].*(create[[:space:]]+configmap|apply).*cozystack-version'; then
There was a problem hiding this comment.
Done — the bypass-detection grep now strips full-comment lines (grep -v '^[[:space:]]*#') before matching, so an explanatory comment that mentions kubectl apply ... cozystack-version can't trip the guard. A real direct stamp never starts with #, so the guard keeps its strength against actual bypasses. All 10 bats cases stay green.
Extract the labeled cozystack-version ConfigMap stamp into a sourced shell helper so migrations cannot drift back to a label-less apply that strips the platform.cozystack.io/no-delete label. render_cozystack_version_manifest emits the manifest on stdout (pure, unit-testable); stamp_cozystack_version renders and applies it. The namespace defaults to cozy-system and honors $NAMESPACE. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…ations Migrations 42, 43 and 44 each carried a copy-pasted heredoc that applied the labeled cozystack-version ConfigMap. Source the shared helper and call it instead, so the labeled manifest has a single definition and future migrations cannot reintroduce a label-less apply. Behavior-preserving: the rendered manifest is byte-identical to the previous heredocs. Migration 42 keeps its MIGRATION_DRY_RUN path, now building its manifest from the helper's pure renderer. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The bootstrap path that creates cozystack-version when the ConfigMap is absent applied it without the platform.cozystack.io/no-delete label, leaving it unguarded by the cozystack-no-delete-guardrail ValidatingAdmissionPolicy. Route it through the shared helper so the bootstrap ConfigMap carries the label like every migration-driven stamp. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Mirror render_cozystack_version_manifest's argument guard in stamp_cozystack_version so a missing version aborts before the kubectl pipe. Without it, a render failure in a shell without pipefail would leave the pipeline exit status to kubectl, which would apply empty input. Add a unittest covering the guard. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…elper The helper header and run-migrations.sh claimed every migration-driven stamp routes through the helper, but migrations 1-41 still stamp the cozystack-version ConfigMap label-less and are backfilled by migration 42. Scope the comments to go-forward stamps (migration 42 onward and the bootstrap path), and add a unittest asserting no migration numbered >= 42 nor run-migrations.sh stamps cozystack-version without the helper, so the no-drift invariant is enforced rather than aspirational. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
46e7125 to
de44ecf
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
The refactor is mechanically correct, behavior-preserving, and closes a genuine bug class with a well-designed structural guardrail and covering tests.
Caveats
-
packages/core/platform/images/migrations/migrations/43and44previously hardcodednamespace: cozy-systemin their inline heredocs; they now use${NAMESPACE:-cozy-system}via the helper. In the current deployment path NAMESPACE is always set to the Helm release namespace (cozy-system) by the migration Job's env block inpackages/core/platform/templates/migration-hook.yaml:36, so the behavioral delta is zero. Worth noting for anyone adding a migration test against a non-default namespace. -
The bootstrap path in
run-migrations.sh(lines 14-22) is only reachable when the migration Job is invoked on a cluster that lacks thecozystack-versionConfigMap; on a normal fresh install the Job is not rendered (the$shouldRunMigrationHookcondition inmigration-hook.yaml:6-11stays false andtemplates/cozystack-version.yamlcreates the ConfigMap instead). The fix is still correct and closes the edge case where the Job runs on a manually deleted ConfigMap. -
run-migrations.shand all migration scripts use#!/bin/shwithset -euo pipefail.pipefailis not POSIX sh but is supported by busybox ash (the Alpine/bin/shinstalled in the image perDockerfile:6). This is a pre-existing pattern, not introduced here. -
SC3040/SC2086 shellcheck warnings visible in
run-migrations.sh(lines 39, 45, 46) are pre-existing; none are introduced by this PR. -
Coordination point for the release tag (not a defect of this PR):
packages/core/platform/values.yaml:8still pins the pre-PRplatform-migrationsimage digest. The release pipeline rebuilds it viamake image-migrationsand rewrites the digest, so this auto-resolves at tag-cut. Deploys frommainbetween merge and the next image rebuild will fail at. /migrations/lib/cozystack-version.sh(file not in the old image).
Recommended follow-ups
- The
hack/cozystack-version-stamp.batstests source the helper with a bare relative path (packages/core/platform/images/migrations/migrations/lib/cozystack-version.sh), so they must be run from the repository root. This is documented in the file header but is not enforced by CI integration noted in the bats file itself; confirmhack/cozytest.shis invoked from the repo root in the CI step that runs unit tests.
|
Successfully created backport PR for |
Backport of #3339 onto release-1.5, on top of the #3282 adopt-in-place fix (#3326). Three differences from main, all forced by what release-1.5 does not carry: * The repair migration is renumbered 53 -> 45 and stamps 46, since release-1.5 tops out at migration 44 with targetVersion 45. platform values.yaml goes to targetVersion 46. * Migrations 43 and 45 keep release-1.5's inline labelled stamp instead of lib/cozystack-version.sh. That helper is the shared stamp refactor (#2980), whose backport (#3065) was closed, so it does not exist on this branch. Only lib/seaweedfs-db-adopt.sh -- the part that actually carries the fix -- is sourced. * The extra/seaweedfs post-delete cleanup hook and its three tests are dropped. The hook arrived with #3092, a breaking change that was never backported, so there is nothing here for #3339's changes to it to apply to. Comments in lib/seaweedfs-db-adopt.sh and the recovery runbook that promised the hook would reclaim an orphaned volume are corrected to say it does not on 1.5.x. The migration renumbering is carried through the shared helper, the bats suite (STAMP 54 -> 46) and the runbook prose. Verified locally: helm unittest green for both seaweedfs charts (51 + 16 assertions) including both blind-upgrade guard canaries, and 34 bats assertions across migration-seaweedfs-db-adopt, seaweedfs-naming-audit and seaweedfs-guard-parity. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit 88e9fdcbbbff8aeab317b08557ca6bd6c8b5706d)
Backport of #3339 onto release-1.5, on top of the #3282 adopt-in-place fix (#3326). Three differences from main, all forced by what release-1.5 does not carry: * The repair migration is renumbered 53 -> 45 and stamps 46, since release-1.5 tops out at migration 44 with targetVersion 45. platform values.yaml goes to targetVersion 46. * Migrations 43 and 45 keep release-1.5's inline labelled stamp instead of lib/cozystack-version.sh. That helper is the shared stamp refactor (#2980), whose backport (#3065) was closed, so it does not exist on this branch. Only lib/seaweedfs-db-adopt.sh -- the part that actually carries the fix -- is sourced. * The extra/seaweedfs post-delete cleanup hook and its three tests are dropped. The hook arrived with #3092, a breaking change that was never backported, so there is nothing here for #3339's changes to it to apply to. Comments in lib/seaweedfs-db-adopt.sh and the recovery runbook that promised the hook would reclaim an orphaned volume are corrected to say it does not on 1.5.x. The migration renumbering is carried through the shared helper, the bats suite (STAMP 54 -> 46) and the runbook prose. Verified locally: helm unittest green for both seaweedfs charts (51 + 16 assertions) including both blind-upgrade guard canaries, and 34 bats assertions across migration-seaweedfs-db-adopt, seaweedfs-naming-audit and seaweedfs-guard-parity. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit 88e9fdcbbbff8aeab317b08557ca6bd6c8b5706d)
What this PR does
Every migration that stamps the
cozystack-versionConfigMap must carry theplatform.cozystack.io/no-delete=truelabel so thecozystack-no-delete-guardrailValidatingAdmissionPolicy guards it. The chart template renders the labeled ConfigMap only on first install, so each upgrade-time stamp has to re-emit the label by hand — migration 43 had to inline-copy migration 42's heredoc to avoid a label-less apply by the same field manager stripping it. The bug class was patched per-migration, not eliminated.This extracts the stamp into a shared, sourced helper (
render_cozystack_version_manifest+stamp_cozystack_version) underimages/migrations/migrations/lib/, and routes migrations 42, 43, 44 and therun-migrations.shbootstrap path through it. The labeled manifest now has a single definition, so future migrations cannot drift back to a label-less apply.The upgrade path is behavior-preserving: the rendered manifest is byte-identical to the previous heredocs (pinned by a golden unit test). The bootstrap path in
run-migrations.shpreviously created the ConfigMap without the label — it now carries it like every migration-driven stamp.Adds
hack/cozystack-version-stamp.bats(8 tests) covering the helper: label presence, ConfigMap name, quoted-string version type, namespace default and override, required-argument guard, and the byte-exact golden manifest.Closes #2781
Release note
Summary by CodeRabbit
Release Notes
Tests
cozystack-versionConfigMap, and quotes the version value correctly.kubectlConfigMap stamping.Refactor
cozystack-versionrendering and stamping into a shared helper and updated migrations and the migrations runner to use it consistently.