refactor: clean up and simplify pipeline scripts - #611
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 23, 2026
Merged
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
Contributor
|
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:
📝 WalkthroughWalkthroughConsolidates pipeline helper logic into ChangesPipeline architecture and shared library consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
/test on-demand-e2e |
typeid
marked this pull request as ready for review
June 13, 2026 10:00
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/buildspec/provision-infra-mc.sh (1)
49-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate
rhobs_api_urlwith the same RC-output readiness loop.
TF_VAR_rhobs_api_urlis read once before the wait loop and never checked again, while the OIDC outputs are retried for up to 45 minutes. In parallel RC/MC runs this lets Terraform start with an empty RHOBS endpoint even though the rest of the script already knows RC outputs may not exist yet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/buildspec/provision-infra-mc.sh` around lines 49 - 80, TF_VAR_rhobs_api_url is read once before the OIDC readiness loop and not retried; move the terraform output invocation for TF_VAR_rhobs_api_url into the same retry loop (use the same _OIDC_MAX_RETRIES/_OIDC_RETRY_DELAY/_OIDC_RETRY_COUNT mechanism), set TF_VAR_rhobs_api_url via: TF_VAR_rhobs_api_url=$(cd "$_RC_TF_DIR" && terraform output -raw rhobs_api_url 2>/dev/null || true), include TF_VAR_rhobs_api_url in the loop's readiness check alongside TF_VAR_oidc_cloudfront_domain/TF_VAR_oidc_bucket_name/TF_VAR_oidc_bucket_arn/TF_VAR_oidc_bucket_region so the break only occurs when rhobs_api_url is non-empty, update the final failure conditional to also test TF_VAR_rhobs_api_url and export TF_VAR_rhobs_api_url with the other TF_VAR_* exports.
🧹 Nitpick comments (1)
scripts/provision-pipelines.sh (1)
76-83: 💤 Low valueRedundant validation checks with identical error messages.
Lines 76-78 check for path traversal patterns (
/,..) and whitespace, while lines 80-82 enforce an alphanumeric-only whitelist. Both violations produce the same error message. The whitelist (line 80) alone is sufficient and would catch all cases flagged by line 76.If both checks are intentional defense-in-depth, consider giving them distinct error messages so users understand which constraint failed.
♻️ Proposed simplification
Remove the redundant first check:
-if [[ "$ENVIRONMENT" == *"/"* || "$ENVIRONMENT" == *".."* || "$ENVIRONMENT" =~ [[:space:]] ]]; then - echo "ERROR: ENVIRONMENT contains invalid characters: $ENVIRONMENT" >&2 - exit 1 -fi if [[ ! "$ENVIRONMENT" =~ ^[A-Za-z0-9._-]+$ ]]; then echo "ERROR: ENVIRONMENT contains invalid characters: $ENVIRONMENT" >&2 exit 1 fiAlternatively, retain both checks but clarify the error messages:
if [[ "$ENVIRONMENT" == *"/"* || "$ENVIRONMENT" == *".."* || "$ENVIRONMENT" =~ [[:space:]] ]]; then - echo "ERROR: ENVIRONMENT contains invalid characters: $ENVIRONMENT" >&2 + echo "ERROR: ENVIRONMENT contains path traversal or whitespace characters: $ENVIRONMENT" >&2 exit 1 fi if [[ ! "$ENVIRONMENT" =~ ^[A-Za-z0-9._-]+$ ]]; then - echo "ERROR: ENVIRONMENT contains invalid characters: $ENVIRONMENT" >&2 + echo "ERROR: ENVIRONMENT must contain only alphanumeric, period, underscore, or hyphen: $ENVIRONMENT" >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/provision-pipelines.sh` around lines 76 - 83, The two consecutive checks on ENVIRONMENT are redundant: the second regex guard ([[ ! "$ENVIRONMENT" =~ ^[A-Za-z0-9._-]+$ ]]) already disallows slashes, ".." and whitespace, so remove the first conditional (the block that tests for "/" or ".." or whitespace) to simplify; if you prefer defense‑in‑depth instead, keep both but change their echo messages so they differ (e.g., first: "ERROR: ENVIRONMENT contains path traversal or whitespace characters: $ENVIRONMENT" and second: "ERROR: ENVIRONMENT contains invalid characters; allowed: A-Za-z0-9._-: $ENVIRONMENT") and ensure both exit 1 remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/environment-provisioning.md`:
- Around line 149-155: Run Prettier to format the updated markdown files and
commit the changes: execute npx prettier --write '**/*.md' locally, then
add/commit the resulting formatting fixes before merging. Apply this to the
anchor docs/environment-provisioning.md (lines 149-155) and the sibling files
docs/sop/break-glass/README.md (lines 8-17),
docs/sop/break-glass/argocd-status.md (lines 12-23), and
terraform/modules/bastion/README.md (lines 49-87); no content changes beyond
Prettier formatting should be made.
In `@scripts/bootstrap-argocd.sh`:
- Around line 13-15: The script currently only checks that CLUSTER_TYPE is
non-empty but then interpolates it into paths (terraform/config/${CLUSTER_TYPE}
and the ArgoCD repo path), which allows arbitrary values; add an explicit
allowlist validation right after the existing empty-check to permit only the
expected values (e.g. "management-cluster" and "regional-cluster") and exit with
a clear error if CLUSTER_TYPE is not one of those; update any uses
(terraform/config/${CLUSTER_TYPE} and ArgoCD repo path usage) only after this
validation so the variable cannot be used for path injection.
- Around line 171-173: The task-level stop reason is being read from the wrong
JSON key; change the jq extraction that sets STOP_REASON (which currently reads
from TASK_DETAILS with '.tasks[0].stopReason') to use the ECS field
'.tasks[0].stoppedReason' instead so STOP_REASON reflects the real task stopped
reason when logging the bootstrap failure.
- Around line 46-48: The exports for
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY/AWS_SESSION_TOKEN are currently done
inline with command substitutions which can mask jq failures; instead extract
each credential into a local variable (e.g. access_key, secret_key,
session_token) using jq -er (or jq -r and explicit test), validate that none are
empty or that jq succeeded, and only then export them; if any extraction fails
or yields empty values, print an error and exit non‑zero so the script aborts
rather than continuing with partial/ambient credentials. Target the export lines
that set AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_SESSION_TOKEN and
replace the inline exports with the guarded-extraction + validation + export
flow.
In `@scripts/buildspec/register.sh`:
- Around line 131-145: The script currently treats HTTP 502 as a successful
registration; remove 502 from the unconditional success check and instead handle
502 by inspecting the response body for a specific "already exists" duplicate
indicator. Update the if that checks "$HTTP_CODE" (the block that sets
REG_OK=true) so it only accepts 201 or 409 directly, and add an elif branch for
502 that inspects /tmp/register-response.json (or the variable holding the
response) for a known duplicate string or code (e.g., grep -q "already exists"
or check JSON field with jq) and only set REG_OK=true and break when that
duplicate condition is found; otherwise treat 502 as a retry/failure as before.
Ensure the rest of the error reporting (echo/exit and cat
/tmp/register-response.json) remains unchanged.
- Around line 46-52: The script currently exits immediately if API_GATEWAY_URL
or CLOUDFRONT_DOMAIN are empty; change this to a readiness loop that retries
reading both terraform outputs until they are non-empty (or a timeout is
reached). Implement a loop in register.sh that calls the two commands
(API_GATEWAY_URL=$(cd terraform/config/regional-cluster && terraform output -raw
api_gateway_invoke_url) and CLOUDFRONT_DOMAIN=$(cd
terraform/config/regional-cluster && terraform output -raw
oidc_cloudfront_domain)) on each iteration, sleeps between attempts, and breaks
when both variables are set; if the loop times out, emit an error and exit
non-zero. Ensure the retry count/sleep interval match the other RC-dependent
scripts in the repo.
In `@scripts/pipeline-common/lib.sh`:
- Around line 200-219: When mode == "management", ensure CLUSTER_ID never
overwrites the canonical MANAGEMENT_ID: after reading CLUSTER_ID via jq
('.management_id'), add a guard that if CLUSTER_ID is empty or null then set
CLUSTER_ID="$MANAGEMENT_ID" (i.e., fallback to the existing MANAGEMENT_ID),
leaving REGIONAL_AWS_ACCOUNT_ID logic unchanged, and then export both CLUSTER_ID
and MANAGEMENT_ID so downstream code uses a single source of truth.
---
Outside diff comments:
In `@scripts/buildspec/provision-infra-mc.sh`:
- Around line 49-80: TF_VAR_rhobs_api_url is read once before the OIDC readiness
loop and not retried; move the terraform output invocation for
TF_VAR_rhobs_api_url into the same retry loop (use the same
_OIDC_MAX_RETRIES/_OIDC_RETRY_DELAY/_OIDC_RETRY_COUNT mechanism), set
TF_VAR_rhobs_api_url via: TF_VAR_rhobs_api_url=$(cd "$_RC_TF_DIR" && terraform
output -raw rhobs_api_url 2>/dev/null || true), include TF_VAR_rhobs_api_url in
the loop's readiness check alongside
TF_VAR_oidc_cloudfront_domain/TF_VAR_oidc_bucket_name/TF_VAR_oidc_bucket_arn/TF_VAR_oidc_bucket_region
so the break only occurs when rhobs_api_url is non-empty, update the final
failure conditional to also test TF_VAR_rhobs_api_url and export
TF_VAR_rhobs_api_url with the other TF_VAR_* exports.
---
Nitpick comments:
In `@scripts/provision-pipelines.sh`:
- Around line 76-83: The two consecutive checks on ENVIRONMENT are redundant:
the second regex guard ([[ ! "$ENVIRONMENT" =~ ^[A-Za-z0-9._-]+$ ]]) already
disallows slashes, ".." and whitespace, so remove the first conditional (the
block that tests for "/" or ".." or whitespace) to simplify; if you prefer
defense‑in‑depth instead, keep both but change their echo messages so they
differ (e.g., first: "ERROR: ENVIRONMENT contains path traversal or whitespace
characters: $ENVIRONMENT" and second: "ERROR: ENVIRONMENT contains invalid
characters; allowed: A-Za-z0-9._-: $ENVIRONMENT") and ensure both exit 1 remain.
🪄 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: Enterprise
Run ID: df4ecc83-203e-49ce-822d-fa5da0724b8b
📒 Files selected for processing (33)
docs/environment-provisioning.mddocs/sop/break-glass/README.mddocs/sop/break-glass/argocd-status.mdscripts/bootstrap-argocd.shscripts/buildspec/bootstrap-argocd-mc.shscripts/buildspec/bootstrap-argocd-rc.shscripts/buildspec/iot-mint.shscripts/buildspec/provision-infra-mc.shscripts/buildspec/provision-infra-rc.shscripts/buildspec/register.shscripts/dev/bastion-connect.shscripts/dev/bastion-port-forward.shscripts/dev/bastion-tasks-cleanup.shscripts/dev/check-api-gateway.shscripts/dev/customer-bastion.shscripts/dev/init-remote-backend.shscripts/dev/validate-argocd-config.shscripts/pipeline-common/account-helpers.shscripts/pipeline-common/assume-target-role.shscripts/pipeline-common/bootstrap-argocd-wrapper.shscripts/pipeline-common/detect-central-state.shscripts/pipeline-common/init-terraform-backend.shscripts/pipeline-common/lib.shscripts/pipeline-common/load-deploy-config.shscripts/pipeline-common/setup-apply-preflight.shscripts/pipeline-common/terraform-import.shscripts/pipeline-common/terraform-install.shscripts/provision-pipelines.shscripts/read-iot-state.shterraform/config/pipeline-management-cluster/buildspec-bootstrap-argocd.ymlterraform/config/pipeline-regional-cluster/buildspec-bootstrap-argocd.ymlterraform/config/regional-cluster/imports.shterraform/modules/bastion/README.md
💤 Files with no reviewable changes (17)
- scripts/read-iot-state.sh
- scripts/pipeline-common/terraform-install.sh
- scripts/pipeline-common/terraform-import.sh
- scripts/dev/bastion-tasks-cleanup.sh
- scripts/pipeline-common/bootstrap-argocd-wrapper.sh
- scripts/pipeline-common/assume-target-role.sh
- scripts/dev/bastion-port-forward.sh
- scripts/pipeline-common/detect-central-state.sh
- scripts/pipeline-common/account-helpers.sh
- scripts/dev/customer-bastion.sh
- scripts/dev/validate-argocd-config.sh
- scripts/pipeline-common/setup-apply-preflight.sh
- scripts/dev/bastion-connect.sh
- scripts/pipeline-common/load-deploy-config.sh
- scripts/pipeline-common/init-terraform-backend.sh
- scripts/dev/check-api-gateway.sh
- scripts/dev/init-remote-backend.sh
typeid
force-pushed
the
cbusse/script-cleanup
branch
2 times, most recently
from
June 13, 2026 10:36
556a383 to
0a2a36f
Compare
Collaborator
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/bootstrap-argocd.sh`:
- Around line 46-52: Remove the top-level "local _ak _sk _st" declaration (the
'local' keyword is only valid inside functions) and leave the subsequent
extraction/assignment and export lines as-is so _ak, _sk, and _st are set and
exported correctly; locate the credential-extraction block that references CREDS
and the variables _ak, _sk, _st and delete only the "local _ak _sk _st" line.
🪄 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: Enterprise
Run ID: d014caa3-72e2-47f6-8030-2c5104b89e5c
📒 Files selected for processing (5)
scripts/bootstrap-argocd.shscripts/buildspec/provision-infra-mc.shscripts/buildspec/register.shscripts/pipeline-common/lib.shscripts/provision-pipelines.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/pipeline-common/lib.sh
- scripts/buildspec/provision-infra-mc.sh
typeid
force-pushed
the
cbusse/script-cleanup
branch
from
June 13, 2026 10:42
0a2a36f to
bffe12d
Compare
Collaborator
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/buildspec/provision-infra-mc.sh (1)
23-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
TF_VAR_rhobs_api_urlplaceholder in destroy path.The provision path (line 84) exports
TF_VAR_rhobs_api_url, but the destroy path has no corresponding placeholder. If the management-cluster terraform module requires this variable,terraform destroywill fail during the planning phase with a missing variable error.🐛 Proposed fix
# Provide placeholders so terraform destroy can pass the planning phase. export TF_VAR_maestro_agent_cert_file=$(mktemp) export TF_VAR_maestro_agent_config_file=$(mktemp) export TF_VAR_oidc_cloudfront_domain="placeholder" export TF_VAR_oidc_bucket_name="placeholder" export TF_VAR_oidc_bucket_arn="arn:aws:s3:::placeholder" export TF_VAR_oidc_bucket_region="us-east-1" + export TF_VAR_rhobs_api_url="placeholder"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/buildspec/provision-infra-mc.sh` around lines 23 - 30, The destroy-path conditional that runs when DELETE_FLAG=="true" is missing a placeholder for TF_VAR_rhobs_api_url, causing terraform destroy planning to fail; update the DELETE_FLAG block (the exports created in that if) to also export TF_VAR_rhobs_api_url with a harmless placeholder value (e.g. export TF_VAR_rhobs_api_url="placeholder" or export TF_VAR_rhobs_api_url=$(mktemp)) so the management-cluster terraform module has the variable during planning.scripts/bootstrap-argocd.sh (2)
99-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
CLUSTER_TYPEenvironment variable in container overrides.
CLUSTER_TYPEis set twice (lines 99 and 106). The second definition shadows the first. While functionally harmless, this appears to be a copy-paste artifact.Proposed fix
{\"name\": \"CLUSTER_NAME\", \"value\": \"$CLUSTER_NAME\"}, {\"name\": \"CLUSTER_TYPE\", \"value\": \"$CLUSTER_TYPE\"}, {\"name\": \"REPOSITORY_URL\", \"value\": \"$REPOSITORY_URL\"}, {\"name\": \"REPOSITORY_PATH\", \"value\": \"$APPLICATIONSET_PATH\"}, {\"name\": \"REPOSITORY_BRANCH\", \"value\": \"$REPOSITORY_BRANCH\"}, {\"name\": \"ENVIRONMENT\", \"value\": \"$ENVIRONMENT\"}, {\"name\": \"AWS_REGION\", \"value\": \"$AWS_REGION\"}, {\"name\": \"REGION_DEPLOYMENT\", \"value\": \"$REGION_DEPLOYMENT\"}, - {\"name\": \"CLUSTER_TYPE\", \"value\": \"$CLUSTER_TYPE\"}, {\"name\": \"API_TARGET_GROUP_ARN\", \"value\": \"$API_TARGET_GROUP_ARN\"},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bootstrap-argocd.sh` around lines 99 - 106, Remove the duplicate CLUSTER_TYPE entry in the container overrides environment array so CLUSTER_TYPE is only defined once; locate the JSON/object array that contains entries like {"name": "CLUSTER_TYPE", "value": "$CLUSTER_TYPE"} and delete the redundant occurrence (the second CLUSTER_TYPE line) to avoid the copy-paste duplicate.
122-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
greppattern uses\swhich requires-Eor-Pflag — may cause false failure detection.The pattern
'"failures":\s*\[\]'uses\sfor whitespace, but basicgrep(without-Eor-P) treats\sas literal characters. AWS CLI typically outputs"failures": []with a space, which won't match, causing successful task starts to be incorrectly treated as failures.Proposed fix — use jq for robust JSON parsing
-if [[ $RUN_TASK_EXIT_CODE -eq 0 ]] && echo "$RUN_TASK_OUTPUT" | grep -q '"failures":\s*\[\]'; then +if [[ $RUN_TASK_EXIT_CODE -eq 0 ]] && echo "$RUN_TASK_OUTPUT" | jq -e '.failures == []' >/dev/null 2>&1; thenAlternative if preferring grep:
-if [[ $RUN_TASK_EXIT_CODE -eq 0 ]] && echo "$RUN_TASK_OUTPUT" | grep -q '"failures":\s*\[\]'; then +if [[ $RUN_TASK_EXIT_CODE -eq 0 ]] && echo "$RUN_TASK_OUTPUT" | grep -qE '"failures":[[:space:]]*\[\]'; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bootstrap-argocd.sh` around lines 122 - 127, The grep call that checks RUN_TASK_OUTPUT uses the PCRE-style escape \s which basic grep won't interpret, so replace the fragile grep with a jq-based check: parse RUN_TASK_OUTPUT with jq to verify .failures is an empty array (e.g. use jq -e '.failures == []' or check that (.failures | length) == 0) and base the conditional on that result; update the condition that currently references RUN_TASK_EXIT_CODE and the grep of RUN_TASK_OUTPUT to instead use the jq exit status on RUN_TASK_OUTPUT, leaving the subsequent extraction of TASK_ARN and its validation (the jq -r line and the check for empty/null TASK_ARN) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/bootstrap-argocd.sh`:
- Around line 99-106: Remove the duplicate CLUSTER_TYPE entry in the container
overrides environment array so CLUSTER_TYPE is only defined once; locate the
JSON/object array that contains entries like {"name": "CLUSTER_TYPE", "value":
"$CLUSTER_TYPE"} and delete the redundant occurrence (the second CLUSTER_TYPE
line) to avoid the copy-paste duplicate.
- Around line 122-127: The grep call that checks RUN_TASK_OUTPUT uses the
PCRE-style escape \s which basic grep won't interpret, so replace the fragile
grep with a jq-based check: parse RUN_TASK_OUTPUT with jq to verify .failures is
an empty array (e.g. use jq -e '.failures == []' or check that (.failures |
length) == 0) and base the conditional on that result; update the condition that
currently references RUN_TASK_EXIT_CODE and the grep of RUN_TASK_OUTPUT to
instead use the jq exit status on RUN_TASK_OUTPUT, leaving the subsequent
extraction of TASK_ARN and its validation (the jq -r line and the check for
empty/null TASK_ARN) unchanged.
In `@scripts/buildspec/provision-infra-mc.sh`:
- Around line 23-30: The destroy-path conditional that runs when
DELETE_FLAG=="true" is missing a placeholder for TF_VAR_rhobs_api_url, causing
terraform destroy planning to fail; update the DELETE_FLAG block (the exports
created in that if) to also export TF_VAR_rhobs_api_url with a harmless
placeholder value (e.g. export TF_VAR_rhobs_api_url="placeholder" or export
TF_VAR_rhobs_api_url=$(mktemp)) so the management-cluster terraform module has
the variable during planning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6600910e-df0d-46fa-8c43-69933d2884e3
📒 Files selected for processing (5)
scripts/bootstrap-argocd.shscripts/buildspec/provision-infra-mc.shscripts/buildspec/register.shscripts/pipeline-common/lib.shscripts/provision-pipelines.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/pipeline-common/lib.sh
- scripts/buildspec/register.sh
- scripts/provision-pipelines.sh
openshift-ci
Bot
added
the
needs-rebase
typeid
force-pushed
the
cbusse/script-cleanup
branch
from
June 22, 2026 11:55
bffe12d to
d7c048c
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/pipeline-common/lib.sh (1)
339-339: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueQuote numeric variable for shellcheck compliance.
$exit_codeshould be quoted per SC2086, even though it's numeric and safe in this context.♻️ Optional fix
- if [ $exit_code -ne 0 ]; then + if [ "$exit_code" -ne 0 ]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pipeline-common/lib.sh` at line 339, The variable $exit_code in the conditional statement needs to be quoted to comply with shellcheck rule SC2086. Change the condition that checks `if [ $exit_code -ne 0 ]` by adding double quotes around the $exit_code variable so it reads `if [ "$exit_code" -ne 0 ]` to properly quote the numeric variable reference.Source: Linters/SAST tools
scripts/buildspec/provision-infra-mc.sh (1)
25-26: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider separating declaration and assignment for
mktempcalls.Shellcheck SC2155 flags that combining
exportwith command substitution can mask non-zero return values. Whileset -ewould catch most failures, separating these is a defensive practice.This is minor since the destroy path is a controlled teardown, but would improve robustness.
♻️ Optional improvement
- export TF_VAR_maestro_agent_cert_file=$(mktemp) - export TF_VAR_maestro_agent_config_file=$(mktemp) + TF_VAR_maestro_agent_cert_file=$(mktemp) + TF_VAR_maestro_agent_config_file=$(mktemp) + export TF_VAR_maestro_agent_cert_file TF_VAR_maestro_agent_config_file🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/buildspec/provision-infra-mc.sh` around lines 25 - 26, The export statements for TF_VAR_maestro_agent_cert_file and TF_VAR_maestro_agent_config_file are combining export with command substitution, which can mask failures from the mktemp commands. Separate these operations by first capturing the output of each mktemp call into a local variable, then exporting those variables with the TF_VAR_ prefix. This ensures that any mktemp failures are properly detected rather than being masked by the export operation.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/buildspec/provision-infra-rc.sh`:
- Around line 103-104: The jq -r commands for exporting TF_VAR_regional_id and
TF_VAR_environment will output the literal string "null" if those fields are
missing from the DEPLOY_CONFIG_FILE, which causes issues with Terraform. Modify
both export statements to use jq's alternative operator (//) to provide sensible
fallback default values when the fields are absent or null in the JSON, ensuring
variables receive proper defaults instead of the literal "null" string.
---
Nitpick comments:
In `@scripts/buildspec/provision-infra-mc.sh`:
- Around line 25-26: The export statements for TF_VAR_maestro_agent_cert_file
and TF_VAR_maestro_agent_config_file are combining export with command
substitution, which can mask failures from the mktemp commands. Separate these
operations by first capturing the output of each mktemp call into a local
variable, then exporting those variables with the TF_VAR_ prefix. This ensures
that any mktemp failures are properly detected rather than being masked by the
export operation.
In `@scripts/pipeline-common/lib.sh`:
- Line 339: The variable $exit_code in the conditional statement needs to be
quoted to comply with shellcheck rule SC2086. Change the condition that checks
`if [ $exit_code -ne 0 ]` by adding double quotes around the $exit_code variable
so it reads `if [ "$exit_code" -ne 0 ]` to properly quote the numeric variable
reference.
🪄 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: Enterprise
Run ID: 5e6b000d-477a-4e8f-ae72-fb8f065fc636
📒 Files selected for processing (34)
docs/environment-provisioning.mddocs/sop/break-glass/README.mddocs/sop/break-glass/argocd-status.mdscripts/bootstrap-argocd.shscripts/buildspec/bootstrap-argocd-mc.shscripts/buildspec/bootstrap-argocd-rc.shscripts/buildspec/iot-mint.shscripts/buildspec/provision-infra-mc.shscripts/buildspec/provision-infra-rc.shscripts/buildspec/register.shscripts/dev/bastion-connect.shscripts/dev/bastion-port-forward.shscripts/dev/bastion-tasks-cleanup.shscripts/dev/check-api-gateway.shscripts/dev/customer-bastion.shscripts/dev/init-remote-backend.shscripts/dev/validate-argocd-config.shscripts/pipeline-common/account-helpers.shscripts/pipeline-common/assume-target-role.shscripts/pipeline-common/bootstrap-argocd-wrapper.shscripts/pipeline-common/detect-central-state.shscripts/pipeline-common/init-terraform-backend.shscripts/pipeline-common/lib.shscripts/pipeline-common/load-deploy-config.shscripts/pipeline-common/setup-apply-preflight.shscripts/pipeline-common/terraform-import.shscripts/pipeline-common/terraform-install.shscripts/provision-pipelines.shscripts/read-iot-state.shterraform/config/pipeline-management-cluster/buildspec-bootstrap-argocd.ymlterraform/config/pipeline-management-cluster/main.tfterraform/config/pipeline-regional-cluster/buildspec-bootstrap-argocd.ymlterraform/config/regional-cluster/imports.shterraform/modules/bastion/README.md
💤 Files with no reviewable changes (17)
- scripts/pipeline-common/init-terraform-backend.sh
- scripts/dev/bastion-connect.sh
- scripts/dev/check-api-gateway.sh
- scripts/pipeline-common/bootstrap-argocd-wrapper.sh
- scripts/pipeline-common/account-helpers.sh
- scripts/pipeline-common/terraform-import.sh
- scripts/read-iot-state.sh
- scripts/dev/bastion-port-forward.sh
- scripts/dev/validate-argocd-config.sh
- scripts/pipeline-common/terraform-install.sh
- scripts/pipeline-common/detect-central-state.sh
- scripts/pipeline-common/setup-apply-preflight.sh
- scripts/pipeline-common/load-deploy-config.sh
- scripts/pipeline-common/assume-target-role.sh
- scripts/dev/customer-bastion.sh
- scripts/dev/init-remote-backend.sh
- scripts/dev/bastion-tasks-cleanup.sh
✅ Files skipped from review due to trivial changes (5)
- terraform/config/pipeline-regional-cluster/buildspec-bootstrap-argocd.yml
- docs/environment-provisioning.md
- terraform/modules/bastion/README.md
- docs/sop/break-glass/argocd-status.md
- docs/sop/break-glass/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- terraform/config/pipeline-management-cluster/buildspec-bootstrap-argocd.yml
- terraform/config/regional-cluster/imports.sh
- scripts/bootstrap-argocd.sh
- scripts/provision-pipelines.sh
Collaborator
Author
|
/test on-demand-e2e |
Merge 8 separate pipeline-common helper scripts into a single lib.sh, simplify all buildspec scripts to use the shared library, remove 7 unused dev scripts, and fix missing TARGET_ACCOUNT_ID in the iot_mint CodeBuild project. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
typeid
force-pushed
the
cbusse/script-cleanup
branch
from
June 23, 2026 11:19
d7c048c to
6ab37bf
Compare
Collaborator
Author
|
/test on-demand-e2e |
Collaborator
Author
|
/test on-demand-e2e Re-running tests as I believe ZOA issue is unrelated flake. |
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamkirkbater 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 |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
merged commit Jun 23, 2026
ed4548a
into
openshift-online:main
9 checks passed
This was referenced Jul 6, 2026
Closed
This was referenced Jul 21, 2026
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.
Summary
bastion-connect.sh,bastion-port-forward.sh,customer-bastion.sh,check-api-gateway.sh,init-remote-backend.sh,validate-argocd-config.sh,bastion-tasks-cleanup.sh) and update 4 docs that referenced thempipeline-common/*.shfiles into a singlelib.shfunction library — eliminates 5-7 level sourcing chains; each buildspec script now sources one file and calls named functions (preflight_check,config_load,use_mc_account,terraform_init_backend, etc.)Net effect: -2,208 lines removed, sourcing depth reduced from 5-7 levels to 1, and pipeline logs become actionable.
Test plan
make pre-pushpasses locally (terraform-fmt, check-docs, check-rendered-files, helm-lint)🤖 Generated with Claude Code
Summary by CodeRabbit