fix(installer): add keep annotation to Namespace and update migration script#2122
fix(installer): add keep annotation to Namespace and update migration script#2122
Conversation
… script Add helm.sh/resource-policy=keep annotation to the cozy-system Namespace in the installer helm chart. This prevents Helm from deleting the namespace when the HelmRelease is removed, which would otherwise destroy all other HelmReleases within it. Update the migration script to annotate the cozy-system namespace and cozystack-version ConfigMap with helm.sh/resource-policy=keep before generating the Package resource. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
📝 WalkthroughWalkthroughThese changes add resource protection mechanisms to prevent critical Kubernetes resources from being deleted during Helm operations. A migration script now includes an interactive step to annotate resources with the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 enhances the robustness of the system by implementing a mechanism to prevent the accidental deletion of critical Kubernetes resources during Helm uninstallations. It ensures that essential components like the Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Changelog
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 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 counter productive. You can react with 👍 and 👎 on @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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
kvaps
requested review from
IvanHunters,
androndo,
lexfrei,
lllamnyp and
sircthulhu
as code owners
dosubot
bot
added
the
size:S
kvaps
added
backport
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds the helm.sh/resource-policy: keep annotation to prevent accidental deletion of the cozy-system namespace and its contents. The changes in the Helm chart and the migration script are logical and well-implemented. I have one suggestion for the migration script to improve its error handling robustness, making it fail explicitly on unexpected errors instead of silently continuing.
| echo "Annotating ConfigMap cozystack-version..." | ||
| kubectl annotate configmap -n "$NAMESPACE" cozystack-version helm.sh/resource-policy=keep --overwrite 2>/dev/null || echo " ConfigMap cozystack-version not found, skipping." |
There was a problem hiding this comment.
The current implementation to annotate the cozystack-version ConfigMap suppresses all errors from kubectl and prints a "not found, skipping" message. This could be misleading if the failure is due to other reasons, such as insufficient permissions. A more robust approach is to first verify the ConfigMap's existence before attempting to annotate it. This ensures that only "not found" cases are skipped, while other errors will correctly cause the script to fail due to the set -e directive.
| echo "Annotating ConfigMap cozystack-version..." | |
| kubectl annotate configmap -n "$NAMESPACE" cozystack-version helm.sh/resource-policy=keep --overwrite 2>/dev/null || echo " ConfigMap cozystack-version not found, skipping." | |
| if kubectl get configmap -n "$NAMESPACE" cozystack-version &>/dev/null; then | |
| echo "Annotating ConfigMap cozystack-version..." | |
| kubectl annotate configmap -n "$NAMESPACE" cozystack-version helm.sh/resource-policy=keep --overwrite | |
| else | |
| echo " ConfigMap cozystack-version not found, skipping." | |
| fi |
dosubot
bot
added
the
size:S
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/migrate-to-version-1.0.sh`:
- Line 50: Replace the blind 2>/dev/null suppression on the kubectl annotate
command for the ConfigMap named cozystack-version and instead capture its
stderr/stdout; if the command fails, inspect the captured output for a NotFound
(or "not found") message and only then echo the "ConfigMap ... not found,
skipping" message, otherwise print the real error and exit non‑zero so
RBAC/connection errors (and other failures) are surfaced; locate the kubectl
annotate invocation that uses the NAMESPACE and adjust error handling
accordingly.
| echo "Annotating namespace $NAMESPACE..." | ||
| kubectl annotate namespace "$NAMESPACE" helm.sh/resource-policy=keep --overwrite | ||
| echo "Annotating ConfigMap cozystack-version..." | ||
| kubectl annotate configmap -n "$NAMESPACE" cozystack-version helm.sh/resource-policy=keep --overwrite 2>/dev/null || echo " ConfigMap cozystack-version not found, skipping." |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the exact line 50 and surrounding context
echo "=== Line 50 and context ==="
sed -n '45,55p' hack/migrate-to-version-1.0.sh
echo ""
echo "=== Searching for the kubectl annotate pattern ==="
rg -n 'kubectl annotate configmap' hack/migrate-to-version-1.0.sh
echo ""
echo "=== Checking for 2>/dev/null error masking pattern ==="
rg -n -C2 'kubectl annotate configmap.*2>/dev/null.*echo' hack/migrate-to-version-1.0.sh || trueRepository: cozystack/cozystack
Length of output: 1368
Differentiate between "not found" and other kubectl failures.
The current approach masks all kubectl annotate errors with 2>/dev/null, then assumes any failure means the ConfigMap doesn't exist. This silently hides RBAC denials, connectivity issues, or other errors while falsely reporting them as "not found," potentially leaving the deployment in an inconsistent state.
Capture the error output and only treat NotFound errors as expected; fail explicitly on other errors:
Proposed fix
- kubectl annotate configmap -n "$NAMESPACE" cozystack-version helm.sh/resource-policy=keep --overwrite 2>/dev/null || echo " ConfigMap cozystack-version not found, skipping."
+ annotate_err=""
+ if ! annotate_err=$(kubectl annotate configmap -n "$NAMESPACE" cozystack-version helm.sh/resource-policy=keep --overwrite 2>&1); then
+ if grep -q "NotFound" <<<"$annotate_err"; then
+ echo " ConfigMap cozystack-version not found, skipping."
+ else
+ echo "$annotate_err" >&2
+ exit 1
+ fi
+ fi📝 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.
| kubectl annotate configmap -n "$NAMESPACE" cozystack-version helm.sh/resource-policy=keep --overwrite 2>/dev/null || echo " ConfigMap cozystack-version not found, skipping." | |
| annotate_err="" | |
| if ! annotate_err=$(kubectl annotate configmap -n "$NAMESPACE" cozystack-version helm.sh/resource-policy=keep --overwrite 2>&1); then | |
| if grep -q "NotFound" <<<"$annotate_err"; then | |
| echo " ConfigMap cozystack-version not found, skipping." | |
| else | |
| echo "$annotate_err" >&2 | |
| exit 1 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/migrate-to-version-1.0.sh` at line 50, Replace the blind 2>/dev/null
suppression on the kubectl annotate command for the ConfigMap named
cozystack-version and instead capture its stderr/stdout; if the command fails,
inspect the captured output for a NotFound (or "not found") message and only
then echo the "ConfigMap ... not found, skipping" message, otherwise print the
real error and exit non‑zero so RBAC/connection errors (and other failures) are
surfaced; locate the kubectl annotate invocation that uses the NAMESPACE and
adjust error handling accordingly.
|
Successfully created backport PR for |
1 participant
What this PR does
Adds
helm.sh/resource-policy: keepannotation to thecozy-systemNamespace resourcein the installer helm chart. This prevents Helm from deleting the namespace (and all
HelmReleases within it) when the installer release is removed.
Also updates the v1.0 migration script to annotate the
cozy-systemnamespace andcozystack-versionConfigMap with the same policy before generating the Package resource.Release note
Summary by CodeRabbit