HYPERFLEET-599 | test: add the tier1 case automation and test logic#44
Open
yingzhanredhat wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Open
HYPERFLEET-599 | test: add the tier1 case automation and test logic#44yingzhanredhat wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
yingzhanredhat wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
|
[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 |
|
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 end-to-end test coverage and helper tooling to deploy and validate adapter failover scenarios. New e2e test e2e/adapter/adapter_failover.go exercises an adapter configured with an invalid Kubernetes resource and asserts reported AdapterStatus. Introduces adapter deployment config fields in pkg/config, Helm/chart utilities and constants in pkg/helper (chart clone, deploy, uninstall, release-name generation, TestDataPath, AdapterConfigsDir, TestWorkDir, resource-type constants), new testdata adapter configs and Helm values, Makefile e2e target updates to set TESTDATA_DIR, a .gitignore entry for .test-work, and several indirect go.mod entries. Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Helper as Helper
participant Git as Git Repo
participant Helm as Helm CLI
participant K8s as Kubernetes API
participant API as Cluster API
Test->>Helper: CloneHelmChart(ctx, opts)
Helper->>Git: git clone (sparse) + sparse-checkout
Git-->>Helper: chartPath + cleanup()
Test->>Helper: DeployAdapter(ctx, opts)
Helper->>Helper: generate release name, expand values (envsubst)
Helper->>Helm: helm upgrade --install --timeout
Helm->>K8s: create Deployment/Pods
K8s-->>Helm: deployment status
Helm-->>Helper: result
alt Adapter deployed
Test->>K8s: poll adapter pods & adapter resources
K8s-->>Test: pod statuses
Test->>API: query cluster AdapterStatus
API-->>Test: AdapterStatus (Available/reason/message)
else Deployment failed
Helper->>K8s: list pods by release label
Helper->>K8s: collect pod logs & describe
Helper->>Helper: write diagnostics to OUTPUT_DIR
Helper-->>Test: error + diagnostics path
end
Test->>Helper: UninstallAdapter(ctx, release, ns)
Helper->>Helm: helm uninstall
Helm->>K8s: remove resources
Helper->>K8s: cleanup ClusterRole/ClusterRoleBinding
K8s-->>Helper: cleanup complete
Helper-->>Test: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 2
🧹 Nitpick comments (6)
pkg/helper/git.go (1)
109-112: Cleanup error is silently ignored on failure paths.When sparse-checkout init fails,
cleanup()is called but its return value is ignored. This could leave behind partially cloned directories if cleanup fails.Suggested improvement
if output, err := cmd.CombinedOutput(); err != nil { - cleanup() + if cleanupErr := cleanup(); cleanupErr != nil { + logger.Error("cleanup failed after sparse-checkout init error", "error", cleanupErr) + } return "", nil, fmt.Errorf("sparse-checkout init failed: %w\nOutput: %s", err, string(output)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helper/git.go` around lines 109 - 112, The cleanup() return value is ignored when cmd.CombinedOutput() fails in the sparse-checkout init path; call cleanup(), capture its error, and include it when returning the failure from that block (e.g. wrap the original error and output with any cleanupErr from cleanup()) so partial artifacts aren't silently left behind; update the error return in the block that currently calls cmd.CombinedOutput() and cleanup() (keeping the existing message like "sparse-checkout init failed") to include cleanupErr details.pkg/helper/adapter.go (2)
356-378: External dependency onenvsubstcommand should be documented or validated.The function relies on the
envsubstcommand being available in the PATH. If it's missing, the error message may not clearly indicate the root cause.Suggested improvement to add validation
func expandEnvVarsInYAMLToBytes(yamlPath string) ([]byte, error) { + // Verify envsubst is available + if _, err := exec.LookPath("envsubst"); err != nil { + return nil, fmt.Errorf("envsubst command not found in PATH (install gettext package): %w", err) + } + // Read the YAML file content, err := os.ReadFile(yamlPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helper/adapter.go` around lines 356 - 378, The function expandEnvVarsInYAMLToBytes currently calls exec.Command("envsubst") without verifying the binary exists, so add a validation using exec.LookPath("envsubst") before creating the command and return a clear, descriptive error if not found (e.g., "envsubst not found in PATH"); alternatively or additionally update documentation to state envsubst is required. Locate expandEnvVarsInYAMLToBytes and replace the direct exec.Command use with a pre-check for the envsubst executable (referencing exec.LookPath and the "envsubst" symbol) and ensure the returned error distinguishes a missing dependency from a runtime failure.
17-25: Consider usingcrypto/randor seedingmath/randfor better randomness.In Go versions before 1.20,
math/randuses a deterministic seed by default, which could lead to predictable release names across test runs. While Go 1.20+ auto-seeds, explicitly seeding or usingcrypto/randensures consistent behavior across Go versions.Alternative using crypto/rand
+import ( + "crypto/rand" + "math/big" +) + func generateRandomString(length int) string { const charset = "abcdefghijklmnopqrstuvwxyz0123456789" b := make([]byte, length) for i := range b { - b[i] = charset[rand.Intn(len(charset))] + n, _ := rand.Int(rand.Reader, big.NewInt(int64(len(charset)))) + b[i] = charset[n.Int64()] } return string(b) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helper/adapter.go` around lines 17 - 25, The generateRandomString function currently uses math/rand which can be deterministic on older Go versions; replace it with a cryptographically secure source or explicitly seed math/rand: preferred fix — switch to crypto/rand by importing "crypto/rand" and use crypto/rand.Read to fill a byte slice and map bytes into the charset inside generateRandomString (refer to function name generateRandomString), ensuring you handle/read errors; alternative quick fix — add an init() that calls rand.Seed(time.Now().UnixNano()) (importing "math/rand" and "time") so math/rand is explicitly seeded across Go versions.e2e/adapter/adapter_failover.go (2)
76-86: Consider registering DeferCleanup only on successful deployment.The cleanup is registered before verifying deployment succeeded. If deployment fails, the uninstall attempt will log a warning but continues execution. While the warning message handles this gracefully, consider restructuring to avoid unnecessary cleanup attempts.
Alternative approach
err := h.DeployAdapter(ctx, deployOpts) + Expect(err).NotTo(HaveOccurred(), "failed to deploy test adapter") + // Ensure adapter cleanup happens after this test ginkgo.DeferCleanup(func(ctx context.Context) { ginkgo.By("Uninstall test adapter") if err := h.UninstallAdapter(ctx, releaseName, h.Cfg.Namespace); err != nil { ginkgo.GinkgoWriter.Printf("Warning: failed to uninstall adapter %s: %v\n", releaseName, err) } else { ginkgo.GinkgoWriter.Printf("Successfully uninstalled adapter: %s\n", releaseName) } }) - Expect(err).NotTo(HaveOccurred(), "failed to deploy test adapter")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/adapter/adapter_failover.go` around lines 76 - 86, Register the ginkgo.DeferCleanup only after confirming DeployAdapter succeeded: call err := h.DeployAdapter(ctx, deployOpts), check Expect(err).NotTo(HaveOccurred()) (or if you prefer a conditional, if err == nil), and then call ginkgo.DeferCleanup that invokes h.UninstallAdapter(ctx, releaseName, h.Cfg.Namespace). Move the current DeferCleanup block so it is executed only when DeployAdapter returned no error, referencing DeployAdapter, UninstallAdapter, ginkgo.DeferCleanup, releaseName and h.Cfg.Namespace.
186-187: Assertion on error message content could be brittle.The check
ContainSubstring("invalid")couples the test to specific error message wording. If the adapter changes its error messaging, this test could fail unexpectedly. Consider whether this level of message validation is necessary, or if verifying the condition status alone suffices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/adapter/adapter_failover.go` around lines 186 - 187, The assertion that the error message contains the literal substring "invalid" is brittle; update the check around availableCondition.Message by removing the ContainSubstring("invalid") assertion and instead assert only the condition's presence and expected status/reason (e.g., verify availableCondition is non-nil and its Status or Reason equals the expected enum), or at minimum assert the message is non-empty rather than matching specific wording; modify the g.Expect(...) call that currently uses ContainSubstring("invalid") on availableCondition.Message accordingly.test-design/testcases/adapter-failover.md (1)
115-116: Minor grammar issue in test step description.Line 115: "adapter will stuck status" → "adapter will get stuck in a status" or similar phrasing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-design/testcases/adapter-failover.md` around lines 115 - 116, The test step text contains a grammar error: replace the phrase "Simulate a scenario that adapter will stuck status" with clearer wording such as "Simulate a scenario where the adapter becomes stuck in a status" or "Simulate a scenario in which the adapter gets stuck in a status"; update the bullet in the test case that currently reads "Simulate a scenario that adapter will stuck status" to one of these corrected phrasings to improve clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/adapter/adapter_failover.go`:
- Line 63: The test sets an environment variable via os.Setenv("ADAPTER_NAME",
adapterName) but does not restore or unset it, causing leakage into other tests;
update the test around the os.Setenv call to capture the previous value (via
os.LookupEnv) and register a cleanup that restores the original value or unsets
ADAPTER_NAME (e.g., using t.Cleanup or defer) so ADAPTER_NAME is returned to its
prior state after the test finishes, referencing the existing os.Setenv call and
the adapterName variable to locate where to add the cleanup.
In `@pkg/helper/adapter.go`:
- Around line 435-447: The custom functions contains and containsAt reimplement
stdlib behavior; remove both functions and replace their usage with
strings.Contains; update any references to call strings.Contains(s, substr) and
add the "strings" import where missing (e.g., in adapter.go) so the standard
library function is used throughout instead of the custom contains/containsAt
helpers.
---
Nitpick comments:
In `@e2e/adapter/adapter_failover.go`:
- Around line 76-86: Register the ginkgo.DeferCleanup only after confirming
DeployAdapter succeeded: call err := h.DeployAdapter(ctx, deployOpts), check
Expect(err).NotTo(HaveOccurred()) (or if you prefer a conditional, if err ==
nil), and then call ginkgo.DeferCleanup that invokes h.UninstallAdapter(ctx,
releaseName, h.Cfg.Namespace). Move the current DeferCleanup block so it is
executed only when DeployAdapter returned no error, referencing DeployAdapter,
UninstallAdapter, ginkgo.DeferCleanup, releaseName and h.Cfg.Namespace.
- Around line 186-187: The assertion that the error message contains the literal
substring "invalid" is brittle; update the check around
availableCondition.Message by removing the ContainSubstring("invalid") assertion
and instead assert only the condition's presence and expected status/reason
(e.g., verify availableCondition is non-nil and its Status or Reason equals the
expected enum), or at minimum assert the message is non-empty rather than
matching specific wording; modify the g.Expect(...) call that currently uses
ContainSubstring("invalid") on availableCondition.Message accordingly.
In `@pkg/helper/adapter.go`:
- Around line 356-378: The function expandEnvVarsInYAMLToBytes currently calls
exec.Command("envsubst") without verifying the binary exists, so add a
validation using exec.LookPath("envsubst") before creating the command and
return a clear, descriptive error if not found (e.g., "envsubst not found in
PATH"); alternatively or additionally update documentation to state envsubst is
required. Locate expandEnvVarsInYAMLToBytes and replace the direct exec.Command
use with a pre-check for the envsubst executable (referencing exec.LookPath and
the "envsubst" symbol) and ensure the returned error distinguishes a missing
dependency from a runtime failure.
- Around line 17-25: The generateRandomString function currently uses math/rand
which can be deterministic on older Go versions; replace it with a
cryptographically secure source or explicitly seed math/rand: preferred fix —
switch to crypto/rand by importing "crypto/rand" and use crypto/rand.Read to
fill a byte slice and map bytes into the charset inside generateRandomString
(refer to function name generateRandomString), ensuring you handle/read errors;
alternative quick fix — add an init() that calls
rand.Seed(time.Now().UnixNano()) (importing "math/rand" and "time") so math/rand
is explicitly seeded across Go versions.
In `@pkg/helper/git.go`:
- Around line 109-112: The cleanup() return value is ignored when
cmd.CombinedOutput() fails in the sparse-checkout init path; call cleanup(),
capture its error, and include it when returning the failure from that block
(e.g. wrap the original error and output with any cleanupErr from cleanup()) so
partial artifacts aren't silently left behind; update the error return in the
block that currently calls cmd.CombinedOutput() and cleanup() (keeping the
existing message like "sparse-checkout init failed") to include cleanupErr
details.
In `@test-design/testcases/adapter-failover.md`:
- Around line 115-116: The test step text contains a grammar error: replace the
phrase "Simulate a scenario that adapter will stuck status" with clearer wording
such as "Simulate a scenario where the adapter becomes stuck in a status" or
"Simulate a scenario in which the adapter gets stuck in a status"; update the
bullet in the test case that currently reads "Simulate a scenario that adapter
will stuck status" to one of these corrected phrasings to improve clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17577438-137e-4aec-b13b-922b4a44200f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignoreMakefilee2e/adapter/adapter_failover.gogo.modpkg/config/config.gopkg/helper/adapter.gopkg/helper/constants.gopkg/helper/git.gotest-design/testcases/adapter-failover.mdtest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-invalid-resource/adapter-config.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-design/testcases/adapter-failover.md (1)
92-100:⚠️ Potential issue | 🟠 MajorUpdate metadata to reflect current automation status.
The second test case's metadata shows "Not Automated" with an old update date (2026-01-30), which is inconsistent with the PR's objective of adding automated test coverage for adapter failover scenarios. If this test case is being automated as part of this PR (similar to the first test case), the metadata should be updated accordingly.
📝 Suggested metadata update
| **Field** | **Value** | |-----------|-----------| | **Pos/Neg** | Negative | -| **Priority** | Tier2 | +| **Priority** | Tier1 | | **Status** | Draft | -| **Automation** | Not Automated | +| **Automation** | Automated | | **Version** | MVP | | **Created** | 2026-01-30 | -| **Updated** | 2026-01-30 | +| **Updated** | 2026-03-13 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-design/testcases/adapter-failover.md` around lines 92 - 100, Update the second test case's metadata table: set the "Automation" field from "Not Automated" to "Automated" and update the "Updated" field to the current date (2026-03-13); locate the change in the second test case's metadata table (the row labels "Automation" and "Updated") in adapter-failover's test case document and commit the corrected table entry.
🧹 Nitpick comments (2)
pkg/helper/adapter.go (1)
364-386: Consider checking forenvsubstavailability.The function relies on the external
envsubstcommand without verifying it's installed. This could cause confusing failures in environments where it's not available.♻️ Suggested improvement
func expandEnvVarsInYAMLToBytes(yamlPath string) ([]byte, error) { + // Check if envsubst is available + if _, err := exec.LookPath("envsubst"); err != nil { + return nil, fmt.Errorf("envsubst command not found: %w (install gettext-base or equivalent)", err) + } + // Read the YAML file content, err := os.ReadFile(yamlPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helper/adapter.go` around lines 364 - 386, The function expandEnvVarsInYAMLToBytes currently calls the external envsubst without verifying it's present; add an explicit availability check using exec.LookPath("envsubst") before creating the command, and if LookPath returns an error return a clear, actionable error (e.g., "envsubst not found in PATH") so callers know the dependency is missing; keep the existing cmd.Run() and stderr handling for runtime failures but fail fast with an informative message when envsubst is not installed.test-design/testcases/adapter-failover.md (1)
67-67: Consider clarifying the expected condition status.The phrase "All the type condition status should be 'False'" is somewhat ambiguous. Based on the example that follows, it appears the intent is that all conditions should have their
statusfield set to'False'.💡 Suggested clarification
-- All the type condition status should be 'False' +- All conditions should have status='False'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-design/testcases/adapter-failover.md` at line 67, Update the ambiguous sentence "All the type condition status should be 'False'" to a clear expectation such as "All conditions MUST have their status field set to 'False'." Locate the occurrence of the exact phrase "All the type condition status should be 'False'" in test-design/testcases/adapter-failover.md and replace it with the clarified wording (or a variant that uses "conditions", "status field", and an explicit MUST/should), ensuring consistency with the example that follows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-design/testcases/adapter-failover.md`:
- Line 115: Update the test step description string "adapter will stuck status"
to correct English—replace it with a clear phrase such as "adapter will be stuck
in a status" or "adapter will get stuck" wherever that exact text appears in the
test case (search for the literal "adapter will stuck status"); ensure the
revised sentence fits the test's intent and matches surrounding tense and style.
---
Outside diff comments:
In `@test-design/testcases/adapter-failover.md`:
- Around line 92-100: Update the second test case's metadata table: set the
"Automation" field from "Not Automated" to "Automated" and update the "Updated"
field to the current date (2026-03-13); locate the change in the second test
case's metadata table (the row labels "Automation" and "Updated") in
adapter-failover's test case document and commit the corrected table entry.
---
Nitpick comments:
In `@pkg/helper/adapter.go`:
- Around line 364-386: The function expandEnvVarsInYAMLToBytes currently calls
the external envsubst without verifying it's present; add an explicit
availability check using exec.LookPath("envsubst") before creating the command,
and if LookPath returns an error return a clear, actionable error (e.g.,
"envsubst not found in PATH") so callers know the dependency is missing; keep
the existing cmd.Run() and stderr handling for runtime failures but fail fast
with an informative message when envsubst is not installed.
In `@test-design/testcases/adapter-failover.md`:
- Line 67: Update the ambiguous sentence "All the type condition status should
be 'False'" to a clear expectation such as "All conditions MUST have their
status field set to 'False'." Locate the occurrence of the exact phrase "All the
type condition status should be 'False'" in
test-design/testcases/adapter-failover.md and replace it with the clarified
wording (or a variant that uses "conditions", "status field", and an explicit
MUST/should), ensuring consistency with the example that follows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51751ea5-6ec0-471e-a909-9c535925f40f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignoreMakefilee2e/adapter/adapter_failover.gogo.modpkg/config/config.gopkg/helper/adapter.gopkg/helper/constants.gopkg/helper/git.gotest-design/testcases/adapter-failover.mdtest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-invalid-resource/adapter-config.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- testdata/adapter-configs/cl-invalid-resource/adapter-config.yaml
- test-design/testcases/adapter-with-maestro-transport.md
- testdata/adapter-configs/cl-invalid-resource/values.yaml
- go.mod
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-design/testcases/adapter-failover.md (1)
40-67:⚠️ Potential issue | 🟡 MinorRename the copied timeout/template-rendering steps.
This first scenario now validates invalid-resource rejection, but the step titles/results still talk about template rendering and timeout handling. The mismatch makes the Tier1 design disagree with the automated flow in
e2e/adapter/adapter_failover.go.As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-design/testcases/adapter-failover.md` around lines 40 - 67, The test steps in test-design/testcases/adapter-failover.md reference "template rendering" and "timeout handling" but the scenario actually validates invalid-resource rejection; update the step titles and expected results to reflect "invalid resource rejection" (e.g., rename "Test template rendering errors" to "Test invalid resource rejection" and update Step 3 to reflect verifying adapter rejects invalid K8s resources and shows related error statuses), and ensure descriptions align with the automated test flow in e2e/adapter/adapter_failover.go so the markdown accurately documents what the code exercises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/config.go`:
- Around line 346-359: Replace the direct logging of
c.AdapterDeployment.ChartRepo inside the slog.Info("Loaded configuration"...)
call with a redacted value (e.g., call the existing redactURL function or a new
redactRepo helper) so credentials in ChartRepo aren't logged; update the
slog.Info entry for "adapter_deployment_chart_repo" to use
redactURL(c.AdapterDeployment.ChartRepo) (or a new redact function) and ensure
any other places that log AdapterDeployment.ChartRepo use the same redaction
helper (refer to slog.Info and c.AdapterDeployment.ChartRepo to locate all
occurrences).
In `@pkg/helper/adapter.go`:
- Around line 48-60: GenerateAdapterReleaseName currently builds baseReleaseName
including randomSuffix then truncates, which can remove the suffix; instead,
compute the prefix without the randomSuffix (e.g., prefix :=
fmt.Sprintf("adapter-%s-%s", resourceType, adapterName)), calculate
allowedPrefixLen = maxReleaseNameLength - (len(randomSuffix) + 1) to reserve the
hyphen and suffix, truncate prefix to allowedPrefixLen if needed, then append
"-" + randomSuffix to form the final releaseName; update references to
baseReleaseName, randomSuffix, maxReleaseNameLength and ensure the final string
never exceeds maxReleaseNameLength.
- Around line 189-206: The uninstall path returns early when output contains
"not found", skipping cleanupClusterScopedResources and leaving orphaned
ClusterRole/ClusterRoleBinding behind; update the handler so that when
strings.Contains(string(output), "not found") is true you still call
h.cleanupClusterScopedResources(ctx, releaseName) before returning (or
restructure so cleanupClusterScopedResources(ctx, releaseName) is invoked in
both the "not found" branch and the successful uninstall branch), keeping the
existing logger.Info("adapter release not found, skipping uninstall",
"release_name", releaseName) behavior and error handling around
cmd.CombinedOutput()/logger.Error unchanged.
- Around line 93-107: The TestDataDir constant is using a relative path; change
it to resolve from an environment variable (e.g., read os.Getenv("TESTDATA_DIR")
and fall back to "testdata" when empty) and update all usages to reference the
resolved value; specifically replace the constant TestDataDir and any hardcoded
"testdata" references used by functions in pkg/helper/adapter.go
(sourceAdapterDir/destAdapterDir logic and copyDir call) and the other files
mentioned (adapter_failover.go, adapter_with_maestro.go, cluster/creation.go,
nodepool/creation.go) to use the new resolver so the code honors the
TESTDATA_DIR env var while preserving the existing fallback for local tests.
In `@pkg/helper/git.go`:
- Around line 50-62: The clone fails when the parent workDir doesn't exist;
before creating componentDir or running the git clone (the code that computes
workDir and componentDir using opts.WorkDir, TestWorkDir and opts.Component),
ensure workDir is created with os.MkdirAll(workDir, 0o755) (or equivalent) and
check/return any error; do this after computing workDir but before the removal
of componentDir and the git clone operation so the clone destination's parent
exists.
- Around line 21-22: The struct field Ref is documented to accept branch, tag,
or commit SHA but the clone logic uses git clone --branch (which fails for
SHAs); either update the Ref docstring to say "branch or tag only" or change the
cloning code (around the git clone call using opts.Ref in pkg/helper/git.go) to
support SHAs: detect if opts.Ref is a commit SHA (40 hex chars) and in that case
clone without --branch/with --no-checkout (or plain shallow clone), then run git
fetch origin <sha> and git checkout FETCH_HEAD (or git checkout <sha>),
otherwise continue using --branch for branch/tag. Ensure the same Ref variable
is used for the subsequent checkout path and handle errors accordingly.
---
Outside diff comments:
In `@test-design/testcases/adapter-failover.md`:
- Around line 40-67: The test steps in test-design/testcases/adapter-failover.md
reference "template rendering" and "timeout handling" but the scenario actually
validates invalid-resource rejection; update the step titles and expected
results to reflect "invalid resource rejection" (e.g., rename "Test template
rendering errors" to "Test invalid resource rejection" and update Step 3 to
reflect verifying adapter rejects invalid K8s resources and shows related error
statuses), and ensure descriptions align with the automated test flow in
e2e/adapter/adapter_failover.go so the markdown accurately documents what the
code exercises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 027daecf-09cb-4cc7-8150-74743b7b1249
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignoreMakefilee2e/adapter/adapter_failover.gogo.modpkg/config/config.gopkg/helper/adapter.gopkg/helper/constants.gopkg/helper/git.gotest-design/testcases/adapter-failover.mdtest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-invalid-resource/adapter-config.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yaml
✅ Files skipped from review due to trivial changes (1)
- testdata/adapter-configs/cl-invalid-resource/adapter-config.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- test-design/testcases/adapter-with-maestro-transport.md
- Makefile
- testdata/adapter-configs/cl-invalid-resource/values.yaml
- testdata/adapter-configs/cl-invalid-resource/adapter-task-config.yaml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
e2e/adapter/adapter_failover.go (1)
63-67:⚠️ Potential issue | 🟡 MinorRestore previous
ADAPTER_NAMEvalue instead of always unsetting it.Line 66 currently clears global process state even when
ADAPTER_NAMEwas already set before this spec, which can affect subsequent specs.Proposed fix
- err := os.Setenv("ADAPTER_NAME", adapterName) + prevAdapterName, hadPrevAdapterName := os.LookupEnv("ADAPTER_NAME") + err := os.Setenv("ADAPTER_NAME", adapterName) Expect(err).NotTo(HaveOccurred(), "failed to set ADAPTER_NAME environment variable") ginkgo.DeferCleanup(func() { - _ = os.Unsetenv("ADAPTER_NAME") + if hadPrevAdapterName { + _ = os.Setenv("ADAPTER_NAME", prevAdapterName) + } else { + _ = os.Unsetenv("ADAPTER_NAME") + } })As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/adapter/adapter_failover.go` around lines 63 - 67, The spec currently sets ADAPTER_NAME with os.Setenv(adapterName) and always unsets it in ginkgo.DeferCleanup, which destroys any previous value; change the cleanup to capture the prior value before setting (e.g., prev, existed := os.LookupEnv("ADAPTER_NAME")), then in ginkgo.DeferCleanup restore the previous value if existed (os.Setenv("ADAPTER_NAME", prev)) or unset it only if it did not exist (os.Unsetenv("ADAPTER_NAME")), leaving the rest of the spec unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/adapter/adapter_failover.go`:
- Around line 111-125: The initial checks on cluster conditions after creation
are race-prone; replace the immediate single GetCluster + HasResourceCondition
assertions with polling using Ginkgo's Eventually to wait for the cluster to
reach the expected conditions. Specifically, wrap calls that invoke
h.Client.GetCluster and checks using h.HasResourceCondition over
cluster.Status.Conditions (for client.ConditionTypeReady and
client.ConditionTypeAvailable with openapi.ResourceConditionStatusFalse) in an
Eventually block with a reasonable timeout and interval, asserting true only
after Eventually returns success so the test tolerates asynchronous condition
propagation.
In `@pkg/helper/git.go`:
- Around line 79-83: The log currently prints opts.RepoURL which can contain
credentials; update the logging before the logger.Info call to sanitize the
repository URL by parsing opts.RepoURL (using net/url.Parse), removing or
masking the User info (clear the URL.User field or replace with a placeholder),
and then log the sanitized URL instead of opts.RepoURL in the logger.Info
invocation (the call that includes "component", opts.Component, "repo",
opts.RepoURL, "ref", opts.Ref, "chart_path", opts.ChartPath). Ensure the
sanitized value replaces the raw repo value in that logger.Info call so no
userinfo/token is emitted.
- Around line 68-92: The code uses a fixed componentDir (componentDir :=
filepath.Join(workDir, opts.Component)) which can be raced by parallel tests;
change the clone destination to a unique per-run directory (e.g., create a
temp/unique subdirectory under workDir using a UUID/PID or ioutil.TempDir) and
update cleanup to only remove that unique directory; replace the existing
stat/remove logic that deletes the shared componentDir and ensure all references
to componentDir (the variable), cleanup, and the logger messages use the new
unique path so parallel runs do not remove each other’s checkouts.
---
Duplicate comments:
In `@e2e/adapter/adapter_failover.go`:
- Around line 63-67: The spec currently sets ADAPTER_NAME with
os.Setenv(adapterName) and always unsets it in ginkgo.DeferCleanup, which
destroys any previous value; change the cleanup to capture the prior value
before setting (e.g., prev, existed := os.LookupEnv("ADAPTER_NAME")), then in
ginkgo.DeferCleanup restore the previous value if existed
(os.Setenv("ADAPTER_NAME", prev)) or unset it only if it did not exist
(os.Unsetenv("ADAPTER_NAME")), leaving the rest of the spec unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 247706bf-938c-4d59-8ce7-6a5a101153e0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.gitignoreMakefilee2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/cluster/creation.goe2e/nodepool/creation.gogo.modpkg/config/config.gopkg/helper/adapter.gopkg/helper/constants.gopkg/helper/git.gopkg/helper/helper.gotest-design/testcases/adapter-failover.mdtest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-invalid-resource/adapter-config.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yaml
✅ Files skipped from review due to trivial changes (2)
- testdata/adapter-configs/cl-invalid-resource/adapter-task-config.yaml
- test-design/testcases/adapter-with-maestro-transport.md
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- pkg/helper/constants.go
- testdata/adapter-configs/cl-invalid-resource/adapter-config.yaml
- testdata/adapter-configs/cl-invalid-resource/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/helper/adapter.go (2)
273-353: Give each diagnostic command its own timeout.
cmdCtxis shared across the pod list plus everykubectl logsandkubectl describecall. Once that 30s budget is consumed, later diagnostics start already canceled and you lose the remaining failure evidence. Create a fresh short-lived context per external call instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helper/adapter.go` around lines 273 - 353, The code currently reuses cmdCtx (created by cmdCtx, cancel := context.WithTimeout(ctx, 30*time.Second)) for the initial pod list and every exec.CommandContext (podLogCmd, podDescCmd), which can exhaust the 30s budget and cancel later kubectl calls; update the pod-level diagnostics to create a fresh short-lived context per external call (e.g., for podLogCmd and podDescCmd use callCtx, callCancel := context.WithTimeout(ctx, 5*time.Second); defer callCancel() immediately after creation) and pass callCtx to exec.CommandContext instead of cmdCtx, ensuring the initial list still uses the 30s cmdCtx while each kubectl logs/describe gets its own timeout.
377-396: Consider simplifying environment variable expansion in values.yaml.
expandEnvVarsInYAMLToBytes()shells out toenvsubstwhenos.ExpandEnv()provides the same plain${VAR}expansion without an external process. Using the in-process approach reduces code complexity and eliminates the exec overhead:Suggested simplification
func expandEnvVarsInYAMLToBytes(yamlPath string) ([]byte, error) { // Read the YAML file content, err := os.ReadFile(yamlPath) // `#nosec` G304 -- yamlPath is constructed from trusted config if err != nil { return nil, fmt.Errorf("failed to read YAML file: %w", err) } - - // Use envsubst command to expand environment variables - cmd := exec.Command("envsubst") - cmd.Stdin = bytes.NewReader(content) - - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - if err := cmd.Run(); err != nil { - return nil, fmt.Errorf("envsubst failed: %w (stderr: %s)", err, stderr.String()) - } - - return stdout.Bytes(), nil + + return []byte(os.ExpandEnv(string(content))), nil }If adopted,
bytesbecomes unused and can be removed from the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helper/adapter.go` around lines 377 - 396, The function expandEnvVarsInYAMLToBytes currently shells out to envsubst; replace that by reading the file into content and using os.ExpandEnv on the file string (e.g., processed := os.ExpandEnv(string(content))) then return []byte(processed) to perform in-process ${VAR} expansion; remove usage/imports of exec and bytes (and any now-unused imports) and ensure error handling still returns the original read error from os.ReadFile, keeping the same function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/adapter/adapter_failover.go`:
- Around line 191-194: Remove the brittle substring assertion that pins the
error to the word "invalid" and instead assert the contract via the condition
fields: when checking availableCondition use
g.Expect(availableCondition.Status).To(Equal(metav1.ConditionFalse)) (or
equivalent), assert availableCondition.Reason is non-nil/non-empty and
availableCondition.Message is non-nil/non-empty; specifically replace the
g.Expect(*availableCondition.Message).To(ContainSubstring("invalid")) check with
assertions that Message and Reason are present (and not empty) so the test
doesn't depend on Kubernetes wording.
In `@pkg/helper/git.go`:
- Around line 109-111: The git clone error path currently returns without
removing the temporary componentDir; update the error branch that checks
cmd.CombinedOutput() so it invokes cleanup() for componentDir before returning
the formatted error (or arrange a defer cleanup() immediately after componentDir
is created so it always runs on error and is canceled/avoided on the successful
return). Modify the code around cmd.CombinedOutput()/componentDir/cleanup() to
ensure cleanup() is called on clone failure and still preserve the successful
return behavior.
---
Nitpick comments:
In `@pkg/helper/adapter.go`:
- Around line 273-353: The code currently reuses cmdCtx (created by cmdCtx,
cancel := context.WithTimeout(ctx, 30*time.Second)) for the initial pod list and
every exec.CommandContext (podLogCmd, podDescCmd), which can exhaust the 30s
budget and cancel later kubectl calls; update the pod-level diagnostics to
create a fresh short-lived context per external call (e.g., for podLogCmd and
podDescCmd use callCtx, callCancel := context.WithTimeout(ctx, 5*time.Second);
defer callCancel() immediately after creation) and pass callCtx to
exec.CommandContext instead of cmdCtx, ensuring the initial list still uses the
30s cmdCtx while each kubectl logs/describe gets its own timeout.
- Around line 377-396: The function expandEnvVarsInYAMLToBytes currently shells
out to envsubst; replace that by reading the file into content and using
os.ExpandEnv on the file string (e.g., processed :=
os.ExpandEnv(string(content))) then return []byte(processed) to perform
in-process ${VAR} expansion; remove usage/imports of exec and bytes (and any
now-unused imports) and ensure error handling still returns the original read
error from os.ReadFile, keeping the same function signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63635d38-4ef5-49b8-b8e5-683b893dbbd4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.gitignoreMakefilee2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/cluster/creation.goe2e/nodepool/creation.gogo.modpkg/config/config.gopkg/helper/adapter.gopkg/helper/constants.gopkg/helper/git.gopkg/helper/helper.gotest-design/testcases/adapter-failover.mdtest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-invalid-resource/adapter-config.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-invalid-resource/values.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/adapter/adapter_with_maestro.go
- testdata/adapter-configs/cl-invalid-resource/adapter-config.yaml
- e2e/cluster/creation.go
- testdata/adapter-configs/cl-invalid-resource/values.yaml
- pkg/helper/helper.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores