BAC-492 - Prevent Rollout From Losing spec.replicas During ArgoCD Syn…#62
Closed
BAC-492 - Prevent Rollout From Losing spec.replicas During ArgoCD Syn…#62
Conversation
…c When Re-enabling HPA
WalkthroughAdds a conditional CI step "Prepare HPA transition" to the Argo deploy workflow and a new script Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
Implement HPA transition preparation script and update ArgoCD deployment steps to clean spec.replicas from last-applied-configuration. This change prevents conflicts during HPA transitions by ensuring the correct replica count is maintained.
![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
🤖 Fix all issues with AI agents
In `@src/scripts/argo_hpa_transition.sh`:
- Around line 48-55: The hpa_enabled extraction is brittle for quoted or
capitalized values; update the hpa_enabled assignment (the variable computed
from "$parameters_file") to strip surrounding quotes and whitespace and
normalize to lowercase before comparing to "true". Locate the hpa_enabled
assignment and replace the current pipeline with one that extracts the value
token, removes surrounding quotes, trims whitespace, and lowercases it (so
values like "true", True, TRUE all normalize to true) and then keep the existing
if [[ "$hpa_enabled" != "true" ]]; check.
- Around line 35-46: The validation currently only checks that the
parameters_file variable is set; update the check so it also fails fast when the
file path is missing or not readable. Replace the single-variable check for
parameters_file with a compound test like [[ -z "$parameters_file" || ! -r
"$parameters_file" ]] and add a clear label such as "ARGO_PARAMETERS_FILE
(missing or unreadable)" to missing_vars so the script exits immediately if the
file does not exist or is not readable; keep the rest of the missing_vars
handling (usage and exit 2) unchanged.
Comment on lines
+35
to
+46
| # Validate required variables | ||
| local missing_vars=() | ||
| [[ -z "$parameters_file" ]] && missing_vars+=("ARGO_PARAMETERS_FILE") | ||
| [[ -z "$release_name" ]] && missing_vars+=("RELEASE_NAME") | ||
| [[ -z "$namespace" ]] && missing_vars+=("NAMESPACE") | ||
|
|
||
| if [[ ${#missing_vars[@]} -gt 0 ]]; then | ||
| echo -e "${RED}Error: The following required environment variables are missing:${NC}" | ||
| printf ' - %s\n' "${missing_vars[@]}" | ||
| usage | ||
| exit 2 | ||
| fi |
There was a problem hiding this comment.
Fail fast if ARGO_PARAMETERS_FILE is missing or unreadable.
Right now only the variable is checked; if the file path is wrong, Line 50 will silently evaluate to "not enabled" and skip cleanup, reintroducing the replica-loss bug.
🛠️ Suggested fix
if [[ ${`#missing_vars`[@]} -gt 0 ]]; then
echo -e "${RED}Error: The following required environment variables are missing:${NC}"
printf ' - %s\n' "${missing_vars[@]}"
usage
exit 2
fi
+
+ if [[ ! -r "$parameters_file" ]]; then
+ echo -e "${RED}Error: ARGO_PARAMETERS_FILE '$parameters_file' not found or not readable.${NC}"
+ exit 2
+ 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.
Suggested change
| # Validate required variables | |
| local missing_vars=() | |
| [[ -z "$parameters_file" ]] && missing_vars+=("ARGO_PARAMETERS_FILE") | |
| [[ -z "$release_name" ]] && missing_vars+=("RELEASE_NAME") | |
| [[ -z "$namespace" ]] && missing_vars+=("NAMESPACE") | |
| if [[ ${#missing_vars[@]} -gt 0 ]]; then | |
| echo -e "${RED}Error: The following required environment variables are missing:${NC}" | |
| printf ' - %s\n' "${missing_vars[@]}" | |
| usage | |
| exit 2 | |
| fi | |
| # Validate required variables | |
| local missing_vars=() | |
| [[ -z "$parameters_file" ]] && missing_vars+=("ARGO_PARAMETERS_FILE") | |
| [[ -z "$release_name" ]] && missing_vars+=("RELEASE_NAME") | |
| [[ -z "$namespace" ]] && missing_vars+=("NAMESPACE") | |
| if [[ ${`#missing_vars`[@]} -gt 0 ]]; then | |
| echo -e "${RED}Error: The following required environment variables are missing:${NC}" | |
| printf ' - %s\n' "${missing_vars[@]}" | |
| usage | |
| exit 2 | |
| fi | |
| if [[ ! -r "$parameters_file" ]]; then | |
| echo -e "${RED}Error: ARGO_PARAMETERS_FILE '$parameters_file' not found or not readable.${NC}" | |
| exit 2 | |
| fi |
🤖 Prompt for AI Agents
In `@src/scripts/argo_hpa_transition.sh` around lines 35 - 46, The validation
currently only checks that the parameters_file variable is set; update the check
so it also fails fast when the file path is missing or not readable. Replace the
single-variable check for parameters_file with a compound test like [[ -z
"$parameters_file" || ! -r "$parameters_file" ]] and add a clear label such as
"ARGO_PARAMETERS_FILE (missing or unreadable)" to missing_vars so the script
exits immediately if the file does not exist or is not readable; keep the rest
of the missing_vars handling (usage and exit 2) unchanged.
Comment on lines
+48
to
+55
| # Check if autoscaling is being enabled in this deploy | ||
| local hpa_enabled | ||
| hpa_enabled=$(grep -A1 'name: autoscaling\.enabled' "$parameters_file" | grep 'value:' | awk '{print $2}' || echo "false") | ||
|
|
||
| if [[ "$hpa_enabled" != "true" ]]; then | ||
| echo -e "${GREEN}HPA not enabled in this deploy, skipping.${NC}" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
HPA enablement parsing is brittle for quoted/uppercase values.
If the parameters file uses value: "true" or value: True, Line 50 returns a non‑true string and the cleanup is skipped. Normalize and strip quotes before comparison.
🛠️ Suggested fix
- hpa_enabled=$(grep -A1 'name: autoscaling\.enabled' "$parameters_file" | grep 'value:' | awk '{print $2}' || echo "false")
+ hpa_enabled=$(awk '
+ $0 ~ /name: autoscaling\.enabled/ {found=1; next}
+ found && $1=="value:" {
+ gsub(/"/,"",$2);
+ print tolower($2);
+ exit
+ }
+ ' "$parameters_file")
+ hpa_enabled=${hpa_enabled:-false}📝 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.
Suggested change
| # Check if autoscaling is being enabled in this deploy | |
| local hpa_enabled | |
| hpa_enabled=$(grep -A1 'name: autoscaling\.enabled' "$parameters_file" | grep 'value:' | awk '{print $2}' || echo "false") | |
| if [[ "$hpa_enabled" != "true" ]]; then | |
| echo -e "${GREEN}HPA not enabled in this deploy, skipping.${NC}" | |
| return 0 | |
| fi | |
| # Check if autoscaling is being enabled in this deploy | |
| local hpa_enabled | |
| hpa_enabled=$(awk ' | |
| $0 ~ /name: autoscaling\.enabled/ {found=1; next} | |
| found && $1=="value:" { | |
| gsub(/"/,"",$2); | |
| print tolower($2); | |
| exit | |
| } | |
| ' "$parameters_file") | |
| hpa_enabled=${hpa_enabled:-false} | |
| if [[ "$hpa_enabled" != "true" ]]; then | |
| echo -e "${GREEN}HPA not enabled in this deploy, skipping.${NC}" | |
| return 0 | |
| fi |
🤖 Prompt for AI Agents
In `@src/scripts/argo_hpa_transition.sh` around lines 48 - 55, The hpa_enabled
extraction is brittle for quoted or capitalized values; update the hpa_enabled
assignment (the variable computed from "$parameters_file") to strip surrounding
quotes and whitespace and normalize to lowercase before comparing to "true".
Locate the hpa_enabled assignment and replace the current pipeline with one that
extracts the value token, removes surrounding quotes, trims whitespace, and
lowercases it (so values like "true", True, TRUE all normalize to true) and then
keep the existing if [[ "$hpa_enabled" != "true" ]]; check.
Contributor
|
Closed in favor of: |
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
2 participants
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.
…c When Re-enabling HPA
Why? 🤔
We need this change because…
What? 📃
Please add a summary for this pull requests
Additional Links 🔗
Summary by CodeRabbit
New Features
Bug Fixes