feat: add 'make doctor' configuration preflight#93
Open
fonta-rh wants to merge 15 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
… 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>
|
[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, validation, and tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (6 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: 3
🧹 Nitpick comments (1)
deploy/common.sh (1)
38-62: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winmtime-based sync is fragile; consider content comparison and defensive
mkdir -p.
sync_config_files()relies purely on-nt(mtime) to decide whether to overwrite a canonical destination. Git checkouts/clones can produce inconsistent mtimes across files (depending on tooling), so a config/ file could appear "newer" than a destination the user intentionally edited directly, silently overwriting their edits — despite the comment stating "direct edits to the canonical locations are never overwritten."Additionally,
cp "${src}" "${REPO_ROOT}/${dest}"has nomkdir -p "$(dirname ...)"guard; if a destination's parent directory is ever missing, this fails silently (this file is sourced before callers seterrexit).♻️ Suggested hardening
for dest in "${fields[@]:1}"; do - if [[ ! -f "${REPO_ROOT}/${dest}" || "${src}" -nt "${REPO_ROOT}/${dest}" ]]; then + if [[ ! -f "${REPO_ROOT}/${dest}" || "${src}" -nt "${REPO_ROOT}/${dest}" ]]; then + if [[ -f "${REPO_ROOT}/${dest}" ]] && cmp -s "${src}" "${REPO_ROOT}/${dest}"; then + continue # content identical, skip noisy "updating" message + fi msg_info "config/${fields[0]} is newer, updating ${dest}" + mkdir -p "$(dirname "${REPO_ROOT}/${dest}")" cp "${src}" "${REPO_ROOT}/${dest}" CONFIG_SYNCED_COUNT=$((CONFIG_SYNCED_COUNT + 1)) fi done🤖 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/common.sh` around lines 38 - 62, The sync_config_files() logic currently uses only mtime (-nt) to decide updates, which can overwrite user-edited canonical files when mtimes are inconsistent. Update the decision in sync_config_files() to compare file contents or otherwise verify the destination hasn’t changed before copying from config/. Also add a defensive mkdir -p for the destination’s parent directory before the cp in the loop so missing directories don’t cause a failed sync.
🤖 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 @.ansible-lint:
- Around line 16-17: The mocked roles list is incomplete in the ansible lint
config: mock_roles currently only includes git-user, but the playbooks
referenced by deploy/openshift-clusters/assisted-install.yml and
deploy/openshift-clusters/kcli-install.yml also need assisted/acm-install,
assisted/assisted-spoke, and kcli/kcli-redfish added. Update the mock_roles
section in .ansible-lint so these additional roles are included alongside
git-user.
In `@deploy/common.sh`:
- Around line 65-70: Keep the optional source in common.sh, but add an upfront
REGION presence check in the AWS-only entrypoints create.sh, start.sh, stop.sh,
and destroy.sh before any later use of instance.env-derived values. Use those
scripts’ main entry flow to fail fast with a clear message pointing to make
doctor and the instance.env.template setup, so they exit cleanly instead of
reaching an unbound variable error.
In `@deploy/openshift-clusters/scripts/doctor.sh`:
- Around line 243-247: The instance.env sourcing check in doctor.sh is capturing
the error output incorrectly, leaving SOURCE_ERR empty in the failure path.
Update the source command inside the if block so stderr is captured into
SOURCE_ERR before any output is redirected away, and keep the existing
check_pass/check_fail flow intact. Use the surrounding source "${INSTANCE_ENV}"
validation logic to locate the fix and ensure the [FAIL] message includes the
actual parse error text.
---
Nitpick comments:
In `@deploy/common.sh`:
- Around line 38-62: The sync_config_files() logic currently uses only mtime
(-nt) to decide updates, which can overwrite user-edited canonical files when
mtimes are inconsistent. Update the decision in sync_config_files() to compare
file contents or otherwise verify the destination hasn’t changed before copying
from config/. Also add a defensive mkdir -p for the destination’s parent
directory before the cp in the loop so missing directories don’t cause a failed
sync.
🪄 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: 010ceb8c-038a-43fe-9e85-87606ff36890
📒 Files selected for processing (51)
.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/doctor.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
Comment on lines
+65
to
+70
| if [[ -f "${COMMON_DIR}/aws-hypervisor/instance.env" ]]; then | ||
| # shellcheck source=/dev/null | ||
| source "${COMMON_DIR}/aws-hypervisor/instance.env" | ||
| else | ||
| msg_warning "instance.env not found (only needed for AWS hypervisor targets). To create it: cp config/instance.env.template config/instance.env and edit it." | ||
| fi |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo "== deploy/common.sh (relevant section) =="
nl -ba deploy/common.sh | sed -n '1,140p'
echo
echo "== call sites that source deploy/common.sh =="
rg -n --glob '*.sh' 'source .*deploy/common\.sh|source .*common\.sh' deploy
echo
echo "== references to instance.env / REGION in deploy/ =="
rg -n --glob '*.sh' 'instance\.env|REGION|make doctor|nounset|set -u' deployRepository: openshift-eng/two-node-toolbox
Length of output: 250
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo "== deploy/common.sh (lines 1-140) =="
awk 'NR>=1 && NR<=140 { printf "%5d %s\n", NR, $0 }' deploy/common.sh
echo
echo "== scripts that source deploy/common.sh =="
rg -n --glob '*.sh' 'source .*common\.sh|source .*deploy/common\.sh' deploy
echo
echo "== instance.env / REGION / doctor references under deploy/ =="
rg -n --glob '*.sh' 'instance\.env|REGION|make doctor|nounset|set -u|set -o nounset' deployRepository: openshift-eng/two-node-toolbox
Length of output: 21326
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo "== deploy/openshift-clusters/scripts/doctor.sh (lines 230-255) =="
awk 'NR>=230 && NR<=255 { printf "%5d %s\n", NR, $0 }' deploy/openshift-clusters/scripts/doctor.sh
echo
echo "== deploy/aws-hypervisor/scripts/start.sh (lines 1-90) =="
awk 'NR>=1 && NR<=90 { printf "%5d %s\n", NR, $0 }' deploy/aws-hypervisor/scripts/start.sh
echo
echo "== deploy/aws-hypervisor/scripts/stop.sh (lines 1-90) =="
awk 'NR>=1 && NR<=90 { printf "%5d %s\n", NR, $0 }' deploy/aws-hypervisor/scripts/stop.shRepository: openshift-eng/two-node-toolbox
Length of output: 10522
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo "== AWS entrypoints around common.sh / nounset =="
for f in deploy/aws-hypervisor/scripts/{create.sh,destroy.sh,force-stop.sh,init.sh,inventory.sh,ssh.sh,start.sh,stop.sh}; do
echo
echo "### $f"
awk 'NR>=1 && NR<=25 { printf "%5d %s\n", NR, $0 }' "$f"
done
echo
echo "== Existing guards for REGION / instance.env =="
rg -n --glob '*.sh' 'REGION:-|AWS_PROFILE:-|instance\.env.*not found|run '\''make doctor'\''|doctor\.sh|check_fail "REGION|check_note "section skipped: instance.env' deployRepository: openshift-eng/two-node-toolbox
Length of output: 8963
Keep instance.env optional here, but fail fast in AWS-only entrypoints
deploy/common.sh:65-70 can stay non-fatal for non-AWS flows, but create.sh, start.sh, stop.sh, and destroy.sh should check REGION up front and exit with a clear make doctor/instance.env.template message instead of hitting an unbound variable later.
🤖 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/common.sh` around lines 65 - 70, Keep the optional source in
common.sh, but add an upfront REGION presence check in the AWS-only entrypoints
create.sh, start.sh, stop.sh, and destroy.sh before any later use of
instance.env-derived values. Use those scripts’ main entry flow to fail fast
with a clear message pointing to make doctor and the instance.env.template
setup, so they exit cleanly instead of reaching an unbound variable error.
Source: Learnings
Comment on lines
+243
to
+247
| if SOURCE_ERR=$( (set -e; set +u; source "${INSTANCE_ENV}") 2>&1 >/dev/null ); then | ||
| check_pass "instance.env sources cleanly" | ||
| else | ||
| check_fail "instance.env does not source cleanly: ${SOURCE_ERR}" \ | ||
| "fix the shell syntax in ${CONFIG_DIR}/instance.env" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Capture the source error before reporting the failure.
SOURCE_ERR ends up empty here because the redirections send stderr to the terminal instead of into the variable, so the [FAIL] message loses the actual parse error.
🐛 Proposed fix
- if SOURCE_ERR=$( (set -e; set +u; source "${INSTANCE_ENV}") 2>&1 >/dev/null ); then
+ if SOURCE_ERR=$(
+ {
+ (set -e; set +u; source "${INSTANCE_ENV}") >/dev/null
+ } 2>&1
+ ); then📝 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
| if SOURCE_ERR=$( (set -e; set +u; source "${INSTANCE_ENV}") 2>&1 >/dev/null ); then | |
| check_pass "instance.env sources cleanly" | |
| else | |
| check_fail "instance.env does not source cleanly: ${SOURCE_ERR}" \ | |
| "fix the shell syntax in ${CONFIG_DIR}/instance.env" | |
| if SOURCE_ERR=$( | |
| { | |
| (set -e; set +u; source "${INSTANCE_ENV}") >/dev/null | |
| } 2>&1 | |
| ); then | |
| check_pass "instance.env sources cleanly" | |
| else | |
| check_fail "instance.env does not source cleanly: ${SOURCE_ERR}" \ | |
| "fix the shell syntax in ${CONFIG_DIR}/instance.env" |
🤖 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/doctor.sh` around lines 243 - 247, The
instance.env sourcing check in doctor.sh is capturing the error output
incorrectly, leaving SOURCE_ERR empty in the failure path. Update the source
command inside the if block so stderr is captured into SOURCE_ERR before any
output is redirected away, and keep the existing check_pass/check_fail flow
intact. Use the surrounding source "${INSTANCE_ENV}" validation logic to locate
the fix and ensure the [FAIL] message includes the actual parse error text.
lucaconsalvi
reviewed
Jul 9, 2026
lucaconsalvi
left a comment
lucaconsalvi
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this one, Pablo! The make doctor idea is excellent — a single preflight that tells you exactly what's wrong and how to fix it is the kind of thing that saves everyone time. The config-folder centralization + sync mechanism is a clean design (the newer-wins timestamp logic, the empty-file guard, the manifest-driven approach). And the ansible-lint containerized runner is a nice addition to make verify.
I found a few issues — one will crash at runtime, a couple could hide errors in the diagnostic tool itself — plus some smaller suggestions. Details are inline below.
Summary: 2 critical, 3 high, 4 medium findings.
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | Critical | helpers/keep-instance.sh |
Sources old path that no longer exists after rename — make keep-instance will crash |
| 2 | Critical | doctor.sh |
Claims "read-only" but triggers sync_config_files() via common.sh |
| 3 | High | doctor.sh |
2>/dev/null on source common.sh suppresses all stderr, not just the instance.env warning |
| 4 | High | deploy/common.sh |
cp exit code unchecked in sync_config_files() — prints "updating" even if copy fails |
| 5 | High | doctor.sh |
ansible-galaxy collection list 2>/dev/null hides diagnostics, causes false "missing" cascade |
| 6 | Medium | hack/ansible-lint.sh |
Unquoted $CONTAINER_ENGINE and $CONTAINER_IMAGE |
| 7 | Medium | doctor.sh |
AWS check conflates timeout error codes 125-127 with auth failures |
| 8 | Medium | doctor.sh |
CI token curl check labels all non-zero exits as "network failure" |
| 9 | Medium | hack/ansible-lint.sh |
*.yml glob with no nullglob — spurious failure in empty directory |
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>
- 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>
- 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>
fonta-rh
added a commit
to fonta-rh/two-node-toolbox-tnf
that referenced
this pull request
Jul 9, 2026
- Remove blanket 2>/dev/null when sourcing common.sh; use SKIP_INSTANCE_ENV_WARNING flag for targeted suppression - ansible-galaxy: capture stderr so config errors surface instead of cascading false "missing collection" failures - timeout exit codes ≥125: report as "could not run" not "credentials not working" - curl: include exit code in CI token check warning Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fonta-rh
added a commit
to fonta-rh/two-node-toolbox-tnf
that referenced
this pull request
Jul 9, 2026
- Remove blanket 2>/dev/null when sourcing common.sh; use SKIP_INSTANCE_ENV_WARNING flag for targeted suppression - ansible-galaxy: capture stderr so config errors surface instead of cascading false "missing collection" failures - timeout exit codes ≥125: report as "could not run" not "credentials not working" - curl: include exit code in CI token check warning Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…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>
Read-only preflight that validates the full deployment configuration and prints a concrete fix command for every failure: required tools, SSH key, Ansible collections, inventory.ini, instance.env plus live AWS credentials, dev-scripts topology configs and pull secret, CI registry auth cross-check with live CI token check, and the kcli pull secret. Exits non-zero if and only if a check failed. - 'make doctor' runs common checks plus every auto-detected section - 'make doctor <cluster-type>' validates strictly for that deployment: the method's section becomes mandatory and its method-specific warnings are promoted to failures; cluster-type goals following 'doctor' are neutralized so they never trigger a deployment - inventory.ini stays informational when absent (the AWS flow generates it via 'make inventory'), but a present-but-unedited copy of the sample fails, since that breaks deployment in confusing ways - deploy-cluster.sh preflight error paths now point at 'make doctor' Checks are ported from the validation steps in .claude/commands/setup.md and the in-playbook preflight in roles/dev-scripts/install-dev/tasks/config.yml. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSBffYjh3rtLToAMR1pjQT
After the config-folder PR moved common.sh from deploy/aws-hypervisor/scripts/ to deploy/, doctor.sh referenced the old path and would fail on startup. Fix the source path and replace msg_err with an inline printf so the summary works even when common.sh fails to load. All fix hints now point to the central config/ folder instead of the deep canonical paths, consistent with the config-folder workflow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Doctor sources common.sh which unconditionally runs sync_config_files, writing to the working tree. Add SKIP_CONFIG_SYNC guard so doctor honours its read-only contract. Also add a missing assisted installer config section so `make doctor fencing-assisted` validates vars/assisted.yml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove blanket 2>/dev/null when sourcing common.sh; use SKIP_INSTANCE_ENV_WARNING flag for targeted suppression - ansible-galaxy: capture stderr so config errors surface instead of cascading false "missing collection" failures - timeout exit codes ≥125: report as "could not run" not "credentials not working" - curl: include exit code in CI token check warning Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
config-folder's sync refactor moved the sync call from common.sh module scope to a Makefile prerequisite. SKIP_CONFIG_SYNC is now a no-op — remove it. Cluster-type sync prerequisites are scoped to the else branch so 'make doctor <type>' doesn't trigger them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Thanks for the thorough review — the findings were spot-on. Here's the status: Already addressed (in earlier commits, before your review):
Addressed in
New in this push — we went further and addressed the root cause of the read-only concern:
The make-doctor branch has been rebased onto the updated config-folder. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deploy/Makefile (1)
78-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify
.PHONYcoverage for new targets.Static analysis flags
$(VALID_CLUSTER_TYPES)andbaremetal-adopt baremetal-fencing-agentas not declared PHONY. Sincesync-configanddoctorare also new non-file targets, confirm they're added to the existing.PHONYlist (not shown in this diff) to avoid conflicts with any same-named files/directories.#!/bin/bash rg -n '^\.PHONY' -A2 deploy/Makefile🤖 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/Makefile` around lines 78 - 93, The new make targets in the deploy Makefile are missing or may be missing .PHONY declarations, which can cause file-name collisions and incorrect no-op behavior. Update the existing .PHONY list to include the new non-file targets referenced by the rules here, specifically sync-config, doctor, baremetal-adopt, baremetal-fencing-agent, and the VALID_CLUSTER_TYPES entries if they are intended as command targets. Verify the declarations alongside the target definitions so the make behavior stays consistent even if matching files or directories exist.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.
Nitpick comments:
In `@deploy/Makefile`:
- Around line 78-93: The new make targets in the deploy Makefile are missing or
may be missing .PHONY declarations, which can cause file-name collisions and
incorrect no-op behavior. Update the existing .PHONY list to include the new
non-file targets referenced by the rules here, specifically sync-config, doctor,
baremetal-adopt, baremetal-fencing-agent, and the VALID_CLUSTER_TYPES entries if
they are intended as command targets. Verify the declarations alongside the
target definitions so the make behavior stays consistent even if matching files
or directories exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ea099182-e229-4fce-ab18-21f1181be307
📒 Files selected for processing (15)
.ansible-lint.claude/commands/setup.mdCLAUDE.mdREADME.mdconfig/README.mddeploy/Makefiledeploy/README.mddeploy/aws-hypervisor/README.mddeploy/common.shdeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/deploy-baremetal.shdeploy/openshift-clusters/scripts/deploy-cluster.shdeploy/openshift-clusters/scripts/doctor.shhack/ansible-lint.shhelpers/keep-instance.sh
✅ Files skipped from review due to trivial changes (4)
- deploy/aws-hypervisor/README.md
- CLAUDE.md
- deploy/README.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- helpers/keep-instance.sh
- .ansible-lint
- deploy/openshift-clusters/scripts/deploy-baremetal.sh
- deploy/openshift-clusters/scripts/baremetal-adopt.sh
- config/README.md
- hack/ansible-lint.sh
- deploy/openshift-clusters/scripts/deploy-cluster.sh
- .claude/commands/setup.md
- deploy/common.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
3 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
make doctor— a read-only preflight that validates required tools, SSH keys, Ansible collections, and every configuration file, printing a concrete fix command for each problem foundmake doctor <cluster-type>(e.g.make doctor fencing-kcli) promotes method-specific warnings to failuresDetails
doctor.shchecks:All fix hints point to the central
config/folder introduced by #92.Dependencies
Depends on #92 (config-folder). Based on that branch; once #92 merges, this PR's diff will reduce to just the doctor changes.
Test plan
make doctorwith no config files — all sections NOTE/WARN, exit 0make doctor fencing-kcli— kcli pull-secret promoted to FAIL, exit 1make doctor bogus-type— Makefile rejects with clear errormake help— doctor appears in help textshellcheck deploy/openshift-clusters/scripts/doctor.sh— clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
make doctorpreflight check, with optional stricter validation for specific cluster types.config/handling withmake sync-configto automatically propagate configuration to the locations used by deployments.ansible-lintcheck to the verification workflow (containerized execution with syntax checks).Bug Fixes
Documentation
config/locations and sync-first workflow, including pre-commit and lint/verify instructions.