feat(workload): run ephemeral checks on Kind#6045
Conversation
📝 WalkthroughWalkthroughThe ephemeral validate and scan paths now provision isolated Kind clusters instead of KWOK clusters. Kind provisioning forwards dedicated expanded kubeconfig paths, while the ephemeral backend manages temporary workspace cleanup and bounded teardown across success, cancellation, and failure paths. Tests cover backend selection, kubeconfig isolation, lifecycle ordering, error joining, and CLI wording. Documentation and help text now describe Kind execution and operator-rendered child processing as the next off-by-default slice. Possibly related issues
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Comment |
✅MegaLinter analysis: Success✅ Linters with no issuesactionlint, bash-exec, git_diff, hadolint, jscpd, jsonlint, lychee, markdown-table-formatter, markdownlint, prettier, prettier, shellcheck, shfmt, stylelint, syft, trivy-sbom, trufflehog, v8r, v8r, yamllint Notices📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining See detailed reports in MegaLinter artifacts
|
@coderabbitai review Fresh full review requested after the adaptive review-limit window reopened. |
|
✅ Action performedReview finished.
|
![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
🤖 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/cli/cmd/workload/ephemeral.go`:
- Around line 162-171: In the Create failure branch of the ephemeral cluster
workflow, update cleanup handling so deleteEphemeralCluster’s ErrClusterNotFound
is suppressed while preserving any real cleanup errors and the original Create
error. Use the existing createErr handling and deleteEphemeralCluster symbols to
apply this only when provisioner.Create fails.
In `@pkg/svc/chat/docs_generated.go`:
- Line 7: Raw commas in jsonschema description tags under the chat documentation
sources are being interpreted as tag separators, truncating generated Markdown
table cells. Search the source files in pkg/svc/chat for comma-containing
jsonschema description tags, then remove or escape those commas while preserving
the intended wording; specifically verify descriptions corresponding to
distributionConfig, NodeAutoscalerConfig.enabled, and expander. Regenerate docs
with go generate ./pkg/svc/chat/... and do not edit generatedDocumentation
directly.
In `@pkg/svc/provisioner/cluster/kind/provisioner_test.go`:
- Around line 89-106: Add a complementary test next to
TestCreateIncludesExpandedKubeconfigFlag that leaves kubeConfig empty, invokes
provisioner.Create, and asserts success plus absence of the "--kubeconfig"
argument in runner.lastArgs, covering the empty-path branch in Create.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9d481bf-bcfe-4428-8e3b-ddb16849bc52
⛔ Files ignored due to path filters (1)
pkg/cli/cmd/workload/__snapshots__/workload_test.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
docs/src/content/docs/cli-flags/workload/workload-scan.mdxdocs/src/content/docs/cli-flags/workload/workload-validate.mdxpkg/cli/cmd/workload/ephemeral.gopkg/cli/cmd/workload/ephemeral_install.gopkg/cli/cmd/workload/ephemeral_install_test.gopkg/cli/cmd/workload/ephemeral_test.gopkg/cli/cmd/workload/export_test.gopkg/cli/cmd/workload/scan.gopkg/cli/cmd/workload/validate.gopkg/svc/chat/docs_generated.gopkg/svc/provisioner/cluster/kind/provisioner.gopkg/svc/provisioner/cluster/kind/provisioner_test.gopkg/svc/provisioner/cluster/multi.gopkg/svc/provisioner/cluster/multi_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: CodeRabbit
- GitHub Check: 🔥 Warm Mirror Cache
- GitHub Check: 📝 Generate Schema, CRD & Docs
- GitHub Check: 🔥 Warm Helm Cache
- GitHub Check: 🏠 Home Isolation Guard
- GitHub Check: 🧪 Test
- GitHub Check: 📊 Code Coverage
- GitHub Check: 🧹 Lint - golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (3)
pkg/svc/chat/docs_generated.go
📄 CodeRabbit inference engine (AGENTS.md)
Do not hand-edit
pkg/svc/chat/docs_generated.go; regenerate it withgo generate ./pkg/svc/chat/...after the docs generation step.
Files:
pkg/svc/chat/docs_generated.go
docs/src/content/docs/{cli-flags/**,configuration/declarative-configuration.mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hand-edit the generated CLI reference docs under
docs/src/content/docs/cli-flags/ordocs/src/content/docs/configuration/declarative-configuration.mdx; regenerate them withgo generate ./docs/....
Files:
docs/src/content/docs/cli-flags/workload/workload-validate.mdxdocs/src/content/docs/cli-flags/workload/workload-scan.mdx
pkg/cli/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/cli/**/*.go: Canonicalize all user-supplied file path arguments in CLI commands withfsutil.EvalCanonicalPathbefore using them.
For output paths that may not already exist, create the parent directory withos.MkdirAll(filepath.Dir(outputPath), <mode>)before callingfsutil.EvalCanonicalPath.
Usefsutil.ReadFileSafefor constrained reads instead of reimplementing path-containment checks.
Files:
pkg/cli/cmd/workload/ephemeral_install_test.gopkg/cli/cmd/workload/ephemeral_install.gopkg/cli/cmd/workload/validate.gopkg/cli/cmd/workload/scan.gopkg/cli/cmd/workload/ephemeral.gopkg/cli/cmd/workload/export_test.gopkg/cli/cmd/workload/ephemeral_test.go
🧠 Learnings (1)
📚 Learning: 2026-07-10T20:56:28.132Z
Learnt from: devantler
Repo: devantler-tech/ksail PR: 6031
File: pkg/svc/chat/docs_generated.go:7-7
Timestamp: 2026-07-10T20:56:28.132Z
Learning: When reviewing code in `pkg/svc/chat/**` that uses `jsonschema:"description=..."` struct tags feeding the `go generate ./pkg/svc/chat/...` docs generator, avoid unescaped/raw commas inside the `description=` value. Struct tag options are comma-separated, so a raw comma will truncate the generated Markdown/MDX table cell text (often right before a `|`). The generator’s `mdxTableCell` correctly escapes literal `|` characters, so truncated output is usually from commas—not pipes. Fix by rewording the description to remove commas (or escaping them according to the tag parser’s rules), then regenerate the docs and ensure the related source-scan regression test passes.
Applied to files:
pkg/svc/chat/docs_generated.go
🔇 Additional comments (12)
pkg/cli/cmd/workload/ephemeral_install.go (1)
39-42: LGTM!pkg/cli/cmd/workload/ephemeral_install_test.go (1)
104-104: LGTM!Also applies to: 118-118
pkg/cli/cmd/workload/scan.go (1)
144-147: LGTM!Also applies to: 150-151
pkg/cli/cmd/workload/validate.go (1)
160-163: LGTM!Also applies to: 167-168
docs/src/content/docs/cli-flags/workload/workload-scan.mdx (1)
46-46: LGTM!docs/src/content/docs/cli-flags/workload/workload-validate.mdx (1)
42-42: LGTM!pkg/svc/provisioner/cluster/multi.go (1)
188-188: LGTM!Also applies to: 207-216
pkg/svc/provisioner/cluster/multi_test.go (1)
5-12: LGTM!Also applies to: 67-114
pkg/svc/provisioner/cluster/kind/provisioner.go (1)
228-236: LGTM!pkg/cli/cmd/workload/ephemeral.go (1)
61-121: LGTM!Also applies to: 132-133
pkg/cli/cmd/workload/export_test.go (1)
464-512: LGTM!pkg/cli/cmd/workload/ephemeral_test.go (1)
7-12: LGTM!Also applies to: 21-53, 75-100, 111-146, 167-251, 283-353, 377-441, 462-475
github-project-automation
Bot
moved this from 🫴 Ready
to 🏃🏻♂️ In Progress
in 🌊 Project Board
@coderabbitai run pre-merge checks Review fixes are pushed in Snapshot verification for the Linked Issues check: The generated chat-doc truncation noted in review predates this PR and is already fixed separately by promoted #6036, so duplicating its schema-source edits here would create conflicting scope. |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
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/svc/provisioner/cluster/multi_test.go (1)
67-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer public test seams over reflection in
pkg/svc/provisioner/cluster/multi_test.go
- Use
KubeConfigForTest()for the kubeconfig assertion instead of readingkindprovisioner.Provisioner.kubeConfigviareflect.- For
kindConfig, prefer an observableCreate()assertion with a command-runner double (or a small exported test helper) rather than accessing the private field directly.🤖 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/svc/provisioner/cluster/multi_test.go` around lines 67 - 118, Replace reflection-based inspection in TestCreateMinimalProvisioner_VanillaForwardsKubeconfigPath with the public KubeConfigForTest() accessor. Rewrite TestCreateMinimalProvisioner_VanillaBuildsValidKindConfig to verify the generated configuration through Create() using a command-runner double, or add and use a small exported test helper, instead of reading the private kindConfig field via reflect.
🤖 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/svc/provisioner/cluster/multi_test.go`:
- Around line 67-118: Replace reflection-based inspection in
TestCreateMinimalProvisioner_VanillaForwardsKubeconfigPath with the public
KubeConfigForTest() accessor. Rewrite
TestCreateMinimalProvisioner_VanillaBuildsValidKindConfig to verify the
generated configuration through Create() using a command-runner double, or add
and use a small exported test helper, instead of reading the private kindConfig
field via reflect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 597ede78-f2f3-4c67-a779-188e0f9f52d2
📒 Files selected for processing (5)
pkg/cli/cmd/workload/ephemeral.gopkg/cli/cmd/workload/ephemeral_test.gopkg/svc/provisioner/cluster/kind/provisioner.gopkg/svc/provisioner/cluster/kind/provisioner_test.gopkg/svc/provisioner/cluster/multi_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: 🏗️ Build KSail Binary
- GitHub Check: 🏠 Home Isolation Guard
- GitHub Check: 🧪 Test
- GitHub Check: 🧹 Lint - golangci-lint
- GitHub Check: 📊 Code Coverage
- GitHub Check: 🧹 Lint - mega-linter
- GitHub Check: 🏗️ Build
- GitHub Check: 🔍 Dead Code Analysis
- GitHub Check: 🛡️ Vulnerability Scan
- GitHub Check: 📦 Tidy
- GitHub Check: CodeRabbit
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (1)
pkg/cli/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/cli/**/*.go: Canonicalize all user-supplied file path arguments in CLI commands withfsutil.EvalCanonicalPathbefore using them.
For output paths that may not already exist, create the parent directory withos.MkdirAll(filepath.Dir(outputPath), <mode>)before callingfsutil.EvalCanonicalPath.
Usefsutil.ReadFileSafefor constrained reads instead of reimplementing path-containment checks.
Files:
pkg/cli/cmd/workload/ephemeral.gopkg/cli/cmd/workload/ephemeral_test.go
🔇 Additional comments (6)
pkg/svc/provisioner/cluster/kind/provisioner.go (1)
228-236: LGTM!pkg/svc/provisioner/cluster/kind/provisioner_test.go (1)
89-122: LGTM!Also applies to: 489-534
pkg/cli/cmd/workload/ephemeral.go (1)
135-211: LGTM!pkg/cli/cmd/workload/ephemeral_test.go (3)
168-252: LGTM!Also applies to: 357-381
404-477: 🩺 Stability & AvailabilityCancellation teardown is already covered. The cancellation case already asserts one
provisioner.Deletecall, one backend cleanup, and removal of the workspace directory.
112-146: 🎯 Functional CorrectnessCleanup path is sound.
@coderabbitai review The five changed declarations behind the 72.22% docstring result now have exact leading comments in c44870b, bringing the identified set to 18 of 18. The branch also includes current main at 0c59cfd. |
|
✅ Action performedReview finished.
|
@coderabbitai run pre-merge checks The five previously missing declaration comments are now present in c44870b, focused tests pass, exact-package lint reports zero issues, and the branch includes current main at 0c59cfd. |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
Outside-diff The maintainability concern is real, so #6049 captures a private factory-injection refactor that can remove both reflection assertions without changing public behavior. This PR remains focused on the ephemeral Kind workflow. Focused tests across the workload, cluster, and Kind packages pass (748 tests), and the historical docstring failure is stale: the current head documents all 21 relevant functions. |
@coderabbitai review Cooldown-bound retry for head 15c6399. The five changed declarations behind the prior 13-of-18 score now each have exact leading comments in c44870b; focused tests and exact-package lint are green. |
|
✅ Action performedReview finished.
|
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/cli/cmd/workload/ephemeral_test.go (1)
118-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn/parameterize the fake waiter for symmetry with the backend.
installEphemeralProvisionerunconditionally installs a fresh defaultfakeEphemeralWaiter{}but only returns the backend fake. Any test needing a custom wait error must know to callExportSetEphemeralClusterWaiteragain afterward to override it — an implicit, undocumented ordering dependency. Consider accepting an optional waiter or returning the installed waiter alongside the backend.♻️ Suggested refactor
-func installEphemeralProvisioner( +func installEphemeralProvisioner( t *testing.T, fake *fakeEphemeralProvisioner, -) *fakeEphemeralBackend { +) (*fakeEphemeralBackend, *fakeEphemeralWaiter) { t.Helper() ... - installEphemeralWaiter(t, &fakeEphemeralWaiter{}) + waiter := &fakeEphemeralWaiter{} + installEphemeralWaiter(t, waiter) - return backend + return backend, waiter }🤖 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/cli/cmd/workload/ephemeral_test.go` around lines 118 - 152, Update installEphemeralProvisioner to expose the fakeEphemeralWaiter it installs, either by accepting an optional waiter or returning it alongside the fakeEphemeralBackend. Ensure tests can configure custom wait errors through that returned or supplied waiter without re-registering ExportSetEphemeralClusterWaiter after the helper call.
🤖 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/cli/cmd/workload/ephemeral_test.go`:
- Around line 118-152: Update installEphemeralProvisioner to expose the
fakeEphemeralWaiter it installs, either by accepting an optional waiter or
returning it alongside the fakeEphemeralBackend. Ensure tests can configure
custom wait errors through that returned or supplied waiter without
re-registering ExportSetEphemeralClusterWaiter after the helper call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2839bd5-efd9-43a1-8f1d-8f953d767077
📒 Files selected for processing (3)
pkg/cli/cmd/workload/ephemeral_test.gopkg/svc/provisioner/cluster/kind/provisioner_test.gopkg/svc/provisioner/cluster/multi.go
📜 Review details
⏰ Context from checks skipped due to timeout. (21)
- GitHub Check: CodeRabbit
- GitHub Check: 🧪 System Test (Docker) (Vanilla, Docker, true, --cni Calico --csi Disabled --load-balancer Dis...
- GitHub Check: 🧪 System Test (Docker) (VCluster, Docker, true, --name system-test-cluster --cni Cilium --csi ...
- GitHub Check: 🧪 System Test (Docker) (VCluster, Docker, true, --gitops-engine Flux --local-registry ghcr.io/...
- GitHub Check: 🧪 System Test (Docker) (VCluster, Docker, true, --cni Calico --csi Disabled --load-balancer Di...
- GitHub Check: 🧪 System Test (Docker) (K3s, Docker, true, --cni Calico --csi Disabled --load-balancer Disable...
- GitHub Check: 🧪 System Test (Docker) (Vanilla, Docker, true, --gitops-engine ArgoCD --local-registry ghcr.io...
- GitHub Check: 🧪 System Test (Docker) (K3s, Docker, true, --gitops-engine ArgoCD --local-registry ghcr.io/dev...
- GitHub Check: 🧪 System Test (Docker) (VCluster, Docker, true)
- GitHub Check: 🧪 System Test (Docker) (K3s, Docker, true, --gitops-engine Flux --local-registry ghcr.io/devan...
- GitHub Check: 🧪 System Test (Docker) (Vanilla, Docker, true, --name system-test-cluster --cni Cilium --csi E...
- GitHub Check: 🧪 System Test (Docker) (K3s, Docker, true)
- GitHub Check: 🧪 System Test (Docker) (K3s, Docker, true, --name system-test-cluster --cni Cilium --csi Enabl...
- GitHub Check: 🧪 System Test (Docker) (Vanilla, Docker, true)
- GitHub Check: 🧪 System Test (Docker) (KWOK, Docker, true, --cni Calico --csi Disabled --load-balancer Disabl...
- GitHub Check: 🏠 Home Isolation Guard
- GitHub Check: 📊 Code Coverage
- GitHub Check: 🧪 Test
- GitHub Check: 🧹 Lint - golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (1)
pkg/cli/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/cli/**/*.go: Canonicalize all user-supplied file path arguments in CLI commands withfsutil.EvalCanonicalPathbefore using them.
For output paths that may not already exist, create the parent directory withos.MkdirAll(filepath.Dir(outputPath), <mode>)before callingfsutil.EvalCanonicalPath.
Usefsutil.ReadFileSafefor constrained reads instead of reimplementing path-containment checks.
Files:
pkg/cli/cmd/workload/ephemeral_test.go
🔇 Additional comments (5)
pkg/svc/provisioner/cluster/multi.go (1)
188-188: LGTM!Also applies to: 207-218
pkg/svc/provisioner/cluster/kind/provisioner_test.go (1)
489-490: LGTM!pkg/cli/cmd/workload/ephemeral_test.go (3)
41-42: LGTM!Also applies to: 52-53, 86-87
197-232: LGTM!Also applies to: 527-546
127-128: 🩺 Stability & AvailabilityNo issue here: the factory executes synchronously The backend factory is called directly inside
withEphemeralClusterbefore any goroutine is spawned, sorequire.NoErrorin this test closure does not violate thetestinggoroutine rule.> Likely an incorrect or invalid review comment.
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall coverage in the branch remains at 66%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
Resolved the latest outside-diff waiter-fixture finding in 90a08d7. installEphemeralProvisioner now accepts the readiness waiter directly (nil selects the ordinary no-op fake), and the lifecycle ordering, readiness, and wait-failure tests supply their custom waiter through that single helper call instead of depending on a later re-registration. Validation: env -u HCLOUD_TOKEN go test ./pkg/cli/cmd/workload passed; golangci-lint run ./pkg/cli/cmd/workload reports 0 issues; git diff --check passed. |
@coderabbitai review The outside-diff waiter-fixture finding from the 07:31Z review is addressed in 90a08d7 and its focused tests/lint are green. Please review the current head. |
|
✅ Action performedReview finished.
|
@coderabbitai review Cooldown-bound retry for current head 90a08d7 after resolving the waiter-fixture outside-diff finding. Focused tests, exact-package lint, and diff check are green. |
|
✅ Action performedReview finished.
|
@coderabbitai review The adaptive review window has reopened. Please review exact head |
|
✅ Action performedReview finished.
|
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 (2)
pkg/cli/cmd/workload/ephemeral_test.go (2)
420-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert workspace removal even when cleanup returns an injected error.
errors.Join(os.RemoveAll(backend.workspace), backend.cleanupErr)still callsos.RemoveAllregardless ofcleanupErr, so the workspace should be gone even thoughcleanupErris non-nil. Addingassert.NoDirExists(t, backend.workspace)here (alongside the existingbackend.cleanedcheck) would confirm cleanup always attempts removal even on the error-joining path, matching the coverage given to other failure-path tests in this 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/cli/cmd/workload/ephemeral_test.go` around lines 420 - 437, Extend TestWithEphemeralClusterJoinsFnDeleteAndCleanupErrors to assert that backend.workspace no longer exists after the joined-error cleanup path, alongside the existing backend.cleaned assertion. Use the test’s existing assertion style and preserve all current error checks.
336-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cleanup assertions to the NotFound-suppression test for consistency.
TestWithEphemeralClusterSkipsFnAndTearsDownWhenCreateFails(Lines 336-367) checksbackend.cleanedandassert.NoDirExists(t, backend.workspace)after a create failure, but the very next test,TestWithEphemeralClusterSuppressesNotFoundCleanupAfterCreateFailure(Lines 374-393), which also drives a create failure through the same backend, drops these assertions entirely (Line 380 discards the returned backend). Since this test specifically targets the NotFound-suppression branch of the create-failure path, verifying that backend cleanup still runs there closes a gap in coverage of the guarantee the PR explicitly requires ("Remove the cluster and temporary kubeconfig after success, errors, timeouts, cancellation...").♻️ Proposed fix
- installEphemeralProvisioner(t, fake, nil) + backend := installEphemeralProvisioner(t, fake, nil) err := workload.ExportWithEphemeralCluster( cmd.Context(), cmd, func(context.Context, workload.EphemeralCluster) error { t.Fatal("callback must not run after create failure") return nil }, ) require.ErrorIs(t, err, errEphemeralCreateFailed) assert.NotErrorIs(t, err, clustererr.ErrClusterNotFound) + assert.Equal(t, 1, backend.cleaned) + assert.NoDirExists(t, backend.workspace)🤖 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/cli/cmd/workload/ephemeral_test.go` around lines 336 - 393, Retain the backend returned by installEphemeralProvisioner in TestWithEphemeralClusterSuppressesNotFoundCleanupAfterCreateFailure, then assert that backend.cleaned equals 1 and backend.workspace no longer exists after ExportWithEphemeralCluster returns. Keep the existing assertions verifying the original create error is preserved and ErrClusterNotFound is suppressed.
🤖 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/cli/cmd/workload/ephemeral_test.go`:
- Around line 420-437: Extend
TestWithEphemeralClusterJoinsFnDeleteAndCleanupErrors to assert that
backend.workspace no longer exists after the joined-error cleanup path,
alongside the existing backend.cleaned assertion. Use the test’s existing
assertion style and preserve all current error checks.
- Around line 336-393: Retain the backend returned by
installEphemeralProvisioner in
TestWithEphemeralClusterSuppressesNotFoundCleanupAfterCreateFailure, then assert
that backend.cleaned equals 1 and backend.workspace no longer exists after
ExportWithEphemeralCluster returns. Keep the existing assertions verifying the
original create error is preserved and ErrClusterNotFound is suppressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78586fa1-094a-42b1-abf4-42267ceafc79
📒 Files selected for processing (1)
pkg/cli/cmd/workload/ephemeral_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pkg/cli/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/cli/**/*.go: Canonicalize all user-supplied file path arguments in CLI commands withfsutil.EvalCanonicalPathbefore using them.
For output paths that may not already exist, create the parent directory withos.MkdirAll(filepath.Dir(outputPath), <mode>)before callingfsutil.EvalCanonicalPath.
Usefsutil.ReadFileSafefor constrained reads instead of reimplementing path-containment checks.
Files:
pkg/cli/cmd/workload/ephemeral_test.go
🔇 Additional comments (1)
pkg/cli/cmd/workload/ephemeral_test.go (1)
7-13: LGTM!Also applies to: 22-58, 80-158, 174-218, 235-264, 271-298, 317-333, 443-461, 510-535
Current-head outside-diff cleanup findings: fixed in Validation: both focused tests pass, |
@coderabbitai run pre-merge checks Exact head |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
…-backend-6044 # Conflicts: # pkg/svc/chat/docs_generated.go
Current head Validation:
The latest two outside-diff cleanup findings remain resolved in the current file: both failure paths assert workspace removal, and the NotFound path also asserts one cleanup call. |
@coderabbitai review Please review exact head 80b6fbd. All 20 changed non-Test Go declarations now have matching leading documentation; the historical 72.22% value came from five gaps at 6afb526, and the two outside-diff cleanup-test findings from 90a08d7 are fixed in c51dc9a. Focused workload tests, scoped golangci-lint, gofmt, and diff validation pass. Please recompute the formal pre-merge summary against this current head. |
|
✅ Action performedReview finished.
|
@coderabbitai run pre-merge checks The fresh review completed on exact head 80b6fbd with no actionable comments. Please recompute the formal checks from the current diff: all 20 changed non-Test Go declarations have matching leading documentation, so the retained 72.22% value is stale against historical head 6afb526. |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
1 participant
Summary
workload validate/scan --ephemeralKWOK lifecycle with a controller-capable Kind backendWhy
KWOK can register CRDs but only simulates pod state; it never executes operator controllers. That made it impossible for the next Phase 3b slice to apply operator CRs and inspect the native children they reconcile. Kind supplies a real control plane and workload runtime while keeping
--ephemeralopt-in and default-off.Validation
go build -o /tmp/ksail-maint-6044 .go test ./pkg/cli/cmd/workload ./pkg/svc/provisioner/cluster/kind ./pkg/svc/provisioner/cluster -count=1golangci-lint run --timeout 5m ./pkg/cli/cmd/workload ./pkg/svc/provisioner/cluster/kind ./pkg/svc/provisioner/cluster(0 issues)npm run buildindocs/(193 pages, internal links valid)workload validate <empty-dir> --ephemeralprovisionedksail-ephemeral-1783748957305085000, reached readiness throughkind-ksail-ephemeral-1783748957305085000, and deleted its node successfullyTMPDIRenv -u HCLOUD_TOKEN go test -p 4 ./...passed every package except the existing load-sensitive Copilot fixture-process timing test; that package passed immediately in isolation withgo test ./pkg/cli/cmd/open/chat -count=1Follow-up
The next #5919 slice can apply rendered manifests, wait for real operator reconciliation, inventory generated children, and validate/scan them with per-layer attribution.
Fixes #6044
Part of #5919 and #5344