hack: fix OperatorConfig keeping#115
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe pull request enhances the deploy-catalog.sh script with a persistent backup mechanism for OperatorConfig. A global SAVED_CONFIG variable and cleanup() function are introduced to enable timestamped backups during uninstall, with restoration during redeploy featuring yq-based metadata cleanup and fallback logic. Changes
Sequence DiagramsequenceDiagram
participant User
participant Script as deploy-catalog.sh
participant K8s as Kubernetes<br/>(OperatorConfig)
participant yq as yq Tool
participant oc as oc Command
rect rgba(100, 150, 200, 0.5)
Note over User,oc: Uninstall Flow (--keep-config)
User->>Script: Execute with --keep-config
Script->>K8s: Query OperatorConfig
K8s-->>Script: OperatorConfig YAML
Script->>Script: Save to timestamped backup file
Script-->>User: Backup saved (SAVED_CONFIG set)
end
rect rgba(200, 150, 100, 0.5)
Note over User,oc: Redeploy Flow
User->>Script: Execute redeploy
Script->>Script: Locate SAVED_CONFIG backup
Script->>yq: Strip metadata fields?
alt yq available & succeeds
yq-->>Script: Cleaned YAML
Script->>oc: Apply cleaned config
else yq unavailable or fails
Script->>oc: Apply original config (fallback)
end
oc->>K8s: Apply OperatorConfig
K8s-->>Script: Applied
Script-->>User: Restoration complete or failed
end
rect rgba(150, 100, 200, 0.5)
Note over User,oc: Failure Scenario (Cleanup Trap)
Script->>Script: Error detected
Script->>Script: cleanup() trap triggered
alt --keep-config was set
Script->>User: Print restoration instructions<br/>(reference SAVED_CONFIG)
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hack/deploy-catalog.sh`:
- Around line 155-160: The backup step can leave an empty/orphaned $SAVED_CONFIG
file when `oc get operatorconfig -n ${NAMESPACE} -o yaml > "$SAVED_CONFIG"`
fails; in the else branch after detecting failure remove the partial file (e.g.
`rm -f "$SAVED_CONFIG"`) before clearing SAVED_CONFIG, and keep the echo "✗
Failed to save OperatorConfig" and the SAVED_CONFIG="" assignment so no stale
file is left behind.
🧹 Nitpick comments (2)
hack/deploy-catalog.sh (2)
469-484:yqversion compatibility:evalsubcommand is specific to Mike Farah's Go yq v4+.
command -v yqwill also match the Python yq (a jq wrapper) or older Go yq v3, both of which don't supporteval. The fallback chain handles this gracefully (failure → directoc apply), so it's not broken, but you could tighten the check to avoid the misleading error output.Optional: validate yq flavor before use
- if command -v yq &> /dev/null; then + if command -v yq &> /dev/null && yq --version 2>&1 | grep -q 'mikefarah'; then
471-471: Pipeline masksyqfailures withoutpipefail.Only
oc apply's exit code is checked. Ifyqfails but produces partial stdout thatoc applyaccepts, the restore would silently apply malformed config. In practiceoc applyrejects bad input, so the fallback kicks in — but a two-step approach is more explicit.Proposed: use a temp file to isolate yq failures
- if 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 -; then + CLEANED_CONFIG=$(mktemp) + if yq eval 'del(.items[].metadata.resourceVersion, .items[].metadata.uid, .items[].metadata.creationTimestamp, .items[].metadata.generation, .items[].metadata.managedFields, .items[].status)' "$SAVED_CONFIG" > "$CLEANED_CONFIG" 2>/dev/null && oc apply -f "$CLEANED_CONFIG"; then + rm -f "$CLEANED_CONFIG"Add
rm -f "$CLEANED_CONFIG"in the failure branches too (or add it to thecleanuptrap).
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
improve-deploy-catalog
branch
from
3f16657 to
ca0dabb
Compare
1 participant
Summary by CodeRabbit
New Features
Improvements