HYPERFLEET-1346 - refactor: Update adapter deployments to use helm sdk - #154
HYPERFLEET-1346 - refactor: Update adapter deployments to use helm sdk#154ma-hill wants to merge 4 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces shell-based adapter deployment with Helm SDK installation and option-based cleanup. Shared Git clone helpers cache adapter and API chart paths. E2E tests use the new helpers, local test state, and the Adapter label. The suite initializes deferred clone configuration. New YAML fixtures cover adapter failures, resource discovery, API errors, consumer errors, crash recovery, and stuck deletion. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant Helper
participant HelmClient
participant Cluster
E2ETest->>Helper: CloneChartOnce()
Helper-->>E2ETest: Chart path
E2ETest->>Helper: InstallAdapter(options)
Helper->>HelmClient: InstallRelease(chart, values, labels)
HelmClient->>Cluster: Install adapter release
E2ETest->>Helper: UninstallAdapter(options)
Helper->>HelmClient: UninstallRelease(release)
HelmClient->>Cluster: Remove adapter release
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/adapter/adapter_failover.go (1)
50-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdapter cleanup is registered after the install assertion in both specs. Both files copy the same idiom: call
InstallAdapter, assert the error, then registerDeferCleanup. Gomega aborts the spec at the assertion, so on a partial install theDeferCleanupnever registers and the Helm release, cluster-scoped RBAC, and Pub/Sub topic/subscription leak for the remainder of the run. Both files also passerr.Error()as a format string toPrintfand re-assert the sameerrtwice.
e2e/adapter/adapter_failover.go#L50-L59: move theginkgo.DeferCleanupblock above theh.InstallAdaptercall, delete the duplicateExpectat Line 51, and change Line 55 toPrintf("Warning: failed to uninstall adapter %s: %v\n", adapterName, err).e2e/cluster/adapter_failure.go#L51-L59: apply the same reordering, delete the duplicateExpectat Line 52, and change Line 56 to a constant format string with%v. This spec isginkgo.Serialand subscribes to all events, so a leaked release corrupts later specs.🤖 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 `@e2e/adapter/adapter_failover.go` around lines 50 - 59, Register ginkgo.DeferCleanup before h.InstallAdapter in both e2e/adapter/adapter_failover.go#L50-L59 and e2e/cluster/adapter_failure.go#L51-L59 so partial installs are cleaned up even when the assertion aborts; remove the duplicate err assertion in each spec, and update each cleanup warning to use a constant format string with adapterName and %v for the uninstall error.Source: Path instructions
🧹 Nitpick comments (8)
testdata/adapter-configs/cl-m-wrong-nest.yaml (1)
277-396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe
post.payloadsblock is byte-identical totestdata/adapter-configs/cl-m-wrong-ds.yaml.Lines 277-406 here match Lines 277-406 of
testdata/adapter-configs/cl-m-wrong-ds.yamlexactly. Lines 122-262 also match, apart from the adapter name. The two fixtures differ only in the adapter name and in which discovery block is deliberately wrong.Any future edit to the
Applied/Available/HealthCEL expressions must be applied twice. A missed edit produces a fixture that fails for a reason unrelated to the scenario it tests.
clusters-base.tmplalready provides a shared-values mechanism, but it covers onlybroker,image,labels, andenv. Consider extending the merge to cover a sharedadapterTaskConfigbase for the maestro scenarios.🤖 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 `@testdata/adapter-configs/cl-m-wrong-nest.yaml` around lines 277 - 396, Deduplicate the shared post.payloads configuration between cl-m-wrong-nest.yaml and cl-m-wrong-ds.yaml by extending the shared-values merge in clusters-base.tmpl to support a common adapterTaskConfig base. Keep the shared Applied, Available, and Health CEL expressions in that base, while preserving each fixture’s distinct adapter name and deliberately incorrect discovery block.pkg/helper/adapter.go (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the commented-out struct and the stray note.
AdapterDeploymentstill exists as real code at lines 29-33. This commented copy plus the["name": "resourcetype"]note is dead weight in a reviewed file.🤖 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/helper/adapter.go` around lines 35 - 43, Remove the commented-out AdapterDeployment struct and the stray ["name": "resourcetype"] note near the active AdapterDeployment definition; leave the real AdapterDeployment implementation unchanged.pkg/helper/helm/helm.go (2)
132-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis comment now contradicts the code.
The comment states that future work can move Helm releases to the Helm SDK. This PR completed that move.
InstallReleaseat lines 95-130 uses the SDK. Remove the stale sentence.🤖 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/helper/helm/helm.go` around lines 132 - 133, Update the comment above UninstallRelease to remove the stale sentence about future migration to the Helm SDK, while retaining the accurate description of the current installation workflow.
93-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe doc comment describes a parameter that does not exist, and the timeout is a magic number.
Line 94 documents
fileValuesas a slice ofkey=filepathentries.InstallReleasehas no such parameter. Delete that line.
5 * time.Minuteat line 109 is a literal.AdapterDeploymentOptions.Timeoutalready exists inpkg/helper/adapter.goand is unused. Accept a timeout parameter, or promote the literal to a named constant.♻️ Proposed fix
+const defaultInstallTimeout = 5 * time.Minute + // InstallRelease installs a Helm chart from a local path with values from a template file -// fileValues is a slice of "key=filepath" entries that will be loaded and set as values (like --set-file) func (c *HelmClient) InstallRelease(ctx context.Context, releaseName string, chartPath string, releaseValues map[string]interface{}, labels map[string]string) error { @@ - installClient.Timeout = 5 * time.Minute + installClient.Timeout = defaultInstallTimeoutAs per coding guidelines: "Magic numbers/strings SHOULD be named constants" (QUAL-02).
🤖 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/helper/helm/helm.go` around lines 93 - 110, The InstallRelease documentation includes the nonexistent fileValues parameter, and its Helm timeout is hard-coded. Remove the fileValues comment, then replace 5 * time.Minute in HelmClient.InstallRelease with a named timeout constant; keep the existing InstallRelease behavior unchanged.Source: Path instructions
pkg/helper/git.go (2)
85-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNothing removes the cloned temp directory on the success path.
os.MkdirTempcreates a unique directory underWorkDiron every process. Each failure path removes it. The success path keeps it, and the previous caller-managed cleanup callback was deleted. Repeated suite runs accumulate full sparse checkouts under.test-workwith no upper bound.Add a suite-level teardown that removes the retained clone paths, or reuse a deterministic directory name per component and ref instead of a random one.
As per coding guidelines: "MUST clean up on ALL paths including early returns" (RES-02).
Also applies to: 152-156
🤖 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/helper/git.go` around lines 85 - 88, The successful clone path in the helper around os.MkdirTemp leaks the generated componentDir, while only failure paths clean it up. Restore caller-managed cleanup or add suite-level teardown that tracks every retained clone path and removes it after each suite, including the paths involved in the later success return around lines 152–156; preserve cleanup on all early-return paths.Source: Path instructions
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe field is
RepoPath, but the doc comment, the validation error, and the#nosecnote all still sayChartPath.Line 29 documents a field named
ChartPath. Line 69 returns"ChartPath is required". Line 130 and line 149 also referenceChartPath. A caller that reads the error message will look for a struct field that no longer exists.♻️ Proposed fix
- // ChartPath is the path within the repository to the chart directory + // RepoPath is the path within the repository to the chart directory // This will be used for sparse checkout to minimize download size RepoPath string @@ if opts.RepoPath == "" { - return "", fmt.Errorf("ChartPath is required") + return "", fmt.Errorf("repoPath is required") }Also applies to: 68-70
🤖 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/helper/git.go` around lines 28 - 30, Rename the stale ChartPath references to RepoPath throughout the relevant struct documentation, validation error, and `#nosec` comments, including the field comment near RepoPath and the checks around the validation logic. Keep the existing validation behavior unchanged while ensuring all user-facing and security annotations identify the actual RepoPath field.pkg/e2e/suite.go (1)
69-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse shared constants for clone options
".test-work"matcheshelper.TestWorkDir, so no directory override occurs. Usehelper.TestWorkDirin both fields andlabels.Adapterfor the component. Correct the stale"./test-work"documentation inHelmChartCloneOptions.🤖 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/e2e/suite.go` around lines 69 - 87, Update both HelmChartCloneOptions initializations to use helper.TestWorkDir for WorkDir, and replace the adapter component string with labels.Adapter (using the corresponding shared label for the API clone). Also update the stale WorkDir documentation in HelmChartCloneOptions to reference the shared test-work directory.Source: Path instructions
pkg/helper/cleanup.go (1)
97-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep Helm cleanup within the sweep deadline.
action.Uninstall.Runin Helm v3.21.0 accepts no context.NewUninstalldefaults toWait=falseandTimeout=0, soDeletionPropagation="foreground"does not make Helm wait for deletion, and settingTimeoutalone does not bound this path. Add context-aware cancellation or a bounded client timeout, and pass the remaining deadline to each release (CWE-400).🤖 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/helper/cleanup.go` around lines 97 - 110, Update the cleanup flow around helmClient.UninstallRelease so Helm uninstall operations are bounded by the existing five-minute context deadline. Add context-aware cancellation or configure a bounded client timeout, and ensure each release receives only the remaining deadline rather than an unbounded wait. Preserve the existing release iteration and error handling behavior.Source: Path instructions
🤖 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 `@e2e/adapter/adapter_with_maestro.go`:
- Around line 465-473: Update the cleanup blocks in
e2e/adapter/adapter_with_maestro.go at 465-473, 620-624, 812-816, and 980-984 to
use the fixed formatted warning with deployOpts.AdapterName and err, and remove
each duplicate install assertion; apply the same Printf fix and
duplicate-assertion removal in e2e/cluster/crash_recovery.go at 55-63 and
e2e/cluster/stuck_deletion.go at 55-63.
In `@e2e/cluster/crash_recovery.go`:
- Around line 32-34: Update the API chart clone log in
e2e/cluster/crash_recovery.go lines 32-34 and e2e/cluster/stuck_deletion.go
lines 33-35 to pass apiPath instead of path to GinkgoWriter.Printf after
CloneChartOnce assigns apiPath; leave the adapter chart logging unchanged.
In `@pkg/helper/adapter.go`:
- Around line 44-53: Update AdapterDeploymentOptions and the InstallAdapter flow
so Namespace, Timeout, and SetValues are either fully honored by the Helm client
and InstallRelease call or removed from the public options struct; do not leave
fields that callers can set but the implementation ignores. If retaining them,
use opts.Namespace for the Helm client, opts.Timeout for the release timeout,
and opts.SetValues for Helm values, while ensuring any constructor initializes
these fields consistently.
- Around line 157-177: Update Helper.UninstallAdapter to aggregate the uninstall
and resource-cleanup failures with errors.Join while preserving each original
cause for inspection, and wrap the returned aggregate with operation context
rather than returning a bare opaque error. Replace both
logger.Error(err.Error()) calls with structured logging that passes the error
under the "error" attribute, and remove the unticketed refactor comment above
the method.
In `@pkg/helper/git.go`:
- Around line 14-17: The exported variable ApiGitClone should use consistent
acronym casing as APIGitClone; rename it and update all references, including
pkg/e2e/suite.go, e2e/cluster, and e2e/adapter. In NewGitClone, rename the
parameter HelmChartCloneOptions to opts and update its uses without changing
behavior.
In `@pkg/helper/helm/helm.go`:
- Around line 147-149: Update the result logging after the uninstall operation
to guard both result and result.Release before dereferencing, and log only the
release name and revision fields. Remove the full Release object from
logger.Info to prevent configuration, values, manifest, and credentials from
entering test logs.
In `@testdata/adapter-configs/cl-m-bad-api.yaml`:
- Around line 11-14: Disable merged-configuration debugging by setting
debug_config to false in testdata/adapter-configs/cl-m-bad-api.yaml lines 11-14
and testdata/adapter-configs/cl-m-unreg-consumer.yaml lines 13-16; update both
fixtures without changing their other logging settings.
In `@testdata/adapter-configs/cl-param-error.yaml`:
- Around line 60-72: Correct the comments in the fixture to identify the
params-phase failure rather than a nonexistent precondition failure, and update
the clusterNamespace manifest’s name template to apply the established
lowercasing filter to clusterId, matching the other adapter fixtures.
In `@testdata/adapter-configs/clusters-base.tmpl`:
- Around line 11-20: Update the RabbitMQ configuration in the template to avoid
rendering credentials through .RabbitmqUrl: use a Kubernetes Secret reference
for the broker URL or its credential fields, and ensure the resulting ConfigMap
and Helm values contain only non-sensitive references. Also update the related
redaction path for Clients.Broker so debug_config output cannot expose AMQP
credentials.
---
Outside diff comments:
In `@e2e/adapter/adapter_failover.go`:
- Around line 50-59: Register ginkgo.DeferCleanup before h.InstallAdapter in
both e2e/adapter/adapter_failover.go#L50-L59 and
e2e/cluster/adapter_failure.go#L51-L59 so partial installs are cleaned up even
when the assertion aborts; remove the duplicate err assertion in each spec, and
update each cleanup warning to use a constant format string with adapterName and
%v for the uninstall error.
---
Nitpick comments:
In `@pkg/e2e/suite.go`:
- Around line 69-87: Update both HelmChartCloneOptions initializations to use
helper.TestWorkDir for WorkDir, and replace the adapter component string with
labels.Adapter (using the corresponding shared label for the API clone). Also
update the stale WorkDir documentation in HelmChartCloneOptions to reference the
shared test-work directory.
In `@pkg/helper/adapter.go`:
- Around line 35-43: Remove the commented-out AdapterDeployment struct and the
stray ["name": "resourcetype"] note near the active AdapterDeployment
definition; leave the real AdapterDeployment implementation unchanged.
In `@pkg/helper/cleanup.go`:
- Around line 97-110: Update the cleanup flow around helmClient.UninstallRelease
so Helm uninstall operations are bounded by the existing five-minute context
deadline. Add context-aware cancellation or configure a bounded client timeout,
and ensure each release receives only the remaining deadline rather than an
unbounded wait. Preserve the existing release iteration and error handling
behavior.
In `@pkg/helper/git.go`:
- Around line 85-88: The successful clone path in the helper around os.MkdirTemp
leaks the generated componentDir, while only failure paths clean it up. Restore
caller-managed cleanup or add suite-level teardown that tracks every retained
clone path and removes it after each suite, including the paths involved in the
later success return around lines 152–156; preserve cleanup on all early-return
paths.
- Around line 28-30: Rename the stale ChartPath references to RepoPath
throughout the relevant struct documentation, validation error, and `#nosec`
comments, including the field comment near RepoPath and the checks around the
validation logic. Keep the existing validation behavior unchanged while ensuring
all user-facing and security annotations identify the actual RepoPath field.
In `@pkg/helper/helm/helm.go`:
- Around line 132-133: Update the comment above UninstallRelease to remove the
stale sentence about future migration to the Helm SDK, while retaining the
accurate description of the current installation workflow.
- Around line 93-110: The InstallRelease documentation includes the nonexistent
fileValues parameter, and its Helm timeout is hard-coded. Remove the fileValues
comment, then replace 5 * time.Minute in HelmClient.InstallRelease with a named
timeout constant; keep the existing InstallRelease behavior unchanged.
In `@testdata/adapter-configs/cl-m-wrong-nest.yaml`:
- Around line 277-396: Deduplicate the shared post.payloads configuration
between cl-m-wrong-nest.yaml and cl-m-wrong-ds.yaml by extending the
shared-values merge in clusters-base.tmpl to support a common adapterTaskConfig
base. Keep the shared Applied, Available, and Health CEL expressions in that
base, while preserving each fixture’s distinct adapter name and deliberately
incorrect discovery block.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d265ba79-467d-4ee1-937c-4beea6bc2e0d
📒 Files selected for processing (45)
e2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/cluster/adapter_failure.goe2e/cluster/crash_recovery.goe2e/cluster/stuck_deletion.gopkg/e2e/suite.gopkg/helper/adapter.gopkg/helper/adapter_test.gopkg/helper/cleanup.gopkg/helper/git.gopkg/helper/helm/helm.gopkg/labels/labels.gotestdata/adapter-configs/cl-crash.yamltestdata/adapter-configs/cl-crash/adapter-config.yamltestdata/adapter-configs/cl-crash/adapter-task-config.yamltestdata/adapter-configs/cl-crash/values.yamltestdata/adapter-configs/cl-invalid-resource.yamltestdata/adapter-configs/cl-invalid-resource/adapter-config.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yamltestdata/adapter-configs/cl-m-bad-api.yamltestdata/adapter-configs/cl-m-bad-api/adapter-config.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/values.yamltestdata/adapter-configs/cl-m-unreg-consumer.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/values.yamltestdata/adapter-configs/cl-m-wrong-ds.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-config.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/values.yamltestdata/adapter-configs/cl-m-wrong-nest.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-config.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/values.yamltestdata/adapter-configs/cl-param-error.yamltestdata/adapter-configs/cl-param-error/adapter-config.yamltestdata/adapter-configs/cl-param-error/adapter-task-config.yamltestdata/adapter-configs/cl-param-error/values.yamltestdata/adapter-configs/cl-stuck.yamltestdata/adapter-configs/cl-stuck/adapter-config.yamltestdata/adapter-configs/cl-stuck/adapter-task-config.yamltestdata/adapter-configs/cl-stuck/values.yamltestdata/adapter-configs/clusters-base.tmpl
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (25)
- testdata/adapter-configs/cl-invalid-resource/adapter-config.yaml
- testdata/adapter-configs/cl-param-error/values.yaml
- testdata/adapter-configs/cl-crash/adapter-config.yaml
- testdata/adapter-configs/cl-stuck/adapter-config.yaml
- testdata/adapter-configs/cl-param-error/adapter-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-config.yaml
- testdata/adapter-configs/cl-crash/adapter-task-config.yaml
- testdata/adapter-configs/cl-invalid-resource/values.yaml
- testdata/adapter-configs/cl-crash/values.yaml
- testdata/adapter-configs/cl-m-bad-api/values.yaml
- testdata/adapter-configs/cl-m-wrong-ds/adapter-config.yaml
- testdata/adapter-configs/cl-stuck/values.yaml
- testdata/adapter-configs/cl-invalid-resource/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-nest/adapter-config.yaml
- testdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-ds/values.yaml
- testdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yaml
- testdata/adapter-configs/cl-param-error/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-nest/values.yaml
- testdata/adapter-configs/cl-m-bad-api/adapter-config.yaml
- testdata/adapter-configs/cl-stuck/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/values.yaml
- testdata/adapter-configs/cl-m-bad-api/adapter-task-config.yaml
- pkg/helper/adapter_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/adapter/adapter_with_maestro.go (1)
460-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the install-plus-DeferCleanup block.
This exact sequence appears at lines 460-472, 610-621, 799-811, and 966-977, and again in
e2e/cluster/stuck_deletion.goat lines 51-62. Six copies of the same eight lines drift apart over time. Add one helper, for examplehelper.InstallAdapterWithCleanup(ctx, h, baseDeployOpts, adapterName, releaseName) error, that setsReleaseNameandAdapterName, installs, and registersDeferCleanup.🤖 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 `@e2e/adapter/adapter_with_maestro.go` around lines 460 - 472, Extract the repeated install-and-cleanup sequence into a shared helper such as InstallAdapterWithCleanup, setting ReleaseName and AdapterName before calling InstallAdapter and registering DeferCleanup to uninstall it. Replace all five occurrences in adapter_with_maestro.go and the matching occurrence in stuck_deletion.go, preserving existing error propagation and cleanup warning behavior.
🤖 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/helper/adapter.go`:
- Around line 170-183: Update the base values file handling after the rendered
YAML unmarshal to include baseValuesFilePath in both the os.ReadFile error and
the subsequent yaml.Unmarshal error. Preserve the existing error wrapping while
replacing the hard-coded adapterConfig.yaml reference with the actual fixture
path.
---
Nitpick comments:
In `@e2e/adapter/adapter_with_maestro.go`:
- Around line 460-472: Extract the repeated install-and-cleanup sequence into a
shared helper such as InstallAdapterWithCleanup, setting ReleaseName and
AdapterName before calling InstallAdapter and registering DeferCleanup to
uninstall it. Replace all five occurrences in adapter_with_maestro.go and the
matching occurrence in stuck_deletion.go, preserving existing error propagation
and cleanup warning behavior.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 98b97aaa-ffa9-42f8-8c04-947fc809d4a8
📒 Files selected for processing (45)
e2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/cluster/adapter_failure.goe2e/cluster/crash_recovery.goe2e/cluster/stuck_deletion.gopkg/e2e/suite.gopkg/helper/adapter.gopkg/helper/adapter_test.gopkg/helper/cleanup.gopkg/helper/git.gopkg/helper/helm/helm.gopkg/labels/labels.gotestdata/adapter-configs/cl-crash.yamltestdata/adapter-configs/cl-crash/adapter-config.yamltestdata/adapter-configs/cl-crash/adapter-task-config.yamltestdata/adapter-configs/cl-crash/values.yamltestdata/adapter-configs/cl-invalid-resource.yamltestdata/adapter-configs/cl-invalid-resource/adapter-config.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yamltestdata/adapter-configs/cl-m-bad-api.yamltestdata/adapter-configs/cl-m-bad-api/adapter-config.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/values.yamltestdata/adapter-configs/cl-m-unreg-consumer.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/values.yamltestdata/adapter-configs/cl-m-wrong-ds.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-config.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/values.yamltestdata/adapter-configs/cl-m-wrong-nest.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-config.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/values.yamltestdata/adapter-configs/cl-param-error.yamltestdata/adapter-configs/cl-param-error/adapter-config.yamltestdata/adapter-configs/cl-param-error/adapter-task-config.yamltestdata/adapter-configs/cl-param-error/values.yamltestdata/adapter-configs/cl-stuck.yamltestdata/adapter-configs/cl-stuck/adapter-config.yamltestdata/adapter-configs/cl-stuck/adapter-task-config.yamltestdata/adapter-configs/cl-stuck/values.yamltestdata/adapter-configs/clusters-base.tmpl
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (25)
- testdata/adapter-configs/cl-invalid-resource/adapter-config.yaml
- testdata/adapter-configs/cl-m-bad-api/adapter-task-config.yaml
- testdata/adapter-configs/cl-param-error/adapter-config.yaml
- testdata/adapter-configs/cl-crash/adapter-config.yaml
- testdata/adapter-configs/cl-stuck/adapter-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/values.yaml
- testdata/adapter-configs/cl-crash/adapter-task-config.yaml
- testdata/adapter-configs/cl-stuck/adapter-task-config.yaml
- testdata/adapter-configs/cl-invalid-resource/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml
- testdata/adapter-configs/cl-stuck/values.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-config.yaml
- testdata/adapter-configs/cl-m-bad-api/values.yaml
- testdata/adapter-configs/cl-m-wrong-ds/values.yaml
- pkg/helper/adapter_test.go
- testdata/adapter-configs/cl-m-bad-api/adapter-config.yaml
- testdata/adapter-configs/cl-param-error/values.yaml
- testdata/adapter-configs/cl-invalid-resource/values.yaml
- testdata/adapter-configs/cl-crash/values.yaml
- testdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-nest/adapter-config.yaml
- testdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-ds/adapter-config.yaml
- testdata/adapter-configs/cl-m-wrong-nest/values.yaml
- testdata/adapter-configs/cl-param-error/adapter-task-config.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/labels/labels.go
- pkg/e2e/suite.go
- e2e/cluster/adapter_failure.go
- testdata/adapter-configs/cl-m-unreg-consumer.yaml
- testdata/adapter-configs/cl-m-wrong-nest.yaml
- testdata/adapter-configs/cl-invalid-resource.yaml
- testdata/adapter-configs/clusters-base.tmpl
- e2e/cluster/crash_recovery.go
- testdata/adapter-configs/cl-m-bad-api.yaml
- testdata/adapter-configs/cl-stuck.yaml
- pkg/helper/cleanup.go
- testdata/adapter-configs/cl-m-wrong-ds.yaml
- testdata/adapter-configs/cl-param-error.yaml
- testdata/adapter-configs/cl-crash.yaml
- pkg/helper/git.go
- e2e/adapter/adapter_failover.go
- pkg/helper/helm/helm.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/helper/adapter.go (1)
135-153: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPropagate cluster-scoped cleanup failures.
UninstallAdaptercontinues aftercleanupClusterScopedResources, but that helper returns no error. Thekubectlfailures at Lines 199-214 are logged atInfoand discarded. A timeout, missingkubectl, or RBAC denial can leaveClusterRoleandClusterRoleBindingobjects behind while cleanup reports success.Return and wrap the delete errors, append them to the uninstall error aggregate, and log failures at
Error. This is incomplete cleanup (CWE-459) and can retain cluster-scoped RBAC objects across adapter runs.As per path instructions: “MUST clean up on ALL paths” and “Log-and-continue MUST be intentional degradation with a comment.”
🤖 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/helper/adapter.go` around lines 135 - 153, Update cleanupClusterScopedResources and UninstallAdapter so cluster-scoped resource deletion errors are returned, logged at Error, and appended to the existing errs aggregate with context. Ensure cleanup is attempted on every path and preserve intentional log-and-continue behavior only where explicitly documented with a comment.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/helper/adapter.go`:
- Around line 135-153: Update cleanupClusterScopedResources and UninstallAdapter
so cluster-scoped resource deletion errors are returned, logged at Error, and
appended to the existing errs aggregate with context. Ensure cleanup is
attempted on every path and preserve intentional log-and-continue behavior only
where explicitly documented with a comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 802434d3-a7d0-45f4-a8b6-3a0a61836225
📒 Files selected for processing (6)
e2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/cluster/adapter_failure.goe2e/cluster/crash_recovery.goe2e/cluster/stuck_deletion.gopkg/helper/adapter.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/cluster/adapter_failure.go
- e2e/cluster/crash_recovery.go
- e2e/adapter/adapter_failover.go
- e2e/cluster/stuck_deletion.go
1 participant
Summary
Refactors adapter Helm chart installation to use the Helm Go SDK instead of shelling out to the
helmCLI (os.Exec), and consolidates adapter config files from multi-file directories into single self-contained YAML files with a shared base template. This eliminates the need for externalhelmbinaries, simplifies adapter config management, and introduces async.Oncepattern for chart cloning so each chart repo is cloned at most once per suite run.Changes
helm install/helm uninstallCLI calls with the Helm v3 Go SDK (helm.sh/helm/v3) via a newpkg/helper/helm/helm.gopackage providingInstallRelease,UninstallRelease, andListReleasesBySelectormethodsCloneHelmChartfrom an instance method onHelperto a package-level function, and introducedGitCloneChartwithsync.Once-basedCloneChartOnceto clone each chart repository exactly once per suite runBeforeSuite(pkg/e2e/suite.go), creatingAdapterGitCloneandApiGitClonesingletons that tests reference instead of cloning per-testInstallAdapterto use a two-layer templating approach: a shared base template (clusters-base.tmpl) for common broker/image/label config, merged with per-adapter YAML value filesadapter-config.yaml,adapter-task-config.yaml,values.yaml) into single flat YAML files undertestdata/adapter-configs/(e.g.,cl-crash.yaml,cl-stuck.yaml,cl-m-bad-api.yaml)adapter_failover.go,adapter_with_maestro.go,crash_recovery.go,stuck_deletion.go,adapter_failure.go) to useCloneChartOnceand the newInstallAdaptersignature, removing per-test chart cloning and cleanup boilerplatelabels.Adapterlabel constant and applied it to adapter test suitespkg/helper/adapter_test.go(tests for the old multi-file config parsing approach that no longer exists)Notes
Test Plan
make checkpasses (generate, fmt-check, vet, lint, test)make buildcompiles successfully