SDCICD-1864 Add bonfire CLI wrapper for ephemeral ROSA HCP cluster lifecycle#3274
SDCICD-1864 Add bonfire CLI wrapper for ephemeral ROSA HCP cluster lifecycle#3274YiqinZhang wants to merge 1 commit into
Conversation
|
There are test jobs defined for this repository which are not configured to run automatically. Comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: YiqinZhang 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
|
/override ci/prow/hypershift-pr-check |
|
@YiqinZhang: Overrode contexts on behalf of YiqinZhang: ci/prow/hypershift-pr-check DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/common/ephemeral/bonfire.go (2)
56-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the
crc-bonfirepip package version.
pip install --quiet crc-bonfireinstalls whatever is currently latest, risking unreproducible/broken builds if a new release changes behavior.♻️ Suggested fix
- if _, err := c.cmd.Run(ctx, nil, "pip", "install", "--quiet", "crc-bonfire"); err != nil { + if _, err := c.cmd.Run(ctx, nil, "pip", "install", "--quiet", "crc-bonfire==<pinned-version>"); err != nil {🤖 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 `@pkg/common/ephemeral/bonfire.go` around lines 56 - 72, The InstallBonfire method currently installs crc-bonfire without a version pin, which makes builds non-reproducible. Update the pip install call in CLI.InstallBonfire to use a fixed crc-bonfire version or otherwise source the version from a constant/config so the installed package is stable across runs. Keep the existing LookPath checks and logging, and only change the package spec used in the c.cmd.Run invocation.
162-185: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider validating the parsed namespace format.
The heuristic returns the last non-log-prefixed line verbatim. A silent bonfire output-format change could yield a bogus "namespace" that later gets passed unchecked into
ReleaseNamespace/GetSecretValue. A light regex check (e.g.^ephemeral-[a-z0-9]+$) before returning would fail fast instead of propagating garbage.🤖 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 `@pkg/common/ephemeral/bonfire.go` around lines 162 - 185, The parseNamespace helper currently returns the last non-log-prefixed line without verifying it is actually a namespace, so add a lightweight format check before returning from parseNamespace. Validate the candidate string against the expected ephemeral namespace pattern (for example, the ephemeral- prefix plus lowercase alphanumerics) and return only if it matches; otherwise keep scanning or return empty so invalid output does not flow into ReleaseNamespace or GetSecretValue.
🤖 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 `@pkg/common/ephemeral/bonfire.go`:
- Line 89: The Bonfire kubeconfig setup is always forcing TLS verification off
via the insecure skip flag. Update the ephemeral cluster configuration in
bonfire.go so this setting is configurable instead of hardcoded, with the
default keeping TLS verification enabled; use the relevant bonfire/kubeconfig
construction path around the insecure skip TLS option to make the flag optional.
- Line 56: The logging in CLI.InstallBonfire and the related helper calls still
uses stdlib log, which violates the structured logging guideline. Add a
logr.Logger to CLI (or pass one into InstallBonfire) and replace each
log.Print/log.Printf usage in this file with logger.Info/logger.Error calls
using structured key/value fields instead of formatted strings. Keep the changes
localized to the CLI methods and any helpers they call so all logging in this
file goes through logr/klog.
- Around line 76-95: The Login method in CLI is exposing the bearer token by
passing it directly to oc login via argv. Replace this token-in-arguments flow
with a file-backed kubeconfig/authentication approach in Login so the secret is
not visible to local process inspection, and keep the existing validation and
logging around serverURL and successful authentication.
---
Nitpick comments:
In `@pkg/common/ephemeral/bonfire.go`:
- Around line 56-72: The InstallBonfire method currently installs crc-bonfire
without a version pin, which makes builds non-reproducible. Update the pip
install call in CLI.InstallBonfire to use a fixed crc-bonfire version or
otherwise source the version from a constant/config so the installed package is
stable across runs. Keep the existing LookPath checks and logging, and only
change the package spec used in the c.cmd.Run invocation.
- Around line 162-185: The parseNamespace helper currently returns the last
non-log-prefixed line without verifying it is actually a namespace, so add a
lightweight format check before returning from parseNamespace. Validate the
candidate string against the expected ephemeral namespace pattern (for example,
the ephemeral- prefix plus lowercase alphanumerics) and return only if it
matches; otherwise keep scanning or return empty so invalid output does not flow
into ReleaseNamespace or GetSecretValue.
🪄 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: 5ea6770d-9614-4012-b659-f213f85ef51c
📒 Files selected for processing (1)
pkg/common/ephemeral/bonfire.go
|
/override ci/prow/hypershift-pr-check |
|
@YiqinZhang: Overrode contexts on behalf of YiqinZhang: ci/prow/hypershift-pr-check DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@YiqinZhang: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
1 participant
Introduces pkg/common/ephemeral — a new package that wraps the bonfire CLI and oc for managing ephemeral ROSA HCP cluster namespaces on the eng-prod management cluster.
JIRA: https://issues.redhat.com/browse/SDCICD-1864
Co-authored-by: Cursor
Summary by CodeRabbit