Add central config/ folder with sync-to-canonical#92
Open
fonta-rh wants to merge 10 commits into
Open
Conversation
Add hack/ansible-lint.sh following the containerized pattern of the existing hack scripts. It runs ansible-lint (configured by the new .ansible-lint, with pre-existing findings baselined in a skip list) and ansible-playbook --syntax-check for every playbook, caching the required collections in ~/.cache/tnt-ansible-collections. Also validate topology and method in setup.yml pre_tasks regardless of interactive_mode, so bad -e values fail fast instead of erroring deep inside a role. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUa6KmTHSMNx9SibfQrLNm
Add a single config/ folder at the repository root where users create their configuration files. Every make-accessible script syncs config/ files to their canonical locations (newer-wins, never deletes) before doing anything else; the playbooks and roles keep reading from the canonical locations unchanged. - Move all templates and example configs into config/ (single source): instance.env.template, config_*_example.sh, kcli.yml.template, assisted.yml.template, plus a README map and a .gitignore that prevents real config files (including the pull secret) from ever being committed - Add sync_config_files() to aws-hypervisor/scripts/common.sh with a manifest of src:dest[:dest2] entries; the pull secret fans out to both the dev-scripts and kcli role locations - Restructure common.sh so the sync runs before instance.env is sourced, and replace the raw missing-file source error with an actionable hint - Add 'make sync-config' for users who invoke ansible-playbook directly, and register init-host.yml.local in the manifest (it is consumed by init-host.yml) - Source common.sh from get-tnf-logs.sh and helpers/keep-instance.sh so every make target inherits the sync - Gitignore vars/assisted.yml (contains credentials, was not ignored) - Update all docs and the /setup command to point at config/, keeping a note that editing canonical locations directly still works Backwards compatible: config/ ships with templates only, so the sync is a no-op until the user explicitly creates a file there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mk1uuxv3YkApKm8w55Kxjd
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fonta-rh 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
Contributor
WalkthroughThis PR centralizes configuration under ChangesCentral config, shared helpers, and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
the
ready-for-human-review
![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: 8
🧹 Nitpick comments (1)
hack/ansible-lint.sh (1)
19-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOverloading
OPENSHIFT_CIas a "skip containerization" flag is confusing.
OPENSHIFT_CIis normally a real environment signal set by Prow/OpenShift CI. Here it's repurposed to mean "ansible-lint/ansible-playbook are already installed, run natively." If a future CI runner setsOPENSHIFT_CIfor unrelated reasons without those tools present, this branch will run natively and fail. Consider a dedicated variable (e.g.SKIP_CONTAINER/NATIVE_RUN) decoupled from the CI-detection semantics ofOPENSHIFT_CI.🤖 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 `@hack/ansible-lint.sh` around lines 19 - 33, The current ansible-lint native-execution branch is keyed off OPENSHIFT_CI, which mixes CI detection with the decision to skip containerization. Update the logic in ansible-lint.sh to use a dedicated flag such as SKIP_CONTAINER or NATIVE_RUN for the native path, and keep OPENSHIFT_CI only as a real CI environment signal. Make sure the condition around the ansible-galaxy install and ansible-lint invocation uses the new flag so future CI environments won’t accidentally trigger the native branch.
🤖 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 @.claude/commands/setup.md:
- Around line 211-212: The markdown list under the numbered setup step has
inconsistent nested bullet indentation, causing MD005. Update the bullet items
in the setup instructions so they are aligned consistently with the other
sibling list items in this section, keeping the existing content but fixing the
nested list structure.
- Around line 145-147: Update the validation example to match the documented
fallback behavior by checking whichever config file exists instead of always
sourcing config/instance.env. Use the existing setup guidance and referenced
paths in the validation block to either source the available file from
config/instance.env or deploy/aws-hypervisor/instance.env, or explicitly state
that the validation step assumes make sync-config has already run.
- Around line 73-75: The pull-secret validation guidance is hardcoded to
config/pull-secret.json, which ignores the supported fallback location and can
mislead users. Update the validation examples in the setup instructions to
either reference the actual existing pull secret path used by the workflow or
explicitly instruct users to sync/copy the secret first; keep the validation
steps aligned with the path handling in the setup process.
In `@deploy/aws-hypervisor/README.md`:
- Around line 8-16: The README wording overstates the sync behavior for all make
targets; narrow the guarantee to the AWS/deployment workflow or the make
sync-config target. Update the guidance around the config/instance.env.template
to config/instance.env flow so it only claims synchronization where it actually
happens, and keep the description aligned with the current make targets used by
the deploy/aws-hypervisor setup.
In `@deploy/openshift-clusters/README-kcli.md`:
- Around line 56-65: The pull-secret example in README-kcli.md is generating
invalid JSON and may alter pasted secret contents. Update the instructions
around the config/pull-secret.json example to use a verbatim copy workflow or a
properly quoted heredoc so the `auths` payload is preserved exactly, and keep
the guidance aligned with the `make sync-config` / `ansible-playbook` flow
described nearby.
In `@deploy/openshift-clusters/roles/kcli/kcli-install/README.md`:
- Around line 135-143: The pull-secret example in the README currently shows an
invalid JSON heredoc, which will create a broken config file if copied verbatim.
Update the instructions around the pull secret step to use a verbatim copy/paste
approach or a clearly valid JSON example, and ensure the guidance in the
pull-secret section is consistent with the `config/pull-secret.json` file and
the `make sync-config` flow.
In `@deploy/README.md`:
- Line 43: The deployment README wording overstates that every make target syncs
config files, but only the config-aware deploy targets do so. Update the
guidance around instance.env and make sync-config in deploy/README.md to clarify
that utility targets like verify do not copy config/ into place, and reference
the existing deploy target behavior so users know when explicit syncing is still
required.
In `@README.md`:
- Around line 9-10: The README wording is too broad because not every make
target performs config syncing. Update the sentence in the documentation around
the config-sync description to scope it to the deploy targets/scripts that
source common.sh, or explicitly call out make sync-config, so the claim matches
the behavior of the relevant make targets.
---
Nitpick comments:
In `@hack/ansible-lint.sh`:
- Around line 19-33: The current ansible-lint native-execution branch is keyed
off OPENSHIFT_CI, which mixes CI detection with the decision to skip
containerization. Update the logic in ansible-lint.sh to use a dedicated flag
such as SKIP_CONTAINER or NATIVE_RUN for the native path, and keep OPENSHIFT_CI
only as a real CI environment signal. Make sure the condition around the
ansible-galaxy install and ansible-lint invocation uses the new flag so future
CI environments won’t accidentally trigger the native branch.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b7c71480-a2a0-4701-97ee-786786d1f01b
📒 Files selected for processing (31)
.ansible-lint.claude/commands/setup.md.claude/commands/setup/README.mdCLAUDE.mdMakefileREADME.mdconfig/.gitignoreconfig/README.mdconfig/assisted.yml.templateconfig/config_arbiter_example.shconfig/config_fencing_example.shconfig/config_sno_example.shconfig/instance.env.templateconfig/kcli.yml.templatedeploy/Makefiledeploy/README.mddeploy/aws-hypervisor/README.mddeploy/aws-hypervisor/scripts/common.shdeploy/openshift-clusters/.gitignoredeploy/openshift-clusters/README-external-host.mddeploy/openshift-clusters/README-kcli.mddeploy/openshift-clusters/README.mddeploy/openshift-clusters/assisted-install.ymldeploy/openshift-clusters/roles/assisted/assisted-spoke/README.mddeploy/openshift-clusters/roles/kcli/kcli-install/README.mddeploy/openshift-clusters/scripts/deploy-fencing-assisted.shdeploy/openshift-clusters/scripts/get-tnf-logs.shdeploy/openshift-clusters/setup.ymlhack/ansible-lint.shhack/pre-commithelpers/keep-instance.sh
Comment on lines
+211
to
212
| - The sync distributes these to `deploy/openshift-clusters/roles/dev-scripts/install-dev/files/`; suggest `cd deploy && make sync-config` before running the playbook directly | ||
| - **Config reference**: https://github.com/openshift-metal3/dev-scripts/blob/master/config_example.sh |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the nested list indentation.
The bullet indentation under the numbered item is inconsistent here and triggers MD005. Based on static analysis, align it with the sibling list items.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 211-211: Inconsistent indentation for list items at the same level
Expected: 6; Actual: 3
(MD005, list-indent)
[warning] 212-212: Inconsistent indentation for list items at the same level
Expected: 6; Actual: 3
(MD005, list-indent)
🤖 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 @.claude/commands/setup.md around lines 211 - 212, The markdown list under
the numbered setup step has inconsistent nested bullet indentation, causing
MD005. Update the bullet items in the setup instructions so they are aligned
consistently with the other sibling list items in this section, keeping the
existing content but fixing the nested list structure.
Source: Linters/SAST tools
Contributor
Author
There was a problem hiding this comment.
Won't fix — this is a Claude Code prompt file, not rendered documentation.
Contributor
There was a problem hiding this comment.
@fonta-rh, understood — thanks for the context. Since this file is consumed as a Claude Code prompt rather than rendered Markdown, the indentation styling isn't functionally important. I'll skip flagging this going forward.
Comment on lines
+135
to
+143
| 2. Download OpenShift pull secret and place it in the central `config/` folder: | ||
| ```bash | ||
| # Navigate to the kcli-install files directory | ||
| cd roles/kcli/kcli-install/files/ | ||
|
|
||
| # Create pull secret file (paste your pull secret content) | ||
| cat > pull-secret.json << EOF | ||
| # From the repository root, create the pull secret file (paste your pull secret content) | ||
| cat > config/pull-secret.json << EOF | ||
| {"auths":{"your-pull-secret-content-here"}} | ||
| EOF | ||
|
|
||
| # Distribute it to the role files directory (or let any make target do it) | ||
| cd deploy/ && make sync-config && cd ../ |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the pull-secret example.
This heredoc writes invalid JSON as shown, so a user copying it verbatim will create a broken file. Please switch to a verbatim copy instruction or otherwise show valid JSON.
🔧 Suggested fix
- cat > config/pull-secret.json << EOF
- {"auths":{"your-pull-secret-content-here"}}
- EOF
+ cp /path/to/downloaded/pull-secret.json config/pull-secret.json📝 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
| 2. Download OpenShift pull secret and place it in the central `config/` folder: | |
| ```bash | |
| # Navigate to the kcli-install files directory | |
| cd roles/kcli/kcli-install/files/ | |
| # Create pull secret file (paste your pull secret content) | |
| cat > pull-secret.json << EOF | |
| # From the repository root, create the pull secret file (paste your pull secret content) | |
| cat > config/pull-secret.json << EOF | |
| {"auths":{"your-pull-secret-content-here"}} | |
| EOF | |
| # Distribute it to the role files directory (or let any make target do it) | |
| cd deploy/ && make sync-config && cd ../ | |
| 2. Download OpenShift pull secret and place it in the central `config/` folder: |
🤖 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 `@deploy/openshift-clusters/roles/kcli/kcli-install/README.md` around lines 135
- 143, The pull-secret example in the README currently shows an invalid JSON
heredoc, which will create a broken config file if copied verbatim. Update the
instructions around the pull secret step to use a verbatim copy/paste approach
or a clearly valid JSON example, and ensure the guidance in the pull-secret
section is consistent with the `config/pull-secret.json` file and the `make
sync-config` flow.
Contributor
Author
There was a problem hiding this comment.
Won't fix — same reasoning as the README-kcli.md finding above.
Contributor
There was a problem hiding this comment.
@fonta-rh, understood — acknowledged, and I'll leave this as-is consistent with the decision on the README-kcli.md finding.
… guard Move common.sh from deploy/aws-hypervisor/scripts/ to deploy/ so all deploy paths (AWS and baremetal) can source it without reaching into aws-hypervisor internals. Add config_baremetal_fencing.sh to the sync manifest and have baremetal-adopt generate it into config/. Skip syncing 0-byte source files to prevent accidental credential blanking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deploy/openshift-clusters/scripts/deploy-baremetal.sh (1)
13-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent ordering:
common.shsourced after strict mode is enabled.Every other script in this cohort sources
common.shbeforeset -o nounset/errexit/pipefail. Here the order is reversed, socommon.sh's side effects (e.g.sync_config_files) run under strict mode uniquely in this script — any future unset-variable reference or non-fatal-but-nonzero command there would abort this script differently than its siblings.🔧 Suggested fix: match sibling script ordering
-set -o nounset -set -o errexit -set -o pipefail - SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" OC_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" DEPLOY_DIR="$(cd "${OC_DIR}/.." && pwd)" # shellcheck source=/dev/null source "${DEPLOY_DIR}/common.sh" + +set -o nounset +set -o errexit +set -o pipefail🤖 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 `@deploy/openshift-clusters/scripts/deploy-baremetal.sh` around lines 13 - 21, The script initialization order is inconsistent with its sibling deploy scripts: `common.sh` is sourced after `set -o nounset`, `set -o errexit`, and `set -o pipefail`, so its side effects run under strict mode only here. Update `deploy-baremetal.sh` to match the cohort by moving the `source "${DEPLOY_DIR}/common.sh"` step before enabling strict mode, keeping the existing `SCRIPT_DIR`, `OC_DIR`, and `DEPLOY_DIR` setup intact.
🤖 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 `@deploy/openshift-clusters/scripts/baremetal-adopt.sh`:
- Around line 511-525: The baremetal adoption flow overwrites the generated
config file unconditionally, which can silently erase user edits to
config_baremetal_fencing.sh. Update generate_baremetal_config to detect an
existing output_file before writing and either back it up, refuse to overwrite,
or prompt/warn the user; keep the behavior aligned with baremetal-adopt and the
sync-config workflow so manual edits to CI_TOKEN and OPENSHIFT_RELEASE_IMAGE are
preserved.
---
Nitpick comments:
In `@deploy/openshift-clusters/scripts/deploy-baremetal.sh`:
- Around line 13-21: The script initialization order is inconsistent with its
sibling deploy scripts: `common.sh` is sourced after `set -o nounset`, `set -o
errexit`, and `set -o pipefail`, so its side effects run under strict mode only
here. Update `deploy-baremetal.sh` to match the cohort by moving the `source
"${DEPLOY_DIR}/common.sh"` step before enabling strict mode, keeping the
existing `SCRIPT_DIR`, `OC_DIR`, and `DEPLOY_DIR` setup intact.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 44ba54d8-b931-42d7-8374-f660c61d9a3b
📒 Files selected for processing (25)
CLAUDE.mdconfig/README.mddeploy/Makefiledeploy/aws-hypervisor/scripts/create.shdeploy/aws-hypervisor/scripts/destroy.shdeploy/aws-hypervisor/scripts/force-stop.shdeploy/aws-hypervisor/scripts/init.shdeploy/aws-hypervisor/scripts/inventory.shdeploy/aws-hypervisor/scripts/print_instance_data.shdeploy/aws-hypervisor/scripts/ssh.shdeploy/aws-hypervisor/scripts/start.shdeploy/aws-hypervisor/scripts/stop.shdeploy/common.shdeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/clean-spoke.shdeploy/openshift-clusters/scripts/clean.shdeploy/openshift-clusters/scripts/deploy-baremetal.shdeploy/openshift-clusters/scripts/deploy-cluster.shdeploy/openshift-clusters/scripts/deploy-fencing-assisted.shdeploy/openshift-clusters/scripts/full-clean.shdeploy/openshift-clusters/scripts/get-tnf-logs.shdeploy/openshift-clusters/scripts/patch-nodes.shdeploy/openshift-clusters/scripts/redeploy-cluster.shdeploy/openshift-clusters/scripts/shutdown-cluster.shdeploy/openshift-clusters/scripts/startup-cluster.sh
✅ Files skipped from review due to trivial changes (7)
- deploy/aws-hypervisor/scripts/init.sh
- deploy/aws-hypervisor/scripts/ssh.sh
- deploy/aws-hypervisor/scripts/destroy.sh
- deploy/aws-hypervisor/scripts/print_instance_data.sh
- deploy/aws-hypervisor/scripts/force-stop.sh
- config/README.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/openshift-clusters/scripts/get-tnf-logs.sh
- deploy/Makefile
5 tasks
Contributor
Code reviewFound 2 issues:
two-node-toolbox/helpers/keep-instance.sh Lines 16 to 18 in 114df4a
two-node-toolbox/hack/ansible-lint.sh Lines 5 to 7 in 114df4a 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
lucaconsalvi
reviewed
Jul 9, 2026
lucaconsalvi
left a comment
lucaconsalvi
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the excellent work here, Pablo! This is a really well-thought-out change — the backwards-compatible sync design (newer-wins, never deletes), the 0-byte file protection, and the thorough documentation updates across every README are all top-notch. The config/.gitignore setup cleanly prevents credential commits, and the ansible-lint integration with a baselined skip list is exactly the right approach for incremental adoption.
I found one critical bug and a few error-handling concerns detailed in the inline comments below.
Summary
| Severity | Count | Key Findings |
|---|---|---|
| Critical | 1 | keep-instance.sh sources deleted path — will crash at runtime |
| High | 2 | cp failure silently ignored in sync, make sync-config masks errors |
| Medium | 3 | Spurious instance.env warning for non-AWS scripts, errexit ordering inconsistency, *.yml glob fragility |
Minor (no inline comment)
deploy/openshift-clusters/.gitignoreis still missing a trailing newline (pre-existing, extended by this PR)- CodeRabbit's finding about
baremetal-adoptsilently clobbering hand-editedconfig/config_baremetal_fencing.shon re-run is worth considering
Re-running `make baremetal-adopt` after editing config_baremetal_fencing.sh (e.g. to set CI_TOKEN) would silently overwrite user edits. Back up the existing file with a timestamped suffix before regenerating. Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed CodeRabbit review-body nitpick findings:
|
- Fix broken common.sh path in keep-instance.sh after move to deploy/ - Handle cp failures in sync_config_files() so errors are reported instead of silently incrementing the sync count - Add nullglob guard in ansible-lint.sh syntax-check loop Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 (2)
deploy/openshift-clusters/scripts/clean-spoke.sh (1)
6-14: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSet error-handling flags before sourcing
common.sh.
set -o nounset/errexit/pipefailare set only aftersource "${DEPLOY_DIR}/common.sh". If the source fails (wrong path, missing file), the script won't fail fast and will continue with undefined functions/variables. This is the same source-then-set-flags pattern already acknowledged (but deferred) fordeploy-baremetal.sh; worth applying the same fix consistently here.🛠️ Proposed fix
SCRIPT_DIR=$(dirname "$0") DEPLOY_DIR="$(cd "${SCRIPT_DIR}/../.." && pwd)" +set -o nounset +set -o errexit +set -o pipefail + # shellcheck source=/dev/null source "${DEPLOY_DIR}/common.sh" -set -o nounset -set -o errexit -set -o pipefail - NODE_DIR="$(get_node_dir)"🤖 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 `@deploy/openshift-clusters/scripts/clean-spoke.sh` around lines 6 - 14, The script currently sources common.sh before enabling nounset/errexit/pipefail, so a missing or bad source can pass silently; move the set -o flags ahead of the source step in clean-spoke.sh, matching the safer pattern used elsewhere and keeping SCRIPT_DIR/DEPLOY_DIR resolution intact.deploy/openshift-clusters/scripts/patch-nodes.sh (1)
4-13: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSet error-handling flags before sourcing
common.sh.Same issue as in
clean-spoke.sh:set -o nounset/errexit/pipefailare applied aftersource "${DEPLOY_DIR}/common.sh", so a failed source won't abort the script early.🛠️ Proposed fix
DEPLOY_DIR="$(cd "${SCRIPT_DIR}/../.." && pwd)" +set -o nounset +set -o errexit +set -o pipefail + # shellcheck source=/dev/null source "${DEPLOY_DIR}/common.sh" -set -o nounset -set -o errexit -set -o pipefail - # Check if instance data exists🤖 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 `@deploy/openshift-clusters/scripts/patch-nodes.sh` around lines 4 - 13, The script in patch-nodes.sh sets nounset/errexit/pipefail only after sourcing common.sh, so a failed source may not stop execution. Move the set -o nounset, set -o errexit, and set -o pipefail lines to run before the source "${DEPLOY_DIR}/common.sh" statement, keeping SCRIPT_DIR/DEPLOY_DIR setup intact.
🤖 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 `@deploy/openshift-clusters/scripts/clean-spoke.sh`:
- Around line 6-14: The script currently sources common.sh before enabling
nounset/errexit/pipefail, so a missing or bad source can pass silently; move the
set -o flags ahead of the source step in clean-spoke.sh, matching the safer
pattern used elsewhere and keeping SCRIPT_DIR/DEPLOY_DIR resolution intact.
In `@deploy/openshift-clusters/scripts/patch-nodes.sh`:
- Around line 4-13: The script in patch-nodes.sh sets nounset/errexit/pipefail
only after sourcing common.sh, so a failed source may not stop execution. Move
the set -o nounset, set -o errexit, and set -o pipefail lines to run before the
source "${DEPLOY_DIR}/common.sh" statement, keeping SCRIPT_DIR/DEPLOY_DIR setup
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 01a87fb2-0b11-44a5-a2e5-eca3ba41835d
📒 Files selected for processing (50)
.ansible-lint.claude/commands/setup.md.claude/commands/setup/README.mdCLAUDE.mdMakefileREADME.mdconfig/.gitignoreconfig/README.mdconfig/assisted.yml.templateconfig/config_arbiter_example.shconfig/config_fencing_example.shconfig/config_sno_example.shconfig/instance.env.templateconfig/kcli.yml.templatedeploy/Makefiledeploy/README.mddeploy/aws-hypervisor/README.mddeploy/aws-hypervisor/scripts/create.shdeploy/aws-hypervisor/scripts/destroy.shdeploy/aws-hypervisor/scripts/force-stop.shdeploy/aws-hypervisor/scripts/init.shdeploy/aws-hypervisor/scripts/inventory.shdeploy/aws-hypervisor/scripts/print_instance_data.shdeploy/aws-hypervisor/scripts/ssh.shdeploy/aws-hypervisor/scripts/start.shdeploy/aws-hypervisor/scripts/stop.shdeploy/common.shdeploy/openshift-clusters/.gitignoredeploy/openshift-clusters/README-external-host.mddeploy/openshift-clusters/README-kcli.mddeploy/openshift-clusters/README.mddeploy/openshift-clusters/assisted-install.ymldeploy/openshift-clusters/roles/assisted/assisted-spoke/README.mddeploy/openshift-clusters/roles/kcli/kcli-install/README.mddeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/clean-spoke.shdeploy/openshift-clusters/scripts/clean.shdeploy/openshift-clusters/scripts/deploy-baremetal.shdeploy/openshift-clusters/scripts/deploy-cluster.shdeploy/openshift-clusters/scripts/deploy-fencing-assisted.shdeploy/openshift-clusters/scripts/full-clean.shdeploy/openshift-clusters/scripts/get-tnf-logs.shdeploy/openshift-clusters/scripts/patch-nodes.shdeploy/openshift-clusters/scripts/redeploy-cluster.shdeploy/openshift-clusters/scripts/shutdown-cluster.shdeploy/openshift-clusters/scripts/startup-cluster.shdeploy/openshift-clusters/setup.ymlhack/ansible-lint.shhack/pre-commithelpers/keep-instance.sh
- Makefile sync-config: add set -e so cp failures propagate - deploy-baremetal.sh: source common.sh before strict flags for consistent sync error handling across scripts - ansible-lint.sh: quote $CONTAINER_ENGINE and $CONTAINER_IMAGE Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Callers like doctor.sh that have their own instance.env validation can suppress the generic warning without blanket stderr suppression. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Playbooks assisted-install.yml and kcli-install.yml reference roles via the `role:` syntax that ansible-lint resolves at the repo root. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Re: CodeRabbit review-body findings (outside diff range)
|
…quisite sync_config_files() was called at module scope whenever any script sourced common.sh, making every import a side-effecting operation. This forced callers like doctor.sh to opt out via SKIP_CONFIG_SYNC=1. Move the sync to a Makefile prerequisite on deploy and cluster targets only, keeping common.sh as a pure library. The sync-config target now explicitly calls sync_config_files, and only targets that actually consume synced config files depend on it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
/test images |
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
config/folder at the repository root where users create configuration files. Everymaketarget syncsconfig/files to their canonical locations before doing anything else (newer-wins, never deletes), so the playbooks and roles keep reading from the canonical locations unchanged.config/(instance.env.template, config_*_example.sh, kcli.yml.template, assisted.yml.template) with a README map and a.gitignorethat prevents real config files from being committed.common.shfromdeploy/aws-hypervisor/scripts/todeploy/so all deploy paths (AWS and baremetal) can source it without reaching intoaws-hypervisor/internals. Update all 21 scripts that source it.sync_config_files()tocommon.shwith a manifest of src→dest entries; the pull secret fans out to both the dev-scripts and kcli role locations. Skip syncing 0-byte source files to prevent accidental credential blanking.config_baremetal_fencing.shto the sync manifest and havebaremetal-adoptgenerate it intoconfig/.make sync-configfor users who invokeansible-playbookdirectly.make ansible-lint/make verifyintegration with ansible-lint and playbook syntax checks (hack/ansible-lint.sh), with pre-existing findings baselined in.ansible-lint.setup.ymlpre_tasks (regardless ofinteractive_mode).Motivation
New contributors currently need to know which of several deeply nested directories to place their config files in (pull secret, topology config, instance.env, etc.), and there's no protection against accidentally committing credentials. This PR introduces a single, gitignored
config/folder as the entry point — the sync mechanism is backwards compatible (no-op until the user creates a file there, and direct edits to canonical locations are preserved).The
common.shrelocation (commit 3) removes a coupling where OC scripts and baremetal scripts had to reach intoaws-hypervisor/to source shared code.Test plan
make sync-configcopies files fromconfig/to canonical locationsconfig/, run anymaketarget, confirm the canonical location is updatedroles/dev-scripts/install-dev/files/config_fencing.sh), confirm it is NOT overwritten by an olderconfig/copy.gitignoreprevents committing real config files inconfig/make ansible-lintruns cleanly on the current playbooksmake verifyincludes the ansible-lint checkssetup.ymlrejects invalid-e topology=boguswith a clear errorcommon.shrelocation: all 19 make targets that sourcecommon.shrun successfully (full EC2 instance lifecycle exercised — create, init, inventory, info, ssh, force-stop, start, destroy; cluster operation targets confirmed common.sh sources correctly)instance.envandconfig_fencing.shboth synced fromconfig/to canonical locations)🤖 Generated with Claude Code
Summary by CodeRabbit
config/source-of-truth workflow, including automatic syncing into canonical deployment locations viamake sync-config.make ansible-lintand broadermake verifychecks.config/layout, syncing behavior, and updated lint/validation commands.