ACM-30906: Add AAP automation script for rhacmstackem integration#5956
Draft
Randy424 wants to merge 5 commits intostolostron:mainfrom
Draft
ACM-30906: Add AAP automation script for rhacmstackem integration#5956Randy424 wants to merge 5 commits intostolostron:mainfrom
Randy424 wants to merge 5 commits intostolostron:mainfrom
Conversation
Add install-aap.sh for automated Ansible Automation Platform provisioning on weekly clusters. Supports idempotent installation, operator deployment, and automated subscription management via Red Hat offline token. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Randy424 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new idempotent Bash installer Changes
Sequence DiagramsequenceDiagram
participant Script as Install Script
participant OCP as OpenShift API
participant OH as OperatorHub
participant AAP as AAP Controller API
participant RH as Red Hat API
participant Secrets as Secrets API
Script->>Script: validate env & CLI prerequisites
Script->>OCP: check namespace, CR, and pod state
alt already healthy
Script->>Script: exit
else install or repair
Script->>OCP: create namespace, OperatorGroup, Subscription
Script->>OH: wait for CSV readiness
Script->>OCP: apply CR (AutomationController or AnsibleAutomationPlatform)
Script->>OCP: poll CR Running condition (with pod-count fallback)
end
Script->>Secrets: get admin password secret
Script->>OCP: fetch platform route/host
alt RH_OFFLINE_TOKEN provided and route available
Script->>RH: exchange offline token → access token
Script->>RH: find/create allocation, select pool, attach subscription
Script->>RH: download entitlement manifest
Script->>AAP: upload manifest to controller subscriptions endpoint
Script->>Script: cleanup temporary manifest
end
Script->>Script: print installation summary (namespace, mode, name, URL, components)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
scripts/aap-automation/install-aap.sh (1)
62-62: Quote variable expansions in command arguments consistently.There are multiple unquoted expansions in
ocinvocations; this is fragile and flagged by ShellCheck (SC2086).Suggested fix pattern
-ROUTE_URL=$(oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE -o jsonpath='{.spec.host}' 2>/dev/null || echo "N/A") +ROUTE_URL=$(oc get route "${PLATFORM_NAME}" -n "${AAP_NAMESPACE}" -o jsonpath='{.spec.host}' 2>/dev/null || echo "N/A") -oc create namespace $AAP_NAMESPACE --dry-run=client -o yaml | oc apply -f - +oc create namespace "${AAP_NAMESPACE}" --dry-run=client -o yaml | oc apply -f -Also applies to: 86-86, 120-120, 132-132, 141-141, 177-177, 190-190, 202-202, 211-211, 216-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` at line 62, The oc command argument expansions are unquoted (e.g., ROUTE_URL assignment using oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE), which is fragile; update all oc invocations in this script to quote variable expansions (for example use "${PLATFORM_NAME}" and "${AAP_NAMESPACE}" and other variables used with oc) so arguments are passed safely and pass ShellCheck SC2086; apply the same fix to every similar occurance (the other oc get/apply commands referenced in the review) to ensure consistent quoting across the script.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/aap-automation/install-aap.sh`:
- Line 311: The final summary currently prints the admin credential via the
log_info call using ADMIN_PASSWORD (log_info "Password:
$ADMIN_PASSWORD"); remove this direct exposure by no longer outputting
ADMIN_PASSWORD to logs — either omit the password line entirely or replace it
with a non-sensitive placeholder such as "[REDACTED]" or a hint where the
password is stored (e.g., "Password: stored in $SOME_SECRET_LOCATION"). Update
the log_info invocation accordingly and ensure any automated tests or consumers
of install-aap.sh read the secret from the secure store instead of the summary
output.
- Around line 48-50: The early gateway health check uses a different label
selector than the later deployment timeout check, causing inconsistent pod sets;
define a single variable GATEWAY_SELECTOR set to the specific selector
"app.kubernetes.io/name=aap-platform,app.kubernetes.io/component=gateway" and
replace the hardcoded selectors where GATEWAY_RUNNING is computed and where the
deployment timeout check uses its selector so both use GATEWAY_SELECTOR
consistently.
- Around line 27-36: The script only validates the oc CLI but later uses curl,
jq, and base64 (and has set -e), so missing binaries cause opaque failures; add
prerequisite checks like the existing oc check using command -v for curl, jq,
and base64 and call log_error + exit 1 if any are missing (mirror the oc check
block), placing these checks near the current prerequisites block so functions
like log_error and the rest of install-aap.sh fail fast with clear messages.
- Around line 240-242: The allocation lookup currently uses
ALLOCATION_UUID=$(curl -sk -H "Authorization: Bearer ${ACCESS_TOKEN}"
"${RHSM_API}/allocations" | jq -r '.body[0].uuid // empty') which blindly picks
the first allocation; change the request and jq filter to select the allocation
whose name matches ALLOCATION_NAME (the name created earlier, e.g.,
AAP-Automation-<date>) so you reliably get the intended allocation UUID; update
the curl invocation to follow the script’s existing curl pattern (keep -s -k and
the Authorization header) and pipe to jq to search .body[] | select(.name ==
env.ALLOCATION_NAME) | .uuid // empty, assigning that result to ALLOCATION_UUID.
- Around line 230-234: Replace hardcoded -k flags in all curl calls with a
centralized, opt-in variable so TLS verification is enabled by default; create a
top-level shell variable (e.g., CURL_OPTS or CURL_INSECURE_OPTS) defaulting to
empty and allow callers to set it (e.g., export CURL_OPTS="--insecure") only
when explicitly intended, then update each curl invocation such as the token
fetch (ACCESS_TOKEN=$(curl -sk ... )) and the other seven calls to use
"${CURL_OPTS}" instead of -k; ensure the token exchange, Bearer token API calls,
and manifest upload use the centralized option and document the opt-in behavior
in a comment near the variable.
---
Nitpick comments:
In `@scripts/aap-automation/install-aap.sh`:
- Line 62: The oc command argument expansions are unquoted (e.g., ROUTE_URL
assignment using oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE), which is
fragile; update all oc invocations in this script to quote variable expansions
(for example use "${PLATFORM_NAME}" and "${AAP_NAMESPACE}" and other variables
used with oc) so arguments are passed safely and pass ShellCheck SC2086; apply
the same fix to every similar occurance (the other oc get/apply commands
referenced in the review) to ensure consistent quoting across the script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e9cbd09-8633-4978-b341-27c0194dcdbf
📒 Files selected for processing (1)
scripts/aap-automation/install-aap.sh
- Add prerequisite checks for curl, jq, base64 - Centralize curl TLS options with opt-in CURL_INSECURE flag - Unify gateway pod label selectors for consistent health checks - Filter allocation lookup by name instead of selecting first result - Remove admin password from log output - Use mktemp + trap for manifest file cleanup - Use awk for pod status matching to avoid false positives - Add AAP_MODE support for platform vs controller (legacy) deployment Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/aap-automation/install-aap.sh (1)
87-87: Quote variable expansions consistently inocinvocations.These sites still use unquoted expansions (SC2086). It’s low risk with current defaults, but quoting avoids accidental globbing/splitting if env overrides contain special characters.
Proposed fix (pattern)
-ROUTE_URL=$(oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE -o jsonpath='{.spec.host}' 2>/dev/null || echo "N/A") +ROUTE_URL=$(oc get route "${PLATFORM_NAME}" -n "$AAP_NAMESPACE" -o jsonpath='{.spec.host}' 2>/dev/null || echo "N/A") -oc create namespace $AAP_NAMESPACE --dry-run=client -o yaml | oc apply -f - +oc create namespace "$AAP_NAMESPACE" --dry-run=client -o yaml | oc apply -f - -CR_STATUS=$(oc get ansibleautomationplatform $PLATFORM_NAME -n $AAP_NAMESPACE \ +CR_STATUS=$(oc get ansibleautomationplatform "$PLATFORM_NAME" -n "$AAP_NAMESPACE" \Also applies to: 111-111, 145-145, 157-157, 166-166, 216-216, 219-219, 234-234, 236-236, 248-248, 257-257, 262-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` at line 87, The oc invocations use unquoted variable expansions (e.g., ROUTE_URL=$(oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE ...)), which can cause word-splitting/globbing; update each oc call to quote all variable expansions (for example quote ${PLATFORM_NAME}, "$AAP_NAMESPACE" and any other $VAR used with oc) so arguments are passed as single tokens and safe from special characters — search for oc get/describe/... occurrences in this script and replace unquoted expansions at the noted sites (including the ROUTE_URL assignment and the other listed occurrences) with quoted expansions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/aap-automation/install-aap.sh`:
- Around line 259-263: The ROUTE_URL check can still leave ROUTE_URL empty and
later steps (token/allocation/upload) will attempt curl to https:// and fail
under set -e; modify the logic around the ROUTE_URL check (the block that sets
ROUTE_URL and the upload/token/allocation sequence referencing ROUTE_URL) to
detect an empty ROUTE_URL and skip the subscription upload (and any curl to
https://${ROUTE_URL}) or retry with a bounded loop: if ROUTE_URL remains empty
after the existing sleep, either log a warning and skip the upload steps that
use ROUTE_URL, or implement a retry loop with a max attempts and a clear error
return; ensure all places that reference ROUTE_URL for curl/upload/token
allocation are gated by a non-empty check so the script will not abort under set
-e.
- Around line 14-20: Validate AAP_MODE immediately after it's set (the AAP_MODE
variable) and reject any value other than the allowed options ("platform" or
"controller") so unknown inputs don't silently fall back to platform; implement
a small validation block after the AAP_MODE assignment that
normalizes/case-folds the value if desired, checks if AAP_MODE is "platform" or
"controller", and if not prints a clear error message and exits (non-zero);
update any later branching that assumes platform-by-default to rely on this
validated AAP_MODE variable (and keep ENABLE_HUB / ENABLE_EDA behavior
unchanged).
---
Nitpick comments:
In `@scripts/aap-automation/install-aap.sh`:
- Line 87: The oc invocations use unquoted variable expansions (e.g.,
ROUTE_URL=$(oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE ...)), which can
cause word-splitting/globbing; update each oc call to quote all variable
expansions (for example quote ${PLATFORM_NAME}, "$AAP_NAMESPACE" and any other
$VAR used with oc) so arguments are passed as single tokens and safe from
special characters — search for oc get/describe/... occurrences in this script
and replace unquoted expansions at the noted sites (including the ROUTE_URL
assignment and the other listed occurrences) with quoted expansions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0857ec1-2058-47f5-a356-795ecbe04550
📒 Files selected for processing (1)
scripts/aap-automation/install-aap.sh
- Validate AAP_MODE rejects unknown values instead of silent fallback - Guard subscription management against empty ROUTE_URL Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/aap-automation/install-aap.sh (4)
51-55: Consider adding--failtoCURL_OPTSfor robust error detection.
CURL_OPTS=(-s)means HTTP 4xx/5xx responses still return exit code 0. Withset -e, failures in API calls (e.g., lines 315-319 attach subscription) won't halt execution. The token check on line 294 partially mitigates this, but other calls redirect output to/dev/nullwithout status validation.Suggested fix
-CURL_OPTS=(-s) +CURL_OPTS=(-s --fail)Note:
--failwill cause immediate exit on HTTP errors. Ensure error handling (e.g.,|| trueor explicit checks) is added where graceful degradation is desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` around lines 51 - 55, The script currently sets CURL_OPTS=(-s) and conditionally appends -k when CURL_INSECURE is true; change CURL_OPTS to include --fail so HTTP 4xx/5xx return non-zero (e.g., CURL_OPTS=(-s --fail)) and keep the existing -k append logic for CURL_INSECURE; then review places that rely on curl succeeding silently (for example the attach subscription call around the block referenced and any curls redirecting output to /dev/null) and add explicit handling (use || true where failure should be tolerated or check curl's exit status and handle errors) to avoid unwanted exits under set -e while preserving intended graceful degradation.
96-96: Quote variables to prevent word splitting (SC2086).Multiple lines use unquoted variables in
occommands. While these variables are unlikely to contain spaces, quoting is best practice and silences shellcheck.Affected lines: 96, 120, 154, 166, 175, 225, 228, 243, 245, 257, 266, 271.
Example fix pattern
-ROUTE_URL=$(oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE -o jsonpath='{.spec.host}' 2>/dev/null || echo "N/A") +ROUTE_URL=$(oc get route "${PLATFORM_NAME}" -n "$AAP_NAMESPACE" -o jsonpath='{.spec.host}' 2>/dev/null || echo "N/A")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` at line 96, Several oc command invocations (e.g., the ROUTE_URL assignment using ROUTE_URL=$(oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE -o jsonpath=...)) use unquoted variables which can cause word-splitting; update each oc invocation (including the occurrences that reference PLATFORM_NAME, AAP_NAMESPACE and other shell variables mentioned) to quote variable expansions (e.g., "${PLATFORM_NAME}", "${AAP_NAMESPACE}", etc.) so arguments are passed as single tokens and shellcheck SC2086 is satisfied; search for other usages in this script that match the same pattern (the lines the reviewer flagged) and apply the same fix to the corresponding assignments and oc commands (functions/assignments that set ROUTE_URL and any other variables used in oc get/oc apply calls).
193-214: YAML construction via string concatenation is functional but fragile.Indentation must be exact for the
hub:andeda:sections. Current implementation works, but any refactoring risks breaking YAML structure silently.Alternative: use
yqto merge sections, or generate complete variants upfront. Not blocking—just a maintenance consideration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` around lines 193 - 214, The PLATFORM_SPEC string is being built via fragile concatenation which can easily break YAML indentation when toggling ENABLE_HUB or ENABLE_EDA; instead construct the full manifest programmatically and merge only the optional sections: build a base manifest string for the AnsibleAutomationPlatform in install-aap.sh, and when ENABLE_HUB or ENABLE_EDA are true append or merge properly-formed YAML blocks (or use yq to add the hub/eda keys) rather than naive string concat—target the PLATFORM_SPEC variable and the conditionals that reference ENABLE_HUB and ENABLE_EDA, or replace them with yq commands to merge optional sections into the base manifest before piping PLATFORM_SPEC to oc apply.
310-312: Pool selection filter may be overly broad.
contains("Ansible")could match multiple subscription types (e.g., "Ansible Automation Platform", "Ansible Tower", "Red Hat Ansible..."). If multiple pools exist, this selects the first arbitrarily.Consider a more specific filter or log which pool was selected for debugging.
Example: more specific filter
- jq -r '[.body[] | select(.product_name | contains("Ansible"))][0].pool_id // empty') + jq -r '[.body[] | select(.product_name | test("Ansible Automation Platform"))][0].pool_id // empty')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` around lines 310 - 312, The current jq filter uses contains("Ansible") which is too broad and may pick an arbitrary pool; change the jq selection used when computing POOL_ID to a more specific predicate (e.g., match product_name == "Ansible Automation Platform" or use startswith/regex for the exact product string/version) instead of contains("Ansible"), and add explicit logging of the selected pool's id and product_name (use the same RHSM_API/ALLOCATION_UUID/CURL_OPTS/ACCESS_TOKEN context and the POOL_ID variable) so reviewers can see which pool was chosen when multiple matches exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/aap-automation/install-aap.sh`:
- Around line 315-319: The curl POST that uses CURL_OPTS with ACCESS_TOKEN,
POOL_ID, RHSM_API and ALLOCATION_UUID currently redirects output to /dev/null
and does not check the HTTP result; capture the response and HTTP status instead
of discarding it, e.g. save curl output to a variable or temp file and use curl
-w '%{http_code}' (or ensure --fail is present in CURL_OPTS), then validate the
HTTP code after the request in the script (in the block that performs the
subscription allocation) and log an error with the response body and exit
non‑zero when the status indicates failure so the subsequent "Subscription
attached" message is only printed on success.
---
Nitpick comments:
In `@scripts/aap-automation/install-aap.sh`:
- Around line 51-55: The script currently sets CURL_OPTS=(-s) and conditionally
appends -k when CURL_INSECURE is true; change CURL_OPTS to include --fail so
HTTP 4xx/5xx return non-zero (e.g., CURL_OPTS=(-s --fail)) and keep the existing
-k append logic for CURL_INSECURE; then review places that rely on curl
succeeding silently (for example the attach subscription call around the block
referenced and any curls redirecting output to /dev/null) and add explicit
handling (use || true where failure should be tolerated or check curl's exit
status and handle errors) to avoid unwanted exits under set -e while preserving
intended graceful degradation.
- Line 96: Several oc command invocations (e.g., the ROUTE_URL assignment using
ROUTE_URL=$(oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE -o jsonpath=...))
use unquoted variables which can cause word-splitting; update each oc invocation
(including the occurrences that reference PLATFORM_NAME, AAP_NAMESPACE and other
shell variables mentioned) to quote variable expansions (e.g.,
"${PLATFORM_NAME}", "${AAP_NAMESPACE}", etc.) so arguments are passed as single
tokens and shellcheck SC2086 is satisfied; search for other usages in this
script that match the same pattern (the lines the reviewer flagged) and apply
the same fix to the corresponding assignments and oc commands
(functions/assignments that set ROUTE_URL and any other variables used in oc
get/oc apply calls).
- Around line 193-214: The PLATFORM_SPEC string is being built via fragile
concatenation which can easily break YAML indentation when toggling ENABLE_HUB
or ENABLE_EDA; instead construct the full manifest programmatically and merge
only the optional sections: build a base manifest string for the
AnsibleAutomationPlatform in install-aap.sh, and when ENABLE_HUB or ENABLE_EDA
are true append or merge properly-formed YAML blocks (or use yq to add the
hub/eda keys) rather than naive string concat—target the PLATFORM_SPEC variable
and the conditionals that reference ENABLE_HUB and ENABLE_EDA, or replace them
with yq commands to merge optional sections into the base manifest before piping
PLATFORM_SPEC to oc apply.
- Around line 310-312: The current jq filter uses contains("Ansible") which is
too broad and may pick an arbitrary pool; change the jq selection used when
computing POOL_ID to a more specific predicate (e.g., match product_name ==
"Ansible Automation Platform" or use startswith/regex for the exact product
string/version) instead of contains("Ansible"), and add explicit logging of the
selected pool's id and product_name (use the same
RHSM_API/ALLOCATION_UUID/CURL_OPTS/ACCESS_TOKEN context and the POOL_ID
variable) so reviewers can see which pool was chosen when multiple matches
exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79a4d9b0-3357-46d5-80d6-ae1654b7165b
📒 Files selected for processing (1)
scripts/aap-automation/install-aap.sh
Comment on lines
+315
to
+319
| curl "${CURL_OPTS[@]}" -X POST \ | ||
| -H "Authorization: Bearer ${ACCESS_TOKEN}" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "{\"pool_id\":\"${POOL_ID}\",\"quantity\":1}" \ | ||
| "${RHSM_API}/allocations/${ALLOCATION_UUID}/subscriptions" > /dev/null |
There was a problem hiding this comment.
Subscription attach silently discards response.
Output redirected to /dev/null without status check. If this call fails (HTTP 4xx/5xx), and --fail isn't in CURL_OPTS, the script continues silently. The "Subscription attached" log on line 320 may be misleading.
Suggested fix
- curl "${CURL_OPTS[@]}" -X POST \
+ ATTACH_RESPONSE=$(curl "${CURL_OPTS[@]}" -X POST \
-H "Authorization: Bearer ${ACCESS_TOKEN}" \
-H "Content-Type: application/json" \
-d "{\"pool_id\":\"${POOL_ID}\",\"quantity\":1}" \
- "${RHSM_API}/allocations/${ALLOCATION_UUID}/subscriptions" > /dev/null
- log_info "Subscription attached"
+ "${RHSM_API}/allocations/${ALLOCATION_UUID}/subscriptions" \
+ -w "\nHTTP_CODE:%{http_code}")
+ HTTP_CODE=$(echo "$ATTACH_RESPONSE" | grep "HTTP_CODE:" | cut -d: -f2)
+ if [ "$HTTP_CODE" = "200" ] || [ "$HTTP_CODE" = "201" ]; then
+ log_info "Subscription attached"
+ else
+ log_warn "Subscription attach returned HTTP $HTTP_CODE"
+ 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
| curl "${CURL_OPTS[@]}" -X POST \ | |
| -H "Authorization: Bearer ${ACCESS_TOKEN}" \ | |
| -H "Content-Type: application/json" \ | |
| -d "{\"pool_id\":\"${POOL_ID}\",\"quantity\":1}" \ | |
| "${RHSM_API}/allocations/${ALLOCATION_UUID}/subscriptions" > /dev/null | |
| ATTACH_RESPONSE=$(curl "${CURL_OPTS[@]}" -X POST \ | |
| -H "Authorization: Bearer ${ACCESS_TOKEN}" \ | |
| -H "Content-Type: application/json" \ | |
| -d "{\"pool_id\":\"${POOL_ID}\",\"quantity\":1}" \ | |
| "${RHSM_API}/allocations/${ALLOCATION_UUID}/subscriptions" \ | |
| -w "\nHTTP_CODE:%{http_code}") | |
| HTTP_CODE=$(echo "$ATTACH_RESPONSE" | grep "HTTP_CODE:" | cut -d: -f2) | |
| if [ "$HTTP_CODE" = "200" ] || [ "$HTTP_CODE" = "201" ]; then | |
| log_info "Subscription attached" | |
| else | |
| log_warn "Subscription attach returned HTTP $HTTP_CODE" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/aap-automation/install-aap.sh` around lines 315 - 319, The curl POST
that uses CURL_OPTS with ACCESS_TOKEN, POOL_ID, RHSM_API and ALLOCATION_UUID
currently redirects output to /dev/null and does not check the HTTP result;
capture the response and HTTP status instead of discarding it, e.g. save curl
output to a variable or temp file and use curl -w '%{http_code}' (or ensure
--fail is present in CURL_OPTS), then validate the HTTP code after the request
in the script (in the block that performs the subscription allocation) and log
an error with the response body and exit non‑zero when the status indicates
failure so the subsequent "Subscription attached" message is only printed on
success.
Check HTTP status code when attaching subscription to allocation instead of discarding the response. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/aap-automation/install-aap.sh (2)
120-120: Optional: Quote shell variables for defensive scripting.Multiple unquoted variable expansions (SC2086) throughout the script, e.g.,
$AAP_NAMESPACE,$PLATFORM_NAME. While safe here since values are controlled, quoting prevents edge-case breakage if values ever contain spaces or special characters.Example fix pattern
-oc create namespace $AAP_NAMESPACE --dry-run=client -o yaml | oc apply -f - +oc create namespace "$AAP_NAMESPACE" --dry-run=client -o yaml | oc apply -f -Apply similarly to lines 96, 154, 166, 175, 225, 228, 243, 245, 257, 266, 271.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` at line 120, The command invoking oc (oc create namespace $AAP_NAMESPACE --dry-run=client -o yaml | oc apply -f -) and other usages must use quoted variable expansions to prevent word-splitting and globbing; update every instance of unquoted variables like $AAP_NAMESPACE, $PLATFORM_NAME, and the other occurrences called out (lines referenced in the review) to use double quotes (e.g., "$AAP_NAMESPACE") consistently across the script (install-aap.sh) including the examples at lines 96, 154, 166, 175, 225, 228, 243, 245, 257, 266, 271 so all shell variable expansions are safely quoted.
193-214: Consider heredoc with conditional logic for YAML construction.Building
PLATFORM_SPECvia string concatenation is indentation-sensitive and error-prone during maintenance. If the base YAML changes, the appended sections may misalign.Alternative approach using yq
If
yqis available, you could build the base spec and conditionally merge sections:PLATFORM_SPEC=$(cat <<EOF apiVersion: aap.ansible.com/v1alpha1 kind: AnsibleAutomationPlatform metadata: name: $PLATFORM_NAME namespace: $AAP_NAMESPACE spec: controller: replicas: 1 EOF ) if [ "$ENABLE_HUB" = "true" ]; then PLATFORM_SPEC=$(echo "$PLATFORM_SPEC" | yq '.spec.hub.replicas = 1') fiAlternatively, keep current approach but add a lint step in CI to validate YAML syntax.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` around lines 193 - 214, The PLATFORM_SPEC is being built via fragile string concatenation which breaks YAML indentation; replace this with a heredoc-based base YAML assigned to PLATFORM_SPEC (using a single cat <<EOF block) and then conditionally append the hub/eda blocks to that heredoc string (or use yq to set .spec.hub.replicas and .spec.eda.replicas when ENABLE_HUB/ENABLE_EDA are true) before piping to oc apply -f -; update references to PLATFORM_SPEC, ENABLE_HUB, ENABLE_EDA and the oc apply invocation accordingly so indentation is preserved and the final YAML is validated (or add a YAML lint step in CI).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/aap-automation/install-aap.sh`:
- Around line 337-339: The curl download of
"${RHSM_API}/allocations/${ALLOCATION_UUID}/export" currently writes to
"${MANIFEST_FILE}" without verifying HTTP status; change the curl invocation
(the command using CURL_OPTS and -o "${MANIFEST_FILE}") to either use --fail -S
(or -fS) so curl returns non-zero on 4xx/5xx, or use -w '%{http_code}' to
capture the HTTP status into a variable, then check curl's exit code or the
captured status before treating the file as valid; if the request fails remove
the temp/partial "${MANIFEST_FILE}" and exit with an error.
---
Nitpick comments:
In `@scripts/aap-automation/install-aap.sh`:
- Line 120: The command invoking oc (oc create namespace $AAP_NAMESPACE
--dry-run=client -o yaml | oc apply -f -) and other usages must use quoted
variable expansions to prevent word-splitting and globbing; update every
instance of unquoted variables like $AAP_NAMESPACE, $PLATFORM_NAME, and the
other occurrences called out (lines referenced in the review) to use double
quotes (e.g., "$AAP_NAMESPACE") consistently across the script (install-aap.sh)
including the examples at lines 96, 154, 166, 175, 225, 228, 243, 245, 257, 266,
271 so all shell variable expansions are safely quoted.
- Around line 193-214: The PLATFORM_SPEC is being built via fragile string
concatenation which breaks YAML indentation; replace this with a heredoc-based
base YAML assigned to PLATFORM_SPEC (using a single cat <<EOF block) and then
conditionally append the hub/eda blocks to that heredoc string (or use yq to set
.spec.hub.replicas and .spec.eda.replicas when ENABLE_HUB/ENABLE_EDA are true)
before piping to oc apply -f -; update references to PLATFORM_SPEC, ENABLE_HUB,
ENABLE_EDA and the oc apply invocation accordingly so indentation is preserved
and the final YAML is validated (or add a YAML lint step in CI).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eeca24e4-b7ec-462b-921b-aef473989c84
📒 Files selected for processing (1)
scripts/aap-automation/install-aap.sh
Comment on lines
+337
to
+339
| curl "${CURL_OPTS[@]}" -H "Authorization: Bearer ${ACCESS_TOKEN}" \ | ||
| "${RHSM_API}/allocations/${ALLOCATION_UUID}/export" \ | ||
| -o "${MANIFEST_FILE}" |
There was a problem hiding this comment.
Manifest download doesn't verify HTTP status code.
If the export endpoint returns an error (4xx/5xx), curl may still create a non-empty file containing an error response. The subsequent size check on line 341 would pass, and the upload would fail with a confusing error.
Suggested fix
if [ -n "$ALLOCATION_UUID" ]; then
log_info "Downloading subscription manifest..."
- curl "${CURL_OPTS[@]}" -H "Authorization: Bearer ${ACCESS_TOKEN}" \
+ DOWNLOAD_RESPONSE=$(curl "${CURL_OPTS[@]}" -H "Authorization: Bearer ${ACCESS_TOKEN}" \
"${RHSM_API}/allocations/${ALLOCATION_UUID}/export" \
- -o "${MANIFEST_FILE}"
+ -o "${MANIFEST_FILE}" \
+ -w "%{http_code}")
- if [ -f "$MANIFEST_FILE" ] && [ -s "$MANIFEST_FILE" ]; then
+ if [ "$DOWNLOAD_RESPONSE" = "200" ] && [ -f "$MANIFEST_FILE" ] && [ -s "$MANIFEST_FILE" ]; then
log_info "Uploading manifest to AAP..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/aap-automation/install-aap.sh` around lines 337 - 339, The curl
download of "${RHSM_API}/allocations/${ALLOCATION_UUID}/export" currently writes
to "${MANIFEST_FILE}" without verifying HTTP status; change the curl invocation
(the command using CURL_OPTS and -o "${MANIFEST_FILE}") to either use --fail -S
(or -fS) so curl returns non-zero on 4xx/5xx, or use -w '%{http_code}' to
capture the HTTP status into a variable, then check curl's exit code or the
captured status before treating the file as valid; if the request fails remove
the temp/partial "${MANIFEST_FILE}" and exit with an error.
Check HTTP status on manifest download and clean up partial file on failure instead of proceeding with an invalid manifest. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/aap-automation/install-aap.sh (2)
94-98: Quote variable expansions inoccommands to prevent word splitting.Multiple
occommands use unquoted variables ($AAP_NAMESPACE,$PLATFORM_NAME). While these are unlikely to contain spaces, quoting is defensive practice and silences shellcheck SC2086.This applies throughout the script (lines 96, 120, 154, 166, 175, 225, 228, 243, 245, 257, 266, 271).
Example fix for line 96
- ROUTE_URL=$(oc get route ${PLATFORM_NAME} -n $AAP_NAMESPACE -o jsonpath='{.spec.host}' 2>/dev/null || echo "N/A") + ROUTE_URL=$(oc get route "${PLATFORM_NAME}" -n "$AAP_NAMESPACE" -o jsonpath='{.spec.host}' 2>/dev/null || echo "N/A")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` around lines 94 - 98, Several oc command invocations (e.g., the ROUTE_URL assignment using oc get route and other oc calls referenced in the comment) use unquoted variable expansions like $AAP_NAMESPACE and $PLATFORM_NAME; update all such oc invocations (including the ROUTE_URL assignment and the other occurrences noted) to quote variable expansions (e.g., "$AAP_NAMESPACE", "$PLATFORM_NAME") to prevent word splitting and satisfy shellcheck SC2086; search for all uses of oc get/get route/oc commands in this script (including lines around ROUTE_URL, and the other occurrences referenced) and replace unquoted variables with quoted ones while preserving existing command syntax and redirections.
149-151: Consider definingINTERVALcloser to where it's reused, or rename for clarity.
INTERVAL=10is defined here but implicitly reused in the CR wait loop starting at line 223. If someone changes this value for the operator wait, it silently affects the CR wait too. Consider either:
- Defining separate
OPERATOR_INTERVALandCR_INTERVALvariables, or- Redefining
INTERVALin each wait section to make the dependency explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-automation/install-aap.sh` around lines 149 - 151, The script currently defines a single INTERVAL=10 used by multiple wait loops (notably the operator wait and the CR wait loop that references INTERVAL), which creates an implicit coupling; split this into two explicit variables (e.g., OPERATOR_INTERVAL and CR_INTERVAL) or redeclare INTERVAL right before each wait loop to localize the dependency: update the global INTERVAL assignment, add OPERATOR_INTERVAL and CR_INTERVAL (or reassign INTERVAL) and replace references in the operator wait and the CR wait loop to use the new names (or the newly scoped INTERVAL) so changes to one loop won’t silently affect the other.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/aap-automation/install-aap.sh`:
- Around line 94-98: Several oc command invocations (e.g., the ROUTE_URL
assignment using oc get route and other oc calls referenced in the comment) use
unquoted variable expansions like $AAP_NAMESPACE and $PLATFORM_NAME; update all
such oc invocations (including the ROUTE_URL assignment and the other
occurrences noted) to quote variable expansions (e.g., "$AAP_NAMESPACE",
"$PLATFORM_NAME") to prevent word splitting and satisfy shellcheck SC2086;
search for all uses of oc get/get route/oc commands in this script (including
lines around ROUTE_URL, and the other occurrences referenced) and replace
unquoted variables with quoted ones while preserving existing command syntax and
redirections.
- Around line 149-151: The script currently defines a single INTERVAL=10 used by
multiple wait loops (notably the operator wait and the CR wait loop that
references INTERVAL), which creates an implicit coupling; split this into two
explicit variables (e.g., OPERATOR_INTERVAL and CR_INTERVAL) or redeclare
INTERVAL right before each wait loop to localize the dependency: update the
global INTERVAL assignment, add OPERATOR_INTERVAL and CR_INTERVAL (or reassign
INTERVAL) and replace references in the operator wait and the CR wait loop to
use the new names (or the newly scoped INTERVAL) so changes to one loop won’t
silently affect the other.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3e8143d-ee97-4214-a27d-7780b98bf73b
📒 Files selected for processing (1)
scripts/aap-automation/install-aap.sh
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.
Add install-aap.sh for automated Ansible Automation Platform provisioning on weekly clusters. Supports idempotent installation, operator deployment, and automated subscription management via Red Hat offline token.
Will run some live testing, make necessary adjustments then convert this to an active PR.
📝 Summary
Ticket Summary (Title):
Ticket Link:
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit