test(live): apikey CRUD, user lifecycle, template versions, cleanup policies, cluster CRUD#100
Merged
Merged
Conversation
…ions, cleanup policies, cluster CRUD Adds five new endpoint-group live tests covering the highest-blast-radius surfaces still missing from cli/test/live/. All tests are wire-shape focused (no real workloads created), follow the existing helpers/cleanup conventions in this package, and run cleanly under the CI api-only flow introduced in #99. New files: apikey_live_test.go - Create → list → revoke cycle against the calling user (whoami). - Locks the raw_key contract: sk_-prefixed, returned once, never in list. Was implicitly relied on by the CI bootstrap but never asserted. user_live_test.go - Register a throwaway user, list (admin-only path), disable, enable, reset-password, delete. Never operates on admin — locking out the caller would break the rest of the suite. template_versions_live_test.go - Publishes the same template twice (description-only change in between) to materialise two version snapshots, then exercises list → get → diff with shape assertions on left/right/chart_diffs. cleanup_policy_live_test.go - Full admin CRUD plus a dry-run execution. The condition "idle_days:9999" deliberately matches nothing so the run never mutates a real instance. cluster_lifecycle_live_test.go - Stub-cluster create → get → update → delete. IsDefault stays false so the test never disrupts requireCluster() for other tests. Exercises registry_* + image_pull_secret_name fields (the registry_password drift in PR #95 is the canonical example of why this surface needs a live test). Bonus findings surfaced during local validation against rancher-desktop (left as commented follow-ups, not blockers for this PR): - stackctl's UpdateTemplateRequest.Name is `omitempty` but the backend rejects PUT with "name is required" when omitted. Either drop the omitempty or relax the backend. - stackctl's CreateTemplateRequest has no `version` field, so the template-level Version is unsettable through the CLI — the version snapshot's `version` round-trips empty as a result. - Backend rejects kubeconfig_data unless KUBECONFIG_ENCRYPTION_KEY is configured (the CI compose env doesn't set it); kubeconfig_path works without that prerequisite. Verified locally against rancher-desktop k8s-stack-manager: full live suite passes (21 passed, 2 skipped by design, 0 failed). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds five new live-only Go tests (build tag: live) that perform end-to-end create/read/update/delete and workflow checks for API keys, users, clusters, cleanup policies, and template versioning against a live backend. ChangesLive Integration Tests for Core API Resources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 4
🤖 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 `@cli/test/live/apikey_live_test.go`:
- Around line 40-41: The test uses assert.Truef for the length check on
created.RawKey but then slices created.RawKey[:3], which can panic if the length
check fails; replace assert.Truef(t, len(created.RawKey) > len("sk_"), ...) with
require.Truef(t, len(created.RawKey) > len("sk_"), ...) so the test aborts on
failure and prevents the subsequent slice from panicking, and ensure the
testify/require import is available in the test file.
In `@cli/test/live/cleanup_policy_live_test.go`:
- Around line 22-106: Refactor TestLiveCleanupPolicy_CRUDAndDryRun into a
table-driven set of subtests: define a slice of test cases (e.g., []struct{name
string; doDryRun bool; ...}) and iterate with for _, tt := range cases { tt :=
tt; t.Run(tt.name, func(t *testing.T){ t.Parallel(); c := newLiveClient(t);
login(t,c); cluster := requireCluster(t,c); ... perform the same
create/list/update/run/delete flow but use tt fields to vary behavior (e.g.,
DryRun flag) and keep the existing cleanup via t.Cleanup calling
c.DeleteCleanupPolicy(created.ID) }); } so each subtest runs in parallel and
uses the tt := tt capture pattern; preserve all assertions and reference
functions CreateCleanupPolicy, ListCleanupPolicies, UpdateCleanupPolicy,
RunCleanupPolicy, DeleteCleanupPolicy and
types.Create/UpdateCleanupPolicyRequest to build requests.
In `@cli/test/live/user_live_test.go`:
- Around line 57-65: The loop that checks Disabled after calling
c.DisableUser(created.ID) may skip the assertion if the user isn't present;
change to find the user first from the slice returned by c.ListUsers() (same
find-first pattern used earlier), then require.NotNilf(t, user, "expected user
%s in list after disable") to fail the test if missing, and only then
assert.True(t, user.Disabled, "user must be marked disabled after DisableUser");
reference the variables/functions: c.DisableUser, c.ListUsers, created.ID, and
the local slice variable (after) when locating the user.
- Around line 67-75: The test should first locate the updated user in the
ListUsers result before asserting re-enabled state: after calling
c.EnableUser(created.ID) and ListUsers (variable after2), loop to find the user
with ID == created.ID, then use require.NotNilf to fail the test if not found,
and only then assert that foundUser.Disabled is false; update the block that
currently iterates over after2 to follow the find-first pattern (use EnableUser,
after2, created.ID, and Disabled to locate and assert).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cdff74fd-6ca7-4961-b044-ba745658ac4e
📒 Files selected for processing (5)
cli/test/live/apikey_live_test.gocli/test/live/cleanup_policy_live_test.gocli/test/live/cluster_lifecycle_live_test.gocli/test/live/template_versions_live_test.gocli/test/live/user_live_test.go
Comment on lines
+22
to
+106
| func TestLiveCleanupPolicy_CRUDAndDryRun(t *testing.T) { | ||
| c := newLiveClient(t) | ||
| login(t, c) | ||
|
|
||
| cluster := requireCluster(t, c) | ||
| prefix := liveResourcePrefix() | ||
|
|
||
| // 1. Create — use the "stop" action and an idle_days condition that | ||
| // will never match in CI (no stack has been idle for 9999 days), so | ||
| // run --dry-run is guaranteed to return an empty result set. | ||
| created, err := c.CreateCleanupPolicy(&types.CreateCleanupPolicyRequest{ | ||
| Name: prefix + "-cleanup", | ||
| ClusterID: cluster.ID, | ||
| Action: "stop", | ||
| Condition: "idle_days:9999", | ||
| Schedule: "0 3 * * *", | ||
| Enabled: false, | ||
| DryRun: true, | ||
| }) | ||
| require.NoError(t, err, "create cleanup policy") | ||
| require.NotEmpty(t, created.ID, "created policy must have an ID") | ||
| assert.Equal(t, cluster.ID, created.ClusterID, "policy must echo cluster_id") | ||
| assert.Equal(t, "stop", created.Action, "policy must echo action") | ||
| assert.Equal(t, "idle_days:9999", created.Condition, "policy must echo condition") | ||
| assert.False(t, created.Enabled, "fresh policy must echo enabled=false") | ||
| assert.True(t, created.DryRun, "fresh policy must echo dry_run=true") | ||
|
|
||
| // Always best-effort delete so a failed assertion doesn't leave the | ||
| // policy in the cluster's schedule. | ||
| t.Cleanup(func() { | ||
| _ = c.DeleteCleanupPolicy(created.ID) | ||
| }) | ||
|
|
||
| // 2. List — newly-created policy must be visible. | ||
| policies, err := c.ListCleanupPolicies() | ||
| require.NoError(t, err, "list cleanup policies") | ||
| var found *types.CleanupPolicy | ||
| for i := range policies { | ||
| if policies[i].ID == created.ID { | ||
| found = &policies[i] | ||
| break | ||
| } | ||
| } | ||
| require.NotNilf(t, found, "newly-created cleanup policy %s must appear in list", created.ID) | ||
| assert.Equal(t, created.Name, found.Name, "list entry must echo name") | ||
|
|
||
| // 3. Update — flip the enabled flag. UpdateCleanupPolicy is a full PUT | ||
| // so we must re-send every field (the type comment in types.go calls | ||
| // this out explicitly). | ||
| updated, err := c.UpdateCleanupPolicy(created.ID, &types.UpdateCleanupPolicyRequest{ | ||
| Name: created.Name, | ||
| ClusterID: created.ClusterID, | ||
| Action: created.Action, | ||
| Condition: created.Condition, | ||
| Schedule: created.Schedule, | ||
| Enabled: true, | ||
| DryRun: created.DryRun, | ||
| }) | ||
| require.NoError(t, err, "update cleanup policy") | ||
| assert.True(t, updated.Enabled, "enabled flag must round-trip through PUT") | ||
|
|
||
| // 4. Run with dry_run=true — wire-shape assertion only. Backend will | ||
| // return an empty slice when nothing matches; that's fine. What | ||
| // matters is that the response decodes into []CleanupResult without | ||
| // dropping fields. | ||
| results, err := c.RunCleanupPolicy(created.ID, true) | ||
| require.NoError(t, err, "run cleanup policy (dry-run)") | ||
| require.NotNil(t, results, "results slice must be non-nil (may be empty)") | ||
| for i, r := range results { | ||
| // On a real match each entry must populate the action + | ||
| // status fields. Status MUST be one of the documented values. | ||
| assert.NotEmptyf(t, r.InstanceID, "results[%d].instance_id must be set", i) | ||
| assert.Containsf(t, []string{"success", "error", "dry_run"}, r.Status, | ||
| "results[%d].status %q must be one of the documented values", i, r.Status) | ||
| } | ||
|
|
||
| // 5. Delete (explicit — cleanup is the safety net). | ||
| require.NoError(t, c.DeleteCleanupPolicy(created.ID), "delete cleanup policy") | ||
| after, err := c.ListCleanupPolicies() | ||
| require.NoError(t, err, "list cleanup policies after delete") | ||
| for _, p := range after { | ||
| assert.NotEqualf(t, created.ID, p.ID, | ||
| "deleted policy %s must not appear in list", created.ID) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="cli/test/live/cleanup_policy_live_test.go"
echo "Verifying required test patterns in $f"
rg -n 'func Test|t\.Parallel\(|\[\]struct|t\.Run\(|tt := tt' "$f" || trueRepository: omattsson/stackctl
Length of output: 200
Refactor TestLiveCleanupPolicy_CRUDAndDryRun to table-driven subtests with t.Parallel()
cli/test/live/cleanup_policy_live_test.go currently contains only the single test function and no t.Parallel(), t.Run(...), table-driven []struct{...}, or tt := tt pattern.
Suggested refactor skeleton
func TestLiveCleanupPolicy_CRUDAndDryRun(t *testing.T) {
- c := newLiveClient(t)
- login(t, c)
+ t.Parallel()
+
+ tests := []struct {
+ name string
+ }{
+ {name: "crud-and-dry-run"},
+ }
+
+ for _, tt := range tests {
+ tt := tt
+ t.Run(tt.name, func(t *testing.T) {
+ t.Parallel()
+ c := newLiveClient(t)
+ login(t, c)
- cluster := requireCluster(t, c)
- prefix := liveResourcePrefix()
+ cluster := requireCluster(t, c)
+ prefix := liveResourcePrefix()
- // ... keep current test body unchanged ...
+ // ... keep current test body unchanged ...
+ })
+ }
}🤖 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 `@cli/test/live/cleanup_policy_live_test.go` around lines 22 - 106, Refactor
TestLiveCleanupPolicy_CRUDAndDryRun into a table-driven set of subtests: define
a slice of test cases (e.g., []struct{name string; doDryRun bool; ...}) and
iterate with for _, tt := range cases { tt := tt; t.Run(tt.name, func(t
*testing.T){ t.Parallel(); c := newLiveClient(t); login(t,c); cluster :=
requireCluster(t,c); ... perform the same create/list/update/run/delete flow but
use tt fields to vary behavior (e.g., DryRun flag) and keep the existing cleanup
via t.Cleanup calling c.DeleteCleanupPolicy(created.ID) }); } so each subtest
runs in parallel and uses the tt := tt capture pattern; preserve all assertions
and reference functions CreateCleanupPolicy, ListCleanupPolicies,
UpdateCleanupPolicy, RunCleanupPolicy, DeleteCleanupPolicy and
types.Create/UpdateCleanupPolicyRequest to build requests.
…okups Two correctness fixes from CodeRabbit on PR #100: - apikey_live_test.go: promote the raw_key length check from assert.Truef to require.Truef. A failing length check followed by created.RawKey[:3] would panic instead of failing cleanly. - user_live_test.go: after DisableUser/EnableUser, look up the created user in the list response first (find-first pattern, matched by the other live tests) and require.NotNil before asserting on the flag. The previous range-and-skip would silently pass if the user was missing from the response. The companion table-driven + t.Parallel() refactor suggestion for cleanup_policy_live_test.go is deliberately skipped — same reason as on PR #99: every other *_live_test.go file in this package uses ad-hoc subtests and runs serially against a shared backend by design. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Actionable comments posted: 0 |
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
Adds five new endpoint-group live tests covering the highest-blast-radius surfaces still missing from
cli/test/live/. All wire-shape focused (no real workloads), all clean up after themselves, all pass under both rancher-desktop and the CI api-only flow.What's covered
apikey_live_test.goraw_key(sk_-prefixed, returned once) contract that the CI bootstrap implicitly depends on but never asserted.user_live_test.gotemplate_versions_live_test.goleft/right/chart_diffs.cleanup_policy_live_test.goidle_days:9999deliberately matches nothing so the run can't mutate a real instance.cluster_lifecycle_live_test.goIsDefaultstays false to avoid disruptingrequireCluster()for other tests. Covers theregistry_*/image_pull_secret_namefields whose drift triggered #95.Bonus findings (left as commented follow-ups, not blockers)
Validating these five tests against rancher-desktop surfaced three stackctl/backend contract gaps — exactly what this layer is for:
UpdateTemplateRequest.Nameisomitempty, but the backend rejects PUT with"name is required"when omitted. Either drop theomitemptyor relax the server-side requirement.CreateTemplateRequesthas noversionfield, so the template-levelVersionis unsettable from the CLI — the version snapshot'sversionround-trips empty as a result.kubeconfig_dataunlessKUBECONFIG_ENCRYPTION_KEYis set;kubeconfig_pathworks without that prerequisite (used in the cluster lifecycle test).Each is called out inline with a
Note:comment explaining the workaround.Verification
Full live suite against rancher-desktop k8s-stack-manager:
Once PR #99 (the live-tests CI workflow) merges, these tests will also run on every PR via the same job.
Test plan
liveworkflow from ci(live): run cli/test/live/ on every PR against a freshly-booted backend #99 picks these up automatically)t.CleanupDELETE🤖 Generated with Claude Code
Summary by CodeRabbit