rerelease script - #5598
Open
ronnll wants to merge 1 commit into
Open
Conversation
Reviewer's GuideAdds a bash utility script to batch-create and monitor Release custom resources for a list of snapshots, with support for dry runs, logging, and configurable batch/wait parameters. Sequence diagram for batch rerelease script creating and monitoring Release CRssequenceDiagram
actor User
participant batch_rerelease_sh
participant OpenShift_cluster
User->>batch_rerelease_sh: ./hack/batch-rerelease.sh --input snapshots.txt
batch_rerelease_sh->>OpenShift_cluster: oc whoami
batch_rerelease_sh->>batch_rerelease_sh: mapfile SNAPSHOTS
loop For each batch
batch_rerelease_sh->>OpenShift_cluster: oc create -f - (Release CR)
OpenShift_cluster-->>batch_rerelease_sh: metadata.name
batch_rerelease_sh->>batch_rerelease_sh: wait_for_releases(names)
loop wait_for_releases polling
batch_rerelease_sh->>OpenShift_cluster: oc get release -o json
OpenShift_cluster-->>batch_rerelease_sh: status.conditions[Released].reason
alt [reason == Progressing or empty]
batch_rerelease_sh->>batch_rerelease_sh: sleep 30
else [reason is Succeeded/Failed]
batch_rerelease_sh->>batch_rerelease_sh: update SUCCEEDED/FAILED
end
end
batch_rerelease_sh->>batch_rerelease_sh: log batch progress
end
batch_rerelease_sh-->>User: Log file rerelease-results.log with summary
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider explicitly checking for the presence of required CLIs like
jq(similar to theoc whoamicheck) to fail fast with a clear error if dependencies are missing. - You might want to make
gracePeriodDaysa configurable option (with a default) instead of hardcoding7, so the script can be reused in contexts with different retention requirements.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider explicitly checking for the presence of required CLIs like `jq` (similar to the `oc whoami` check) to fail fast with a clear error if dependencies are missing.
- You might want to make `gracePeriodDays` a configurable option (with a default) instead of hardcoding `7`, so the script can be reused in contexts with different retention requirements.
## Individual Comments
### Comment 1
<location path="hack/batch-rerelease.sh" line_range="34-35" />
<code_context>
+while [[ $# -gt 0 ]]; do
+ case "$1" in
+ --input) INPUT="$2"; shift 2 ;;
+ --batch-size) BATCH_SIZE="$2"; shift 2 ;;
+ --max-wait) MAX_WAIT="$2"; shift 2 ;;
+ --namespace) NAMESPACE="$2"; shift 2 ;;
+ --release-plan) RELEASE_PLAN="$2"; shift 2 ;;
</code_context>
<issue_to_address>
**issue (bug_risk):** Validate that numeric options (batch-size, max-wait) are integers before using them in arithmetic.
Since these values come directly from user input and are used in arithmetic (e.g., `idx + BATCH_SIZE`, `attempts >= MAX_WAIT`), a non-integer will cause a shell arithmetic error. Please add input validation (for example, `[[ $BATCH_SIZE =~ ^[0-9]+$ ]]` and similarly for `MAX_WAIT`) and exit with a clear error message when the input is invalid.
</issue_to_address>
### Comment 2
<location path="hack/batch-rerelease.sh" line_range="117-118" />
<code_context>
+ done
+
+ for name in "${names[@]}"; do
+ local rel_json
+ rel_json=$(oc get release "$name" -n "$NAMESPACE" -o json 2>/dev/null) || true
+ local rel_result
+ rel_result=$(echo "$rel_json" | \
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle missing or failed `oc get release` calls explicitly to avoid misclassifying results as timeouts.
Right now, if `oc get release` fails (deleted Release, API error, etc.), `rel_json` is empty, `jq` yields null/empty, and we count it as `TIMED_OUT`. That mixes API/resource errors with real timeouts. Please treat empty `rel_json` explicitly (e.g., log "release not found" and either skip incrementing or use a separate category) so the metrics remain accurate and easier to interpret.
</issue_to_address>
### Comment 3
<location path="hack/batch-rerelease.sh" line_range="120-101" />
<code_context>
+ local rel_json
+ rel_json=$(oc get release "$name" -n "$NAMESPACE" -o json 2>/dev/null) || true
+ local rel_result
+ rel_result=$(echo "$rel_json" | \
+ jq -r '(.status.conditions // [])[] | select(.type == "Released") | .reason') || true
+ local snapshot
+ snapshot=$(echo "$rel_json" | jq -r '.spec.snapshot') || true
</code_context>
<issue_to_address>
**suggestion:** Guard against multiple `Released` conditions or missing condition, rather than assuming a single reason string.
The jq expression currently returns `.reason` for every `Released` condition, so `rel_result` may contain multiple lines while the `case` logic assumes a single value. Consider constraining this to one well-defined status (e.g., selecting the last matching condition) and also handling the scenario where there is no `Released` condition yet.
Suggested implementation:
```
local rel_result
rel_result=$(echo "$rel_json" | \
jq -r '(.status.conditions // [] | map(select(.type == "Released")) | last | .reason // "")') || true
```
This change:
1. Ensures only a single reason is used by mapping all `Released` conditions and taking the last one.
2. Safely handles the case where there are no `Released` conditions by returning an empty string (which already falls into the `*` branch of the `case` statement).
If you have other logic elsewhere that assumes `rel_result` is non-empty, you may want to add explicit handling for the empty-string case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+34
to
+35
| --batch-size) BATCH_SIZE="$2"; shift 2 ;; | ||
| --max-wait) MAX_WAIT="$2"; shift 2 ;; |
There was a problem hiding this comment.
issue (bug_risk): Validate that numeric options (batch-size, max-wait) are integers before using them in arithmetic.
Since these values come directly from user input and are used in arithmetic (e.g., idx + BATCH_SIZE, attempts >= MAX_WAIT), a non-integer will cause a shell arithmetic error. Please add input validation (for example, [[ $BATCH_SIZE =~ ^[0-9]+$ ]] and similarly for MAX_WAIT) and exit with a clear error message when the input is invalid.
Comment on lines
+117
to
+118
| local rel_json | ||
| rel_json=$(oc get release "$name" -n "$NAMESPACE" -o json 2>/dev/null) || true |
There was a problem hiding this comment.
issue (bug_risk): Handle missing or failed oc get release calls explicitly to avoid misclassifying results as timeouts.
Right now, if oc get release fails (deleted Release, API error, etc.), rel_json is empty, jq yields null/empty, and we count it as TIMED_OUT. That mixes API/resource errors with real timeouts. Please treat empty rel_json explicitly (e.g., log "release not found" and either skip incrementing or use a separate category) so the metrics remain accurate and easier to interpret.
| for name in "${names[@]}"; do | ||
| local rel_reason | ||
| rel_reason=$(oc get release "$name" -n "$NAMESPACE" -o json 2>/dev/null | \ | ||
| jq -r '(.status.conditions // [])[] | select(.type == "Released") | .reason') || true |
There was a problem hiding this comment.
suggestion: Guard against multiple Released conditions or missing condition, rather than assuming a single reason string.
The jq expression currently returns .reason for every Released condition, so rel_result may contain multiple lines while the case logic assumes a single value. Consider constraining this to one well-defined status (e.g., selecting the last matching condition) and also handling the scenario where there is no Released condition yet.
Suggested implementation:
local rel_result
rel_result=$(echo "$rel_json" | \
jq -r '(.status.conditions // [] | map(select(.type == "Released")) | last | .reason // "")') || true
This change:
- Ensures only a single reason is used by mapping all
Releasedconditions and taking the last one. - Safely handles the case where there are no
Releasedconditions by returning an empty string (which already falls into the*branch of thecasestatement).
If you have other logic elsewhere that assumesrel_resultis non-empty, you may want to add explicit handling for the empty-string case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a batch rerelease utility script for creating and monitoring Release CRs from a list of snapshots.
New Features: