Skip to content

Add /prepare skill for installation config generation#95

Draft
fonta-rh wants to merge 4 commits into
openshift-eng:mainfrom
fonta-rh:prepare-install-skill
Draft

Add /prepare skill for installation config generation#95
fonta-rh wants to merge 4 commits into
openshift-eng:mainfrom
fonta-rh:prepare-install-skill

Conversation

@fonta-rh

@fonta-rh fonta-rh commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds 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 validation
  • Adds helpers/prepare-config.sh — generates config/config_<topology>.sh 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 regex patterns
  • Adds hack/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
  • Adds .claude/commands/prepare.md — thin orchestrator skill that chains resolve → config → doctor → print deploy command (never deploys)

Test plan

  • hack/test-prepare-config.sh passes (238/238 offline)
  • shellcheck helpers/resolve-release-image.sh helpers/prepare-config.sh clean
  • Manual: helpers/resolve-release-image.sh --version 4.21 returns latest GA pullspec
  • Manual: helpers/resolve-release-image.sh --version 4.21-nightly returns CI registry pullspec
  • Manual: full /prepare tnf ipi aws 4.21 flow generates config and runs doctor

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added utilities to resolve OpenShift release images from version specs or pullspecs, with architecture-aware selection, optional digest pinning, and access validation.
    • Added config preparation that generates topology-specific dev-scripts configs with constraint checks, CI token handling, safe force/backup behavior, and Metal3 overrides.
    • Added a non-interactive /prepare flow that generates config and prints the deployment command without executing it.
  • Documentation

    • Expanded /prepare and helper usage docs, including examples, option matrices, and prerequisites.
  • Tests

    • Added offline and online validation test scripts, including a new Makefile test-prepare-online target.

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>
@openshift-ci

openshift-ci Bot commented Jul 15, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2026
@openshift-ci

openshift-ci Bot commented Jul 15, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2026
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Adds release-image resolution and dev-scripts configuration helpers, offline and online validation harnesses, a Makefile test target, and documentation for the non-deploying /prepare workflow.

Changes

Installation preparation tooling

Layer / File(s) Summary
Release image resolution
helpers/resolve-release-image.sh, helpers/README.md, hack/test-prepare-online.sh
Resolves nightly, stable, and explicit OpenShift releases, validates registry access, optionally returns digest-pinned pullspecs, and tests resolver behavior online.
Configuration generation and constraints
helpers/prepare-config.sh, hack/test-prepare-config.sh, hack/test-prepare-online.sh, Makefile
Generates topology-specific configuration, enforces architecture and IP-stack constraints, updates inventory overrides, self-checks output, and provides offline and online validation targets.
Prepare command contract and documentation
.claude/commands/prepare.md, .claude/commands/prepare/README.md, CLAUDE.md
Documents command arguments, prerequisites, execution sequencing, helper usage, validation, and printed deployment paths.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Operator
  participant PrepareCommand
  participant ResolveReleaseImage
  participant PrepareConfig
  participant MakeDoctor
  Operator->>PrepareCommand: provide preparation arguments
  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
Loading

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (3 errors, 2 warnings)

Check name Status Explanation Resolution
No-Sensitive-Data-In-Logs ❌ Error helpers/prepare-config.sh logs repo=${ds_repo}/branch=${ds_branch}, and helpers/resolve-release-image.sh logs registry/pullspec; this can expose internal hostnames. Remove raw values from stderr/info messages or guard them behind a debug flag; keep logs generic and sanitized.
No-Hardcoded-Secrets ❌ Error Test scripts hardcode token-like literals (ci_token="sha256~test_matrix_token_abc123", --ci-token "sha256~bogus_token_12345"), matching the secret heuristic. Replace those literals with runtime-generated dummy values or env-sourced placeholders, and avoid initializing token-named vars from string literals.
No-Injection-Vectors ❌ Error prepare-config writes CLI values into a shell file and then sources it; unescaped release_image/ci_token can become code execution. Escape/validate all interpolated values, or replace the source self-check with non-executing parsing so user input is never executed.
Docstring Coverage ⚠️ Warning Docstring coverage is 31.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Ai-Attribution ⚠️ Warning PR/commits mention Claude Code and use Co-Authored-By trailers; no Assisted-by or Generated-by Red Hat trailers are present. Replace AI-tool Co-Authored-By trailers with Assisted-by or Generated-by in the relevant commit messages/PR metadata.
✅ Passed checks (6 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Weak-Crypto ✅ Passed No weak crypto algorithms or custom crypto implementations were added; token checks are simple placeholder validation, not secret verification.
Container-Privileges ✅ Passed No changed file contains privileged/hostPID/hostNetwork/hostIPC/SYS_ADMIN/allowPrivilegeEscalation settings, and the PR touches only docs and shell scripts.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding a new /prepare skill for installation config generation.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Document credential setup and environment variables explicitly.

The helper documentation lists binaries but does not explain how CI_TOKEN and the pull secret are provisioned, where they are expected, or how /setup prepares 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 win

Add language identifiers to every changed fenced block.

The changed Markdown contains MD040 violations.

  • .claude/commands/prepare.md#L39-L44: use text.
  • .claude/commands/prepare.md#L128-L139: use bash for both command blocks.
  • .claude/commands/prepare/README.md#L7-L28: use text or bash as 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6c55fe5 and ad52f3a.

📒 Files selected for processing (7)
  • .claude/commands/prepare.md
  • .claude/commands/prepare/README.md
  • CLAUDE.md
  • hack/test-prepare-config.sh
  • helpers/README.md
  • helpers/prepare-config.sh
  • helpers/resolve-release-image.sh

Comment on lines +82 to +90
```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>")
```

@coderabbitai coderabbitai Bot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment on lines +107 to +109
$([ "<force>" = "true" ] && echo "--force") \
$([ -n "<ds_repo>" ] && echo "--ds-repo <ds_repo>") \
$([ -n "<ds_branch>" ] && echo "--ds-branch <ds_branch>")

@coderabbitai coderabbitai Bot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#!/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 -->

Comment thread .claude/commands/prepare.md
Comment thread .claude/commands/prepare/README.md
Comment thread .claude/commands/prepare/README.md
Comment thread helpers/prepare-config.sh Outdated
Comment thread helpers/prepare-config.sh Outdated
Comment thread helpers/resolve-release-image.sh Outdated
Comment thread helpers/resolve-release-image.sh
Comment thread helpers/resolve-release-image.sh
fonta-rh and others added 2 commits July 15, 2026 14:29
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>
@fonta-rh

Copy link
Copy Markdown
Contributor Author

Addressed CodeRabbit review-body and nitpick findings:

  • helpers/README.md:16-30 — Document credential setup: Won't fix — prerequisites already documented in options tables and --help output. Credential provisioning details belong in /setup docs, not duplicated here.
  • prepare.md:39-44 — MD040 language identifiers on fenced blocks: Won't fix — no markdownlint in CI pipeline (make verify runs shellcheck, yamlfmt, ansible-lint). The fenced blocks in the skill prompt file render for Claude, not a markdown viewer.

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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
hack/test-prepare-online.sh (1)

114-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Version 4.22 is hardcoded across ~10+ call sites.

Every Tier 1/Tier 2 test (T1.1-T1.12, E2E-1, E2E-2, E2E-4) hardcodes 4.22 as 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 derived 4.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

📥 Commits

Reviewing files that changed from the base of the PR and between 390e912 and 652baf0.

📒 Files selected for processing (2)
  • Makefile
  • hack/test-prepare-online.sh

Comment on lines +315 to +355
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"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant