Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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
|
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:
WalkthroughAdds release-image resolution and dev-scripts configuration helpers, offline and online validation harnesses, AWS-aware doctor checks, worktree configuration synchronization, a Makefile test target, and documentation for the non-deploying ChangesInstallation preparation tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant PrepareCommand
participant SyncWorktreeConfig
participant ResolveReleaseImage
participant PrepareConfig
participant MakeDoctor
Operator->>PrepareCommand: provide preparation arguments
PrepareCommand->>SyncWorktreeConfig: synchronize worktree configuration
PrepareCommand->>ResolveReleaseImage: resolve release image
ResolveReleaseImage-->>PrepareCommand: return pullspec
PrepareCommand->>PrepareConfig: generate configuration
PrepareConfig-->>PrepareCommand: return config path
PrepareCommand->>MakeDoctor: run read-only validation
MakeDoctor-->>PrepareCommand: return validation result
PrepareCommand-->>Operator: print deployment command
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helpers/README.md (1)
16-30: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument credential setup and environment variables explicitly.
The helper documentation lists binaries but does not explain how
CI_TOKENand the pull secret are provisioned, where they are expected, or how/setupprepares them. Add a concise prerequisites/setup section for these new tools.As per coding guidelines, README files must document prerequisites, setup, usage examples, configuration options, and environment variables.
Also applies to: 69-108
🤖 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 `@helpers/README.md` around lines 16 - 30, Expand the prerequisites/setup documentation in the README around the listed helper requirements to explicitly describe CI_TOKEN and pull-secret provisioning, their expected locations or configuration, and how /setup prepares them. Add concise usage examples, configuration options, and environment-variable details for the new tools while preserving the existing per-script prerequisite sections.Source: Coding guidelines
🧹 Nitpick comments (1)
.claude/commands/prepare.md (1)
39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to every changed fenced block.
The changed Markdown contains MD040 violations.
.claude/commands/prepare.md#L39-L44: usetext..claude/commands/prepare.md#L128-L139: usebashfor both command blocks..claude/commands/prepare/README.md#L7-L28: usetextorbashas appropriate.🤖 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/prepare.md around lines 39 - 44, Add language identifiers to every changed fenced Markdown block: use text for the prerequisites block in .claude/commands/prepare.md lines 39-44; use bash for both command blocks in .claude/commands/prepare.md lines 128-139; and use text or bash appropriately for each changed block in .claude/commands/prepare/README.md lines 7-28.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 @.claude/commands/prepare.md:
- Around line 82-90: Update the command template around
helpers/resolve-release-image.sh so the CI token is supplied through a protected
environment variable or stdin-based mechanism rather than interpolated into the
generated Bash command. Remove the <ci_token> placeholder from command arguments
and preserve conditional token usage without exposing the credential in chat,
logs, code, or commits.
- Around line 107-109: Update the command argument construction around the
ds_repo and ds_branch substitutions to validate each value against the accepted
repository URL and branch grammar, then pass them as safely quoted argument
values rather than raw shell interpolation. Avoid eval and preserve the existing
optional-argument behavior; include a security review of command execution and
untrusted input handling for this .claude command change.
- Around line 117-131: Normalize the user-provided topology before constructing
either Make command in the preparation workflow. Map tna to arbiter and tnf to
fencing, while leaving supported topology values such as sno unchanged; use the
normalized value in both the Step 3 doctor command and Step 4 deployment
command.
In @.claude/commands/prepare/README.md:
- Around line 20-21: Replace the invalid OCI digest example in
.claude/commands/prepare/README.md lines 20-21 with a real digest or a
64-character hexadecimal sha256 placeholder. Apply the same valid-shape digest
guidance to the aarch64 agent example in helpers/README.md lines 81-84, keeping
both documentation examples consistent.
- Around line 7-28: Update the /prepare README to document prerequisites,
including pull-secret and CI-token requirements, relevant environment variables,
and the setup flow, while retaining usage examples and configuration guidance.
Add a text or bash language identifier to both fenced code blocks containing the
command syntax and examples to satisfy markdownlint MD040.
In `@hack/test-prepare-config.sh`:
- Around line 284-294: Update the missing-inventory test around the PREPARE
invocation to assert the specifically defined failure exit code instead of
accepting any nonzero status, and verify that the requested output file inv4.sh
is absent after failure. Keep the existing error-case coverage and pass/fail
structure while adding both assertions.
In `@helpers/prepare-config.sh`:
- Around line 390-394: The prepare-config flow currently copies the generated
config before handle_inventory succeeds. In helpers/prepare-config.sh, update
the sequence around handle_inventory so inventory changes are validated or
staged first, and only copy to OUTPUT and report success after it succeeds;
preserve propagation of the inventory failure status. In
hack/test-prepare-config.sh, update the relevant test to assert the exact
failure status and verify that inv4.sh is not created.
- Around line 30-69: The transform_config function inserts release_image,
ci_token, and metal3_tag into a sourced shell configuration without validation
or safe escaping. Strictly validate each value against its expected image,
token, or tag format before any sed or heredoc substitution, and shell-quote the
validated values wherever they are written, including the Metal3 override block
and related handling at the other affected call sites.
- Around line 77-114: Extend self_check to validate the exact generated
OPENSHIFT_RELEASE_IMAGE and IP_STACK values, and verify the expected inventory
changes produced by each transformation. Update upsert_ini_var to create the
[metal_machine:vars] section when absent, then confirm insertion or replacement
succeeds before reporting success.
- Around line 359-390: Harden the token-bearing files in the output flow around
the existing backup and final `cp` operations: apply a restrictive umask, ensure
the generated `${OUTPUT}` is written with mode 0600 rather than inheriting an
existing destination mode, and explicitly restrict `${OUTPUT}.bak` to 0600 after
creation. Preserve the current force/backup and generation behavior.
In `@helpers/resolve-release-image.sh`:
- Line 34: Add a consistent curl total timeout via --max-time to the fetch_url
helper and every direct curl invocation in the registry/release API request
paths, while retaining the existing connection timeout and other options.
- Around line 158-167: Update registry_head_digest and its caller to preserve
the HTTP/curl failure status instead of collapsing all failures to exit code 1.
Return or propagate status 5 only for 401/403 responses, and use status 4 for
all other digest lookup failures, including curl errors, while retaining the
successful digest behavior.
- Around line 127-134: Keep CI_TOKEN out of command-line arguments and generated
configuration: in helpers/resolve-release-image.sh ranges 127-134, 146-156,
474-480, 507-508, and 543-545, obtain the token via stdin, an external secret
file, or CI_TOKEN and provide it to curl through a header or temporary config
file; in helpers/prepare-config.sh ranges 181-185 and 241-243, stop writing
CI_TOKEN into config/config_*.sh, source it from an external secret at runtime,
and update the related help and examples.
---
Outside diff comments:
In `@helpers/README.md`:
- Around line 16-30: Expand the prerequisites/setup documentation in the README
around the listed helper requirements to explicitly describe CI_TOKEN and
pull-secret provisioning, their expected locations or configuration, and how
/setup prepares them. Add concise usage examples, configuration options, and
environment-variable details for the new tools while preserving the existing
per-script prerequisite sections.
---
Nitpick comments:
In @.claude/commands/prepare.md:
- Around line 39-44: Add language identifiers to every changed fenced Markdown
block: use text for the prerequisites block in .claude/commands/prepare.md lines
39-44; use bash for both command blocks in .claude/commands/prepare.md lines
128-139; and use text or bash appropriately for each changed block in
.claude/commands/prepare/README.md lines 7-28.
🪄 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: 6ce1327a-0350-4af7-a433-4f2e85ec0418
📒 Files selected for processing (7)
.claude/commands/prepare.md.claude/commands/prepare/README.mdCLAUDE.mdhack/test-prepare-config.shhelpers/README.mdhelpers/prepare-config.shhelpers/resolve-release-image.sh
| ```bash | ||
| helpers/resolve-release-image.sh \ | ||
| --version <spec> \ | ||
| --arch <arch> \ | ||
| $([ "<method>" = "agent" ] && echo "--digest") \ | ||
| --pull-secret config/pull-secret.json \ | ||
| --validate-access \ | ||
| $([ -n "<ci_token>" ] && echo "--ci-token <ci_token>") | ||
| ``` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not interpolate the CI token into the generated command.
The contract says never echo the token into chat, but these templates instruct the agent to substitute <ci_token> directly into Bash. Keep the credential in a protected environment/secret channel and avoid rendering its value in the command transcript; ideally support environment-variable or stdin-based token input in the helpers.
Based on learnings, secrets must use environment variables and never be included in code or commits.
As per path instructions, .claude command changes require security-focused review of credential handling.
Also applies to: 99-105
🤖 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/prepare.md around lines 82 - 90, Update the command
template around helpers/resolve-release-image.sh so the CI token is supplied
through a protected environment variable or stdin-based mechanism rather than
interpolated into the generated Bash command. Remove the <ci_token> placeholder
from command arguments and preserve conditional token usage without exposing the
credential in chat, logs, code, or commits.
Sources: Path instructions, Learnings
There was a problem hiding this comment.
Won't fix — this is a Claude skill prompt, not executable shell. The <ci_token> placeholders are substituted at runtime. The --ci-token CLI flag is the repo's established helper pattern, and the token is intentionally written to config files (the Ansible role greps for it).
There was a problem hiding this comment.
@fonta-rh, thanks for the clarification — that makes sense. Since .claude/commands/prepare.md is a prompt template consumed by the agent (not a shell script), and --ci-token is the established helper pattern with the token intentionally persisted to config/config_*.sh for the Ansible role to consume, the original concern doesn't apply here. Withdrawing this one.
✏️ Learnings added
Learnt from: fonta-rh
Repo: openshift-eng/two-node-toolbox PR: 95
File: .claude/commands/prepare.md:82-90
Timestamp: 2026-07-15T14:17:16.464Z
Learning: In the openshift-eng/two-node-toolbox repository, `.claude/commands/*.md` files (e.g., `.claude/commands/prepare.md`) are Claude skill prompt templates consumed by an AI agent at runtime, not executable shell scripts. Placeholders like `<ci_token>` in embedded bash snippets are substituted by the agent, not literally interpolated into a shell. The `--ci-token` CLI flag is the established pattern for helpers/resolve-release-image.sh and helpers/prepare-config.sh, and the CI token is intentionally written into generated `config/config_*.sh` files because the Ansible role greps for it there.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| $([ "<force>" = "true" ] && echo "--force") \ | ||
| $([ -n "<ds_repo>" ] && echo "--ds-repo <ds_repo>") \ | ||
| $([ -n "<ds_branch>" ] && echo "--ds-branch <ds_branch>") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Shell-quote and validate free-form repository inputs before execution.
ds_repo and ds_branch are user-controlled values inserted unquoted into shell command substitutions. A value containing shell metacharacters can change the command executed by the agent. Validate the URL/branch grammar and construct an argument vector without raw interpolation or eval.
As per path instructions, .claude command changes require security review of command execution and untrusted input handling.
🤖 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/prepare.md around lines 107 - 109, Update the command
argument construction around the ds_repo and ds_branch substitutions to validate
each value against the accepted repository URL and branch grammar, then pass
them as safely quoted argument values rather than raw shell interpolation. Avoid
eval and preserve the existing optional-argument behavior; include a security
review of command execution and untrusted input handling for this .claude
command change.
Source: Path instructions
There was a problem hiding this comment.
Won't fix — skill template placeholders are substituted by Claude, not by shell expansion. The helper script's argument parser handles quoting via positional shift, and sed delimiter | is safe for git URLs.
There was a problem hiding this comment.
#!/bin/bash
set -e
sed -n '1,160p' .claude/commands/prepare.md
echo "-----"
fd -e sh . helpers
echo "-----"
cat -n helpers/prepare-config.sh | sed -n '1,80p'
</shell>
<!-- This is an auto-generated reply by CodeRabbit -->
|
Addressed CodeRabbit review-body and nitpick findings:
|
Auto-applied: - helpers/resolve-release-image.sh:34: add --max-time 120 to fetch_url Accepted after review: - .claude/commands/prepare.md:117: normalize topology aliases before Make commands - helpers/prepare-config.sh:366,390: harden permissions on token-bearing config files Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/test-prepare-online.sh (1)
114-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVersion
4.22is hardcoded across ~10+ call sites.Every Tier 1/Tier 2 test (T1.1-T1.12, E2E-1, E2E-2, E2E-4) hardcodes
4.22as the literal version. When this needs to roll forward, every site must be edited individually.♻️ Proposed fix: extract shared test version constant
readonly RESOLVE="${REPO_ROOT}/helpers/resolve-release-image.sh" readonly PREPARE="${REPO_ROOT}/helpers/prepare-config.sh" readonly PULL_SECRET="${REPO_ROOT}/config/pull-secret.json" readonly INVENTORY="${REPO_ROOT}/inventory.ini" +readonly TEST_VERSION="${TEST_VERSION:-4.22}"Then replace each literal
4.22(and derived4.22-prerelease,4.22-nightly) with"$TEST_VERSION"/"${TEST_VERSION}-nightly"etc.🤖 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/test-prepare-online.sh` around lines 114 - 151, Define a shared TEST_VERSION constant in the test setup and replace every hardcoded 4.22 across test_resolver_ga and the other Tier 1/Tier 2 and E2E test call sites with "$TEST_VERSION". Update derived prerelease and nightly references to use the corresponding TEST_VERSION-based forms, preserving all existing assertions and expected behaviors.
🤖 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 `@hack/test-prepare-online.sh`:
- Around line 315-355: Make test_e2e_inventory_fork register a temporary
EXIT/interrupt-safe restore trap immediately after creating inv_backup and
before either $PREPARE invocation. Have the trap restore $INVENTORY from
inv_backup, preserve the existing TMPDIR_BASE cleanup behavior, and remove or
disable the temporary trap after the normal final restore so it does not affect
later tests.
---
Nitpick comments:
In `@hack/test-prepare-online.sh`:
- Around line 114-151: Define a shared TEST_VERSION constant in the test setup
and replace every hardcoded 4.22 across test_resolver_ga and the other Tier
1/Tier 2 and E2E test call sites with "$TEST_VERSION". Update derived prerelease
and nightly references to use the corresponding TEST_VERSION-based forms,
preserving all existing assertions and expected behaviors.
🪄 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: 04f5518b-8ea4-40ac-b040-412b634bd25f
📒 Files selected for processing (2)
Makefilehack/test-prepare-online.sh
| test_e2e_inventory_fork() { | ||
| echo "" | ||
| echo "=== Tier 2: Inventory fork override (E2E-4) ===" | ||
|
|
||
| local inv_backup="${TMPDIR_BASE}/inventory.ini.bak" | ||
| cp "$INVENTORY" "$inv_backup" | ||
|
|
||
| local img rc | ||
| img=$("$RESOLVE" --version 4.22 --quiet 2>/dev/null) || { | ||
| fail "E2E-4 setup: resolve failed" | ||
| cp "$inv_backup" "$INVENTORY" | ||
| return | ||
| } | ||
|
|
||
| # E2E-4a: --ds-repo + --ds-branch → upserts both | ||
| rc=0 | ||
| "$PREPARE" --topology fencing --method ipi \ | ||
| --release-image "$img" --ci-token "${CI_TOKEN:-placeholder}" \ | ||
| --inventory "$INVENTORY" \ | ||
| --ds-repo https://github.com/example/dev-scripts --ds-branch test-branch \ | ||
| --output "${TMPDIR_BASE}/inv_fork1.sh" --force >/dev/null 2>&1 || rc=$? | ||
| assert_exit 0 "$rc" "E2E-4a fork upsert" | ||
| [[ "$rc" -eq 0 ]] && { | ||
| assert_grep "$INVENTORY" '^dev_scripts_src_repo=https://github.com/example/dev-scripts' "E2E-4a repo upserted" | ||
| assert_grep "$INVENTORY" '^dev_scripts_branch=test-branch' "E2E-4a branch upserted" | ||
| } | ||
|
|
||
| # E2E-4b: without fork args → purges | ||
| rc=0 | ||
| "$PREPARE" --topology fencing --method ipi \ | ||
| --release-image "$img" --ci-token "${CI_TOKEN:-placeholder}" \ | ||
| --inventory "$INVENTORY" \ | ||
| --output "${TMPDIR_BASE}/inv_fork2.sh" --force >/dev/null 2>&1 || rc=$? | ||
| assert_exit 0 "$rc" "E2E-4b fork purge" | ||
| [[ "$rc" -eq 0 ]] && { | ||
| assert_not_grep "$INVENTORY" '^dev_scripts_src_repo=' "E2E-4b repo purged" | ||
| assert_not_grep "$INVENTORY" '^dev_scripts_branch=' "E2E-4b branch purged" | ||
| } | ||
|
|
||
| cp "$inv_backup" "$INVENTORY" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Live inventory.ini mutation has no crash/interrupt safety net.
test_e2e_inventory_fork backs up the real $INVENTORY file into $TMPDIR_BASE, then mutates it twice via $PREPARE, and only restores it as the function's last statement (Line 354). The only registered EXIT trap (Line 31) is rm -rf "$TMPDIR_BASE", which deletes the backup itself. If the script is interrupted (Ctrl-C, hang, unexpected set -e exit) between the backup (Line 320) and the restore (Line 354), the user's live inventory.ini is left permanently in its test-mutated state with no way to recover the original content.
Register a trap-based restore before mutating so the original is always recovered on any exit path.
🔒 Proposed fix: trap-based inventory restore
PASS=0
FAIL=0
SKIP=0
TMPDIR_BASE=""
+INVENTORY_BACKUP=""
+restore_inventory() {
+ if [[ -n "$INVENTORY_BACKUP" && -f "$INVENTORY_BACKUP" ]]; then
+ cp "$INVENTORY_BACKUP" "$INVENTORY"
+ fi
+}
+
setup() {
TMPDIR_BASE=$(mktemp -d)
- trap 'rm -rf "$TMPDIR_BASE"' EXIT
+ trap 'restore_inventory; rm -rf "$TMPDIR_BASE"' EXIT
} local inv_backup="${TMPDIR_BASE}/inventory.ini.bak"
cp "$INVENTORY" "$inv_backup"
+ INVENTORY_BACKUP="$inv_backup"
...
cp "$inv_backup" "$INVENTORY"
+ INVENTORY_BACKUP=""
}🤖 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/test-prepare-online.sh` around lines 315 - 355, Make
test_e2e_inventory_fork register a temporary EXIT/interrupt-safe restore trap
immediately after creating inv_backup and before either $PREPARE invocation.
Have the trap restore $INVENTORY from inv_backup, preserve the existing
TMPDIR_BASE cleanup behavior, and remove or disable the temporary trap after the
normal final restore so it does not affect later tests.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Makefile`:
- Around line 4-6: Validate AWS immediately in deploy/Makefile so only 0 or 1 is
accepted and invalid or inherited values fail fast; update the AWS flag emission
near line 97 to emit --aws only for validated AWS=1, and update
.claude/commands/prepare.md lines 154-157 to invoke the AWS doctor path
explicitly with AWS=1.
In `@helpers/sync-worktree-config.sh`:
- Around line 70-73: Update the copy operation in the synchronization flow
around src and dest to ensure every synchronized credential file is assigned
restrictive 0600 permissions, rather than preserving potentially unsafe source
permissions. Keep the existing source selection and destination behavior
unchanged.
- Around line 68-73: Update the sync logic in sync-worktree-config.sh to refresh
existing destination config files when the corresponding source is newer, rather
than unconditionally skipping any existing $dest. Preserve local-only files by
copying only when the source has a newer modification time, using the existing
newer_of helper or equivalent mtime comparison before cp -p.
🪄 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: 649a41db-1edf-4cf4-ac57-c939f1b894ca
📒 Files selected for processing (4)
.claude/commands/prepare.mddeploy/Makefiledeploy/openshift-clusters/scripts/doctor.shhelpers/sync-worktree-config.sh
| # AWS hypervisor checks are on by default for doctor; opt out with AWS=0 | ||
| AWS ?= 1 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make AWS prerequisite enforcement explicit and fail closed.
The current contract can silently run the AWS preparation flow without AWS checks when AWS is inherited or set to an invalid value.
deploy/Makefile#L4-L6: reject values other than0and1.deploy/Makefile#L97-L97: ensure only validatedAWS=1emits--aws..claude/commands/prepare.md#L154-L157: invoke the AWS doctor path withAWS=1explicitly.
As per coding guidelines, configuration should be validated early and fail fast.
📍 Affects 2 files
deploy/Makefile#L4-L6(this comment)deploy/Makefile#L97-L97.claude/commands/prepare.md#L154-L157
🤖 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 4 - 6, Validate AWS immediately in
deploy/Makefile so only 0 or 1 is accepted and invalid or inherited values fail
fast; update the AWS flag emission near line 97 to emit --aws only for validated
AWS=1, and update .claude/commands/prepare.md lines 154-157 to invoke the AWS
doctor path explicitly with AWS=1.
Source: Coding guidelines
| [[ -f "$dest" ]] && continue | ||
|
|
||
| src="$(newer_of "${MAIN_CHECKOUT}/config/${name}" "${MAIN_CHECKOUT}/${canonical}")" | ||
| [[ -n "$src" ]] || continue | ||
|
|
||
| cp -p "$src" "$dest" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
printf '--- helpers/sync-worktree-config.sh (outline) ---\n'
ast-grep outline helpers/sync-worktree-config.sh --view expanded || true
printf '\n--- helpers/sync-worktree-config.sh (relevant lines) ---\n'
cat -n helpers/sync-worktree-config.sh | sed -n '1,220p'
printf '\n--- search for newer_of definition/usages ---\n'
rg -n "newer_of|sync-worktree-config|pull-secret\.json|instance\.env" -S .Repository: openshift-eng/two-node-toolbox
Length of output: 259
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo '--- helpers/sync-worktree-config.sh (outline) ---'
ast-grep outline helpers/sync-worktree-config.sh --view expanded || true
echo
echo '--- helpers/sync-worktree-config.sh (relevant lines) ---'
cat -n helpers/sync-worktree-config.sh | sed -n '1,220p'
echo
echo '--- search for newer_of definition/usages ---'
rg -n "newer_of|sync-worktree-config|pull-secret\.json|instance\.env" -S .Repository: openshift-eng/two-node-toolbox
Length of output: 19294
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo '--- sync-worktree-config references ---'
rg -n "sync-worktree-config|sync-config|worktree" -S Makefile .github deploy helpers config README.md
echo
echo '--- docs around config sync ---'
sed -n '1,140p' config/README.md 2>/dev/null || true
echo
sed -n '1,120p' deploy/README.md 2>/dev/null || true
echo
sed -n '1,120p' deploy/aws-hypervisor/README.md 2>/dev/null || true
echo
sed -n '1,120p' deploy/openshift-clusters/README.md 2>/dev/null || trueRepository: openshift-eng/two-node-toolbox
Length of output: 5008
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo '--- deploy/Makefile sync-config section ---'
sed -n '70,110p' deploy/Makefile
echo
echo '--- helpers/prepare-config.sh references to sync-config ---'
sed -n '360,430p' helpers/prepare-config.sh
echo
echo '--- any worktree-specific config docs ---'
rg -n "worktree|immutable|local overrides|syncs? .*config" -S helpers deploy config README.mdRepository: openshift-eng/two-node-toolbox
Length of output: 6538
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo '--- deploy/common.sh sync_config_files implementation ---'
sed -n '1,120p' deploy/common.sh
echo
echo '--- any references to helpers/sync-worktree-config.sh or worktree config semantics ---'
rg -n "sync-worktree-config\.sh|worktree config|worktree.*config|config.*worktree" -S helpers deploy README.md configRepository: openshift-eng/two-node-toolbox
Length of output: 5070
Refresh existing worktree config files instead of skipping them. helpers/sync-worktree-config.sh:68-73 never updates a destination once it exists, so a worktree can keep stale pull-secret.json, instance.env, or config_*.sh even when the main checkout has newer copies. Compare source and destination mtimes, or document that worktree config is intentionally immutable/local-only.
🤖 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 `@helpers/sync-worktree-config.sh` around lines 68 - 73, Update the sync logic
in sync-worktree-config.sh to refresh existing destination config files when the
corresponding source is newer, rather than unconditionally skipping any existing
$dest. Preserve local-only files by copying only when the source has a newer
modification time, using the existing newer_of helper or equivalent mtime
comparison before cp -p.
| src="$(newer_of "${MAIN_CHECKOUT}/config/${name}" "${MAIN_CHECKOUT}/${canonical}")" | ||
| [[ -n "$src" ]] || continue | ||
|
|
||
| cp -p "$src" "$dest" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Force restrictive permissions on copied credentials.
cp -p preserves the source mode, so a group/world-readable file in the main checkout remains readable after copying. These files can contain pull secrets, AWS settings, or CI tokens. Use a restrictive mode such as 0600 for the synchronized files.
As per coding guidelines, secrets and credentials must be handled securely and not exposed through repository files or permissions.
🤖 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 `@helpers/sync-worktree-config.sh` around lines 70 - 73, Update the copy
operation in the synchronization flow around src and dest to ensure every
synchronized credential file is assigned restrictive 0600 permissions, rather
than preserving potentially unsafe source permissions. Keep the existing source
selection and destination behavior unchanged.
Source: Coding guidelines
Two helper scripts + Claude skill that automate the manual process of turning "TNF agent cluster on latest 4.21 nightly on ARM" into correctly written dev-scripts config files. - helpers/resolve-release-image.sh: resolves version specs (4.21-nightly, 4.20, 4.22-ec) to concrete pullspecs via release controller or quay.io tag API, with optional digest pinning and access validation - helpers/prepare-config.sh: generates config from example templates with constraint matrix enforcement (aarch64 IPv6 block, -multi tag block), CI_TOKEN/OPENSHIFT_CI handling, Metal3 overrides, and self-check against the Ansible role's expected patterns - hack/test-prepare-config.sh: offline test matrix (238 assertions) covering all topology/method/arch/stack combinations plus constraint and inventory fork handling - .claude/commands/prepare.md: thin orchestrator skill that chains resolve → config → doctor → print deploy command (never deploys) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Auto-applied: - helpers/resolve-release-image.sh:34: add --max-time 120 to fetch_url Accepted after review: - .claude/commands/prepare.md:117: normalize topology aliases before Make commands - helpers/prepare-config.sh:366,390: harden permissions on token-bearing config files Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two-tier test script (hack/test-prepare-online.sh) that validates resolve-release-image.sh and prepare-config.sh against live registries. Tier 1 runs on any dev machine with pull-secret; Tier 2 requires a configured AWS instance. Not wired into make verify (stays offline/CI-safe). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The /prepare skill accepted medium=aws but never ensured config/instance.env existed — config generation succeeded but deployment would fail without AWS hypervisor settings (REGION, AWS_PROFILE, EC2_INSTANCE_TYPE). Three fixes: - doctor.sh: add --aws flag with NEED_AWS severity promotion (NOTE→FAIL) - Makefile: default AWS=1 so make doctor requires instance.env (AWS=0 opts out) - prepare.md: add worktree config bootstrap (Step 1), instance.env template copy (Step 3), sync-config before doctor (Step 5), fix EC2_INSTANCE_TYPE variable name New helpers/sync-worktree-config.sh copies essential config files from the main checkout to a worktree's config/ folder, picking the newer of config/ and canonical locations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ef9d5a4 to
f0c2a11
Compare
|
/hold remove |
openshift-ci
Bot
added
do-not-merge/hold
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
| readonly RESOLVE="${REPO_ROOT}/helpers/resolve-release-image.sh" | ||
| readonly PREPARE="${REPO_ROOT}/helpers/prepare-config.sh" | ||
| readonly PULL_SECRET="${REPO_ROOT}/config/pull-secret.json" | ||
| readonly INVENTORY="${REPO_ROOT}/inventory.ini" |
There was a problem hiding this comment.
[P1] Point Tier 2 at the inventory created by the supported workflows. make inventory writes deploy/openshift-clusters/inventory.ini, and doctor reads that same path, but this constant checks a repository-root inventory.ini that the normal workflow never creates. As a result HAS_E2E_CONFIG remains false and every Tier 2 test is silently skipped. Please use ${REPO_ROOT}/deploy/openshift-clusters/inventory.ini here.
| local outfile="${TMPDIR_BASE}/config_fencing.sh" | ||
| rc=0 | ||
| "$PREPARE" --topology fencing --method ipi \ | ||
| --release-image "$resolved" --ci-token "${CI_TOKEN:-placeholder}" \ |
There was a problem hiding this comment.
[P1] Do not pass a value that prepare-config explicitly rejects. detect_capabilities allows Tier 2 to run when CI_TOKEN is unset, but this fallback becomes the literal placeholder, and prepare-config exits 2 for that exact value. Once the inventory-path bug is fixed, E2E-1, E2E-2, and E2E-4 will fail on an otherwise configured machine without CI_TOKEN exported. Either make CI_TOKEN a Tier 2 prerequisite and skip clearly, or reuse the token from the existing config as the production workflow does.
| if [[ "$rc" -eq 0 ]]; then | ||
| pass "E2E-3 make doctor fencing-ipi" | ||
| else | ||
| echo -e "${COLOR_YELLOW} NOTE${COLOR_CLEAR} E2E-3 make doctor exited $rc (instance-level issue, not counted as failure)" |
There was a problem hiding this comment.
[P1] Treat a failed doctor gate as a test failure. This branch deliberately avoids incrementing FAIL, so test-prepare-online.sh can return success even when make doctor reports configuration failures. If an unmet prerequisite should be optional, detect and skip Tier 2 before entering it; once this gate runs, a nonzero result should call fail so the target cannot produce a false green.
| test-prepare-online: | ||
| @./hack/test-prepare-online.sh $(ARGS) | ||
|
|
||
| verify: |
There was a problem hiding this comment.
[P2] Run the new offline regression matrix in an automated check. The PR adds 238 assertions for the core config transformation behavior, but verify still runs only shellcheck, yamlfmt, and ansible-lint, and the current PR checks do not execute hack/test-prepare-config.sh. A logic regression can therefore merge with green CI. Please add an offline test target to verify or wire the script into a dedicated Prow job; the online suite should remain separate.
lucaconsalvi
left a comment
lucaconsalvi
left a comment
There was a problem hiding this comment.
Thanks @fonta-rh! Really nice piece of work — the exit-code contracts staying consistent across the script, --help, helpers/README.md and prepare.md made this unusually pleasant to review, and the stdout/stderr discipline in the resolver (only the pullspec on stdout, every message on stderr) is the kind of thing that's easy to get wrong and you got right throughout.
I went through the whole diff plus the resolved CodeRabbit threads. Findings below as inline comments; a few notes on how to read them:
Three I'd treat as blocking, all of the "silently wrong, no error" variety:
--validate-accessreturns PASS with no credentials at all — verified anonymousHEADagainst quay returns 200, so the gate certifies configs that will fail at install time.prepare-config.shdeletesdev_scripts_src_repo=/dev_scripts_branch=from the user's realinventory.inion every run that omits--ds-repo— which is what/preparedoes.- The offline test suite inherits that same default and mutates the real inventory ~24 times per run.
Three revisit threads that were closed — not to relitigate, but because I found evidence that changes the premise, so flagging rather than staying quiet:
--max-time(#3587080046, closed as applied): still missing on 3 of the 4curlcalls on current head.--ci-tokenmandatory (#3587080053, withdrawn): the Ansible role usesdefault('', true)and gates everything onci_token | length > 0— it doesn't hard-fail without a token.self_check/upsert_ini_var(#3587080013, withdrawn): the offline matrix that justified the withdrawal isn't wired into anything, and[metal_machine:vars]isn't guaranteed by the inventory generator.
One theme worth a decision rather than a patch: both helpers are GNU-only (grep -oP, no-suffix sed -i, sort -V, base64 -d) and resolve-release-image.sh:152 faults under set -u on bash 3.2. The repo accommodates macOS in a few places, so it'd be good to settle whether it's supported and then either fix the portability or add an explicit guard — right now macOS users get a raw sed: error and an undocumented exit 1.
Highest-value single change: wire hack/test-prepare-config.sh into make verify. It's hermetic and CI-safe, but it has no Make target, while the one target that does exist (test-prepare-online) exits 0 without running anything when config/pull-secret.json is absent — i.e. always, in CI. Worth fixing the --inventory default first so it doesn't write to the real inventory on every CI run.
Everything else is a suggestion — happy to be argued out of any of them, and glad to pair on the resolver bits if it's easier.
| validate_access() { | ||
| local pullspec="$1" pull_secret="$2" ci_token="$3" | ||
|
|
||
| if command -v oc >/dev/null 2>&1 && [[ -f "$pull_secret" ]]; then |
There was a problem hiding this comment.
Blocking — --validate-access returns PASS when there are no credentials at all.
PULL_SECRET defaults to config/pull-secret.json at L570 and its existence is never checked. Here the [[ -f "$pull_secret" ]] guard just skips the oc path, then get_bearer_token at L448 is || true, so registry_head_digest runs anonymously. Verified against the live registry:
$ curl -sI -o /dev/null -w '%{http_code}' \
https://quay.io/v2/openshift-release-dev/ocp-release/manifests/4.22.0-multi
200
So for every quay-hosted payload (i.e. all -ga/-ec/-rc specs) the access gate passes with no pull secret and no token. /prepare passes --validate-access on every run, so the one check that is supposed to prove the image is pullable happily certifies configs that will fail later at install time. A missing or mistyped --pull-secret is indistinguishable from a working one.
Suggested fix: fail fast when the resolved PULL_SECRET path does not exist (unless a --ci-token was supplied for a CI-registry pullspec), and treat "no credentials were actually used" as a failed validation rather than a pass.
| upsert_ini_var "$inventory" "dev_scripts_src_repo" "$ds_repo" | ||
| upsert_ini_var "$inventory" "dev_scripts_branch" "$ds_branch" | ||
| msg_info "Set dev-scripts fork: repo=${ds_repo} branch=${ds_branch}" | ||
| elif [[ -f "$inventory" ]]; then |
There was a problem hiding this comment.
Blocking — this branch silently strips the developer's dev-scripts fork override on every run that omits --ds-repo.
--inventory defaults to the live deploy/openshift-clusters/inventory.ini (L349), and handle_inventory reaches this elif whenever --ds-repo is absent — which is the normal case. .claude/commands/prepare.md Step 4 never passes --inventory or --ds-repo, so simply bumping a release version with /prepare deletes dev_scripts_src_repo= / dev_scripts_branch= from the user's real inventory.
Failure mode: a developer pins a dev-scripts fork, later re-runs /prepare to move to a new nightly, and the next make redeploy-cluster silently builds upstream master. They then debug a fix that "didn't apply" — with nothing in the output pointing at the cause (the msg_info at L165 goes to stderr and /prepare doesn't surface it).
Purging is a destructive edit to user state; it shouldn't be the default for a command whose job is to generate a config. Suggest making it explicit (--clear-ds-repo), or at minimum skipping the whole inventory step when neither --ds-repo nor --ds-branch was passed.
| fi | ||
|
|
||
| local result | ||
| result=$(run_prepare "$label" \ |
There was a problem hiding this comment.
Blocking (same root cause as L155 of prepare-config.sh) — the offline suite mutates the developer's real inventory.ini.
run_prepare passes --output but never --inventory, so prepare-config.sh:349 falls back to the live deploy/openshift-clusters/inventory.ini and the purge branch at prepare-config.sh:155-166 runs on it — roughly 24 times per suite run. The file header says "Runs without network access — validates config transforms against real examples", so nobody expects it to write outside $TMPDIR_BASE.
This is invisible in a fresh clone (only inventory.ini.sample exists), which is why it shipped. On any checkout where make inventory has been run — i.e. every real dev box, per the repo's own docs — running the test suite destroys the fork pinning. Two concurrent runs also race on that one file.
Everything else about the hygiene here is right (mktemp -d + trap at L23-26, all outputs under the temp dir, nothing touching $HOME); this is a single missing flag, not a pattern. One-line fix:
"$PREPARE" --output "$outfile" --inventory "${TMPDIR_BASE}/inventory.ini" "$@" >/dev/null 2>&1 || rc=$?and the same on the explicit invocation at L111-114.
| fi | ||
|
|
||
| local headers realm service | ||
| headers=$(curl -sI --connect-timeout 10 "https://${registry}/v2/" 2>/dev/null) || true |
There was a problem hiding this comment.
--max-time is still missing on 3 of the 4 curl calls.
Thread #3587080046 was closed as "Applied — fixed in 390e912" and CodeRabbit confirmed the timeout was "now applied consistently." On current head f0c2a11, only fetch_url (L34) has it:
$ grep -n 'curl ' helpers/resolve-release-image.sh
34: fetch_url() { curl -fsS --connect-timeout 30 --max-time 120 "$@"; }
122: headers=$(curl -sI --connect-timeout 10 ...)
133: token_json=$(curl -fsS --connect-timeout 10 ...)
152: http_code=$(curl -sI --connect-timeout 30 ...)
$ grep -n 'max-time' helpers/resolve-release-image.sh
34: ...
--connect-timeout only bounds the TCP handshake; a registry that accepts the connection and then stalls hangs /prepare indefinitely. Not flagging the reply as wrong on purpose — just that the fix landed in one place and the thread got closed on all four. This line also lacks -f.
| case "$http_code" in | ||
| 200) echo "$digest"; return 0 ;; | ||
| 401|403) return 1 ;; | ||
| *) return 1 ;; |
There was a problem hiding this comment.
404, 429, 5xx and connect failures are all reported to the user as "Access denied — refresh your CI token."
Re-raising #3587080059 (withdrawn as stylistic) because the user-visible impact is concrete, not cosmetic. Both case arms return 1, so resolve_digest (L418-422) and validate_access (L451-453) map every outcome to exit 5 + access_hint. Verified against the live registry:
$ curl -sI -o /dev/null -w '%{http_code}' \
https://quay.io/v2/openshift-release-dev/ocp-release/manifests/4.22.0-nope
404
So a version typo produces Access denied ... Refresh CI token / Check pull-secret.json. .claude/commands/prepare.md:105 then maps exit 5 to "point the user at /setup for credentials" — the agent sends them off to regenerate a CI token for a problem that is a mistyped version. A stalled network (http_code="000", L156) reads the same way.
Distinguishing the arms is a few lines and turns a misleading error into an actionable one:
case "$http_code" in
200) echo "$digest"; return 0 ;;
401|403) return 1 ;; # genuine auth failure
404) msg_err "Image not found: ${repo}:${reference}"; return 2 ;;
000) msg_err "Registry unreachable: ${registry}"; return 3 ;;
*) msg_err "Registry returned HTTP ${http_code}"; return 3 ;;
esac| @./helpers/resource-agents-build/local-build-test.sh $(ARGS) | ||
|
|
||
| test-prepare-online: | ||
| @./hack/test-prepare-online.sh $(ARGS) |
There was a problem hiding this comment.
The offline suite is the CI-safe one, but only the network-bound suite got a target.
hack/test-prepare-config.sh is hermetic, needs no credentials, and is exactly what CI should run — yet it's referenced nowhere outside its own file (no Make target, not in AGENTS.md, helpers/README.md, or prepare.md). verify: below still runs only shellcheck, yamlfmt and ansible-lint, and make test-prepare-online self-skips to green without a pull secret.
This is the highest-value single change in the PR, and it would also have caught the unguarded "$PREPARE" calls at hack/test-prepare-config.sh:262/270/278:
test-prepare-config:
@./hack/test-prepare-config.sh $(ARGS)
verify:
VALIDATE_ONLY=true $(MAKE) shellcheck
...
$(MAKE) test-prepare-config(worth landing the --inventory fix first — as-is it would write to the real inventory on every CI run.)
| INV | ||
|
|
||
| # --ds-repo + --ds-branch → upserts both | ||
| "$PREPARE" --topology fencing --method ipi --release-image "$img" \ |
There was a problem hiding this comment.
Three unguarded invocations here can kill the suite before it prints a verdict.
L262, L270 and L278 call "$PREPARE" without || rc=$? or || true. Under set -euo pipefail (L4), a non-zero exit from any of them terminates the script — skipping the remaining tests, the Results: N passed, M failed line (L306-309) and the [[ "$FAIL" -eq 0 ]] verdict (L311). The status that escapes is prepare-config.sh's own, not a test result.
These three succeed on Linux today, so the suite does complete — this is about the failure mode, not current behaviour. But it's the wrong one for a test harness: a regression in handle_inventory would produce a bare non-zero exit with no indication of which assertions passed, and CI would show a failure with no diagnosable output. Appending || rc=$? and asserting the expected status also turns three silent preconditions into real assertions.
|
|
||
| **medium=external:** | ||
| ```bash | ||
| cd deploy && make doctor AWS=0 <topology>-<method> |
There was a problem hiding this comment.
The external-hypervisor branch omits make sync-config, so doctor checks a stale path and reports a false FAIL.
The AWS branch at L156 runs make sync-config && make doctor ..., but this one doesn't. doctor: has no sync-config prerequisite in deploy/Makefile, and doctor.sh:321-323 reads the canonical location:
CONFIG_FILE="${DS_FILES_DIR}/config_${topology}.sh"
if [[ ! -f "${CONFIG_FILE}" ]]; then
... check_fail "config_${topology}.sh missing (required for the requested deployment)"Step 4 writes to config/config_<topology>.sh, and only sync-config fans that out to DS_FILES_DIR. So on an external hypervisor, a /prepare run that just succeeded is immediately followed by doctor reporting the config as missing — and check_fail means a non-zero exit, which Step 5 treats as a blocker.
L164 also describes sync-config as propagating only config/instance.env; it propagates the config_*.sh files too, which is precisely why this branch needs it. Adding make sync-config && here fixes both.
| canonical="${entry#*:}" | ||
| dest="${REPO_ROOT}/config/${name}" | ||
|
|
||
| [[ -f "$dest" ]] && continue |
There was a problem hiding this comment.
Worth a comment: this is the dominant rule, and it means a worktree config is never refreshed.
Any existing config/<name> short-circuits the sync, so the newer_of logic below only ever applies on first population. That's a defensible policy (don't clobber worktree-local edits), but it reads as a guard clause rather than the main behaviour — and it means a rotated pull secret in the main checkout never reaches an existing worktree. A one-line comment saying so would save the next reader the trace.
Three smaller things in this file:
- L33:
git rev-parse --git-dir 2>/dev/nullunderset -e— outside a git repo this exits 128 with no message at all. - L41:
MAIN_CHECKOUTis never checked non-empty before being used in the-dtest at L43;head -1in apipefailpipeline is also a SIGPIPE hazard. - L78:
IFS=', ' msg_ok "...${COPIED[*]}"— a var assignment prefixed to a function call doesn't affect[*]expansion in the already-evaluated argument, so this prints space-separated, not comma-separated. SetIFSon its own line (in a subshell or with a save/restore) if the comma matters.
| # Valid cluster types for 'make deploy <cluster-type>' | ||
| VALID_CLUSTER_TYPES := fencing-ipi fencing-agent fencing-assisted arbiter-ipi arbiter-agent arbiter-kcli fencing-kcli sno-ipi sno-agent | ||
|
|
||
| # AWS hypervisor checks are on by default for doctor; opt out with AWS=0 |
There was a problem hiding this comment.
AWS ?= 1 changes make doctor from passing to failing for every non-AWS user.
make doctor is documented in README.md:83 and deploy/README.md:43 as the general read-only preflight. With this default it always passes --aws, which promotes missing instance.env from a note to check_fail — and doctor.sh:451-453 exits 1 on any failure. Anyone on a baremetal or libvirt hypervisor who runs the documented command now gets a hard failure for a file they correctly don't have.
The AWS=0 escape hatch exists but is documented only inside .claude/commands/prepare.md:161, which non-/prepare users never read.
The comment on L3 is accurate about the mechanism, so this may well be deliberate — but as the default it's a behaviour change for existing users. Options, roughly in order of preference: default to 0 and have /prepare pass AWS=1 explicitly; or auto-detect (require instance.env only when deploy/aws-hypervisor/instance.env or an AWS inventory is present); or keep the default and document AWS=0 in both READMEs.
2 participants
Summary
helpers/resolve-release-image.sh— resolves version specs (4.21-nightly,4.20,4.22-ec) to concrete pullspecs via the release controller (nightlies) or quay.io tag API (GA/EC/RC), with optional digest pinning and access validationhelpers/prepare-config.sh— generatesconfig/config_<topology>.shfrom example templates with constraint matrix enforcement (aarch64 IPv6 block,-multitag block), CI_TOKEN/OPENSHIFT_CI handling, Metal3 overrides, and self-check against the Ansible role's regex patternshack/test-prepare-config.sh— offline test matrix (238 assertions) covering all {arbiter,fencing,sno} × {ipi,agent} × {x86_64,aarch64} × {v4,v6,v4v6} combinations plus constraint and inventory fork handling.claude/commands/prepare.md— thin orchestrator skill that chains resolve → config → doctor → print deploy command (never deploys)Test plan
hack/test-prepare-config.shpasses (238/238 offline)shellcheck helpers/resolve-release-image.sh helpers/prepare-config.shcleanhelpers/resolve-release-image.sh --version 4.21returns latest GA pullspechelpers/resolve-release-image.sh --version 4.21-nightlyreturns CI registry pullspec/prepare tnf ipi aws 4.21flow generates config and runs doctor🤖 Generated with Claude Code
Summary by CodeRabbit
/prepareflow that prepares configuration and prints the deployment command without executing it./prepareand helper usage docs with syntax, examples, and option/prerequisite details.test-prepare-onlineMakefile target.