Add --keep-config flag to preserve OperatorConfig during redeploy#107
Conversation
to avoid losing OperatorConfig Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
📝 WalkthroughWalkthroughThe deploy catalog script now supports a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/deploy-catalog.sh (1)
57-70:⚠️ Potential issue | 🟠 MajorCluster context is not displayed when
-yis passed.The coding guideline requires: "Before running deploy-catalog.sh (redeploy or uninstall), verify the cluster context by running
oc whoami --show-server && oc whoami, display the cluster URL and user to the user, and only proceed with -y flag after user confirms."Currently,
-yskips the entire block, so the user never sees which cluster/user the script will target. At minimum, the cluster info should still be printed even when the confirmation prompt is skipped.Suggested change
if [ "$COMMAND" != "build" ] && [ "$SKIP_CONFIRM" = false ]; then CLUSTER_URL=$(oc whoami --show-server 2>/dev/null || echo "unknown") CLUSTER_USER=$(oc whoami 2>/dev/null || echo "unknown") echo "" echo " Cluster: ${CLUSTER_URL}" echo " User: ${CLUSTER_USER}" echo " Action: ${COMMAND}" echo "" read -r -p "Proceed? [y/N] " response if [[ ! "$response" =~ ^[Yy]$ ]]; then echo "Aborted." exit 0 fi +elif [ "$COMMAND" != "build" ]; then + CLUSTER_URL=$(oc whoami --show-server 2>/dev/null || echo "unknown") + CLUSTER_USER=$(oc whoami 2>/dev/null || echo "unknown") + echo "" + echo " Cluster: ${CLUSTER_URL}" + echo " User: ${CLUSTER_USER}" + echo " Action: ${COMMAND} (auto-confirmed with -y)" + echo "" fiAs per coding guidelines: "Before running deploy-catalog.sh (redeploy or uninstall), verify the cluster context by running
oc whoami --show-server && oc whoami, display the cluster URL and user to the user, and only proceed with -y flag after user confirms."
🤖 Fix all issues with AI agents
In `@hack/deploy-catalog.sh`:
- Around line 118-124: The script currently saves OperatorConfig to SAVED_CONFIG
when KEEP_CONFIG is true but the restore only runs for redeploy, so running with
COMMAND=uninstall or build silently does nothing useful; fix by rejecting the
incompatible flag: after parsing flags, add a check that if KEEP_CONFIG=true and
COMMAND is "uninstall" or "build" then print a clear error (e.g. "ERROR:
--keep-config is not supported with 'uninstall' or 'build'") and exit 1;
reference the KEEP_CONFIG and COMMAND variables (and the SAVED_CONFIG save
block) so the check runs before creating the temp file, ensuring users are
prevented from thinking their config was preserved.
- Line 427: Add a guard that verifies yq is installed before using it (similar
to the existing opm check) to avoid silent failures when running the yq pipeline
on SAVED_CONFIG; detect yq with a simple binary check (e.g., command -v yq or
which yq) and if missing, print a clear error and exit (or handle fallback
behavior), ensuring the check occurs before the line that pipes yq eval into oc
apply so the script will fail fast and not leave the operator in a
partially-configured state.
🧹 Nitpick comments (1)
hack/deploy-catalog.sh (1)
424-428: Saved config can be lost if the restore fails.If
yqoroc applyfails on line 427,set -ekills the script before line 428 (rm -f), so the temp file survives — but only by accident. The user gets no guidance on where it is or how to retry. Consider adding atrapto print the temp file path on error, or move thermto an explicit cleanup after confirmed success with a message.
| # Save OperatorConfig if --keep-config was specified | ||
| if [ "$KEEP_CONFIG" = true ]; then | ||
| echo "Saving OperatorConfig CRs..." | ||
| SAVED_CONFIG=$(mktemp /tmp/operatorconfig-XXXXXX.yaml) | ||
| oc get operatorconfig -n ${NAMESPACE} -o yaml > "$SAVED_CONFIG" 2>/dev/null || true | ||
| echo " Saved to $SAVED_CONFIG" | ||
| fi |
There was a problem hiding this comment.
--keep-config silently does nothing useful with the uninstall command.
When $0 uninstall --keep-config is run, the config is saved to a temp file here, but the restore logic (line 424) only runs during redeploy. The user gets a "Saved to …" message suggesting the config is preserved, but the script exits at line 178 without restoring or even informing the user where the file is for manual restore.
Consider either:
- Rejecting
--keep-configwhenCOMMAND=uninstall(orbuild), or - Printing a clear message at exit with the temp file path so the user can restore manually.
Option: reject incompatible flag
COMMAND="${COMMAND:-redeploy}"
+
+if [ "$KEEP_CONFIG" = true ] && [ "$COMMAND" != "redeploy" ]; then
+ echo "ERROR: --keep-config is only supported with the redeploy command"
+ exit 1
+fi🤖 Prompt for AI Agents
In `@hack/deploy-catalog.sh` around lines 118 - 124, The script currently saves
OperatorConfig to SAVED_CONFIG when KEEP_CONFIG is true but the restore only
runs for redeploy, so running with COMMAND=uninstall or build silently does
nothing useful; fix by rejecting the incompatible flag: after parsing flags, add
a check that if KEEP_CONFIG=true and COMMAND is "uninstall" or "build" then
print a clear error (e.g. "ERROR: --keep-config is not supported with
'uninstall' or 'build'") and exit 1; reference the KEEP_CONFIG and COMMAND
variables (and the SAVED_CONFIG save block) so the check runs before creating
the temp file, ensuring users are prevented from thinking their config was
preserved.
| if [ "$KEEP_CONFIG" = true ] && [ -n "${SAVED_CONFIG:-}" ] && [ -s "$SAVED_CONFIG" ]; then | ||
| echo "Restoring saved OperatorConfig..." | ||
| # Strip resourceVersion/uid/creationTimestamp so apply works cleanly | ||
| yq eval 'del(.items[].metadata.resourceVersion, .items[].metadata.uid, .items[].metadata.creationTimestamp, .items[].metadata.generation, .items[].metadata.managedFields, .items[].status)' "$SAVED_CONFIG" | oc apply -f - |
There was a problem hiding this comment.
yq is used without verifying it's installed.
The script checks for opm (line 282) but not for yq. If yq is missing, the restore fails and set -e aborts the script — leaving the operator installed but without any OperatorConfig, which is arguably worse than the problem --keep-config is meant to solve.
Suggested guard
+ if ! command -v yq &>/dev/null; then
+ echo "ERROR: yq is required for --keep-config but not found in PATH"
+ echo " Saved config is at: $SAVED_CONFIG"
+ exit 1
+ fi
yq eval 'del(.items[].metadata.resourceVersion, .items[].metadata.uid, .items[].metadata.creationTimestamp, .items[].metadata.generation, .items[].metadata.managedFields, .items[].status)' "$SAVED_CONFIG" | oc apply -f -📝 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.
| yq eval 'del(.items[].metadata.resourceVersion, .items[].metadata.uid, .items[].metadata.creationTimestamp, .items[].metadata.generation, .items[].metadata.managedFields, .items[].status)' "$SAVED_CONFIG" | oc apply -f - | |
| if ! command -v yq &>/dev/null; then | |
| echo "ERROR: yq is required for --keep-config but not found in PATH" | |
| echo " Saved config is at: $SAVED_CONFIG" | |
| exit 1 | |
| fi | |
| yq eval 'del(.items[].metadata.resourceVersion, .items[].metadata.uid, .items[].metadata.creationTimestamp, .items[].metadata.generation, .items[].metadata.managedFields, .items[].status)' "$SAVED_CONFIG" | oc apply -f - |
🤖 Prompt for AI Agents
In `@hack/deploy-catalog.sh` at line 427, Add a guard that verifies yq is
installed before using it (similar to the existing opm check) to avoid silent
failures when running the yq pipeline on SAVED_CONFIG; detect yq with a simple
binary check (e.g., command -v yq or which yq) and if missing, print a clear
error and exit (or handle fallback behavior), ensuring the check occurs before
the line that pipes yq eval into oc apply so the script will fail fast and not
leave the operator in a partially-configured state.
bennyz
changed the title
1 participant
to avoid losing OperatorConfig
Summary by CodeRabbit
--keep-configflag for deployments to preserve configuration settings during redeploys, eliminating the need to reconfigure after updates.