Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions cli/test/live/apikey_live_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
//go:build live

package live

import (
"testing"
"time"

"github.com/omattsson/stackctl/cli/pkg/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestLiveAPIKey_CRUD locks the API-key wire contract end-to-end:
// create → list → revoke. Catches drift on the highest-blast-radius
// surface — a field-name regression here would mask every other
// authenticated test (CI mints its key via this exact flow).
//
// We scope every key to the calling user (whoami) and revoke it on
// cleanup so the test leaves no debris in the user's keyring.
func TestLiveAPIKey_CRUD(t *testing.T) {
c := newLiveClient(t)
login(t, c)

me, err := c.Whoami()
require.NoError(t, err, "whoami")
require.NotEmpty(t, me.ID, "whoami must return a populated user id")

prefix := liveResourcePrefix()
days := 1

// Create
created, err := c.CreateAPIKey(me.ID, &types.CreateAPIKeyRequest{
Name: prefix + "-apikey",
ExpiresInDays: &days,
})
require.NoError(t, err, "create api key")
require.NotEmpty(t, created.ID, "created key must have an ID")
require.NotEmpty(t, created.RawKey, "raw_key must be populated on create (this is the only time it's returned)")
// require (not assert) so a short response doesn't panic the slice below.
require.Truef(t, len(created.RawKey) > len("sk_"), "raw_key %q must be longer than the sk_ prefix", created.RawKey)
assert.Equal(t, "sk_", created.RawKey[:3], "raw_key must carry the sk_ prefix")
assert.NotEmpty(t, created.Prefix, "prefix must be set so the key shows up in list")
require.NotNil(t, created.ExpiresAt, "expires_at must echo back on create when expires_in_days was set")
assert.True(t, created.ExpiresAt.After(time.Now()), "expires_at must be in the future")

// Always best-effort revoke so a failed assertion doesn't leave the
// key around the calling user's keyring.
t.Cleanup(func() {
_ = c.DeleteAPIKey(me.ID, created.ID)
})

// List — the new key must be visible. Raw key MUST NOT come back on list.
keys, err := c.ListAPIKeys(me.ID)
require.NoError(t, err, "list api keys")
var found *types.APIKey
for i := range keys {
if keys[i].ID == created.ID {
found = &keys[i]
break
}
}
require.NotNilf(t, found, "newly-created key %s must appear in list", created.ID)
assert.Equal(t, created.Prefix, found.Prefix, "prefix must match between create and list responses")
assert.Equal(t, me.ID, found.UserID, "list entry must echo back the owning user id")

// Revoke (explicit — cleanup is the safety net).
require.NoError(t, c.DeleteAPIKey(me.ID, created.ID), "revoke api key")

// Confirm gone.
after, err := c.ListAPIKeys(me.ID)
require.NoError(t, err, "list api keys after revoke")
for _, k := range after {
assert.NotEqualf(t, created.ID, k.ID,
"revoked key %s must not appear in list", created.ID)
}
}
106 changes: 106 additions & 0 deletions cli/test/live/cleanup_policy_live_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
//go:build live

package live

import (
"testing"

"github.com/omattsson/stackctl/cli/pkg/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestLiveCleanupPolicy_CRUDAndDryRun exercises the cleanup-policy
// wire contract end-to-end: create → list → update → run --dry-run →
// delete. Admin-gated on the backend; the CI suite runs as the
// env-seeded admin so all five verbs are reachable.
//
// The run is always invoked with dry_run=true so we never mutate stack
// instances — only the response wire shape ([]CleanupResult) is checked.
// Empty results are fine: the seed backend rarely has stacks matching
// "idle_days:9999", which is the deliberately-impossible condition.
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)
}
}
Comment on lines +22 to +106

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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" || true

Repository: 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.

89 changes: 89 additions & 0 deletions cli/test/live/cluster_lifecycle_live_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
//go:build live

package live

import (
"testing"

"github.com/omattsson/stackctl/cli/pkg/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestLiveCluster_CreateUpdateDelete locks the cluster-CRUD wire
// contract end-to-end: create → get → update → delete. The shaped
// fields (registry_password, image_pull_secret_name, max_namespaces,
// etc.) drifted twice during the epic #59 work — this catches that
// class of regression on the lifecycle path.
//
// IsDefault is deliberately set to false on both create and update —
// flipping the default would disrupt every other test that resolves
// the default cluster via requireCluster().
func TestLiveCluster_CreateUpdateDelete(t *testing.T) {
c := newLiveClient(t)
login(t, c)

prefix := liveResourcePrefix()
maxNS := 5

// 1. Create — registry fields are exercised because the
// registry_password drift in PR #95 is the canonical example of why
// this surface needs a live test.
// Backend requires api_server_url AND one of {kubeconfig_data,
// kubeconfig_path, use_in_cluster}. We use kubeconfig_path because
// kubeconfig_data is rejected unless KUBECONFIG_ENCRYPTION_KEY is
// configured (often not set in dev/CI). The path string is only
// validated for non-emptiness on create — it isn't dereferenced
// until a deploy actually runs, which we never trigger here.
created, err := c.CreateCluster(&types.CreateClusterRequest{
Name: prefix + "-cluster",
Description: "live-test stub cluster",
APIServerURL: "https://ci-stub-cluster.invalid:6443",
KubeconfigPath: "/dev/null/ci-stub-kubeconfig",
Region: "ci-region",
MaxNamespaces: maxNS,
IsDefault: false,
UseInCluster: false,
RegistryURL: "registry.example.com",
RegistryUsername: "ci-test",
RegistryPassword: "ci-test-password",
ImagePullSecretName: "ci-test-pull-secret",
})
require.NoError(t, err, "create cluster")
require.NotEmpty(t, created.ID, "created cluster must have an ID")
assert.Equal(t, prefix+"-cluster", created.Name, "name must round-trip")

// Always best-effort delete so a failed assertion leaves no debris.
t.Cleanup(func() {
_ = c.DeleteCluster(created.ID)
})

// 2. Get — round-trip every field that has historically drifted.
// We can't assert registry_password because the backend treats it
// as write-only (json:"-" on the read path).
got, err := c.GetCluster(created.ID)
require.NoError(t, err, "get cluster by ID")
assert.Equal(t, created.ID, got.ID, "id must round-trip via GET")
assert.Equal(t, "live-test stub cluster", got.Description, "description must round-trip via GET")
assert.False(t, got.IsDefault, "fresh non-default cluster must echo is_default=false")

// 3. Update — flip the description, leave the rest alone. The CLI
// uses pointer fields on UpdateClusterRequest so unset fields are
// not sent. We re-send the registry fields to confirm the
// registry_password update path doesn't 400 on an unchanged value.
newDesc := "live-test stub cluster (updated)"
pw := "ci-test-password" // intentionally unchanged
updated, err := c.UpdateCluster(created.ID, &types.UpdateClusterRequest{
Description: &newDesc,
RegistryPassword: &pw,
})
require.NoError(t, err, "update cluster")
assert.Equal(t, newDesc, updated.Description, "description must round-trip through PUT")

// 4. Delete (explicit — cleanup is the safety net).
require.NoError(t, c.DeleteCluster(created.ID), "delete cluster")

// Confirm gone — GET should 404.
_, err = c.GetCluster(created.ID)
require.Error(t, err, "GetCluster on deleted cluster must fail")
}
102 changes: 102 additions & 0 deletions cli/test/live/template_versions_live_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
//go:build live

package live

import (
"testing"

"github.com/omattsson/stackctl/cli/pkg/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestLiveTemplate_VersionsListGetDiff exercises the template-version
// wire contract: list → get → diff. Each publish creates a new version
// snapshot server-side, so we publish twice (with a description change
// in between) to guarantee at least two diffable versions.
//
// Coverage rationale: the versions endpoints carry a custom envelope
// (TemplateVersionDetail wraps TemplateVersion + Snapshot; the diff
// response is left/right/chart_diffs) — wire-shape regressions on any
// of these would silently break `template versions diff` in the CLI.
func TestLiveTemplate_VersionsListGetDiff(t *testing.T) {
c := newLiveClient(t)
login(t, c)

prefix := liveResourcePrefix()

// Create a throwaway template with one chart so the snapshot is
// non-empty. Note: stackctl's CreateTemplateRequest has no `version`
// field, so the template-level Version stays empty and the diff
// response's left.version / right.version come back empty too. We
// assert on the snapshot's template name (always populated) rather
// than .version, and call out the gap below.
tmpl, err := c.CreateTemplate(&types.CreateTemplateRequest{
Name: prefix + "-versions",
Description: "live-test versions fixture (v1)",
Charts: []types.ChartConfig{
{ChartName: "noop-a", RepoURL: "", ChartVersion: "0.1.0"},
},
})
require.NoError(t, err, "create template")
require.NotEmpty(t, tmpl.ID, "created template must have an ID")
deleteTemplateIfExists(t, c, tmpl.ID)

// First publish → version 1.
_, err = c.PublishTemplate(tmpl.ID)
require.NoError(t, err, "publish template (v1)")

// Update description so the second snapshot differs from the first.
// Backend rejects PUT /api/v1/templates/:id with `name is required`
// when Name is empty (the stackctl type has json:"name,omitempty" but
// the backend treats it as required). Echo the existing name to keep
// it a description-only change. Worth a follow-up: tighten stackctl's
// UpdateTemplateRequest to drop the omitempty on Name, or relax the
// backend.
_, err = c.UpdateTemplate(tmpl.ID, &types.UpdateTemplateRequest{
Name: tmpl.Name,
Description: "live-test versions fixture (v2)",
})
require.NoError(t, err, "update template")

// Second publish → version 2.
_, err = c.PublishTemplate(tmpl.ID)
require.NoError(t, err, "publish template (v2)")

// 1. List — must return both snapshots.
versions, err := c.ListTemplateVersions(tmpl.ID)
require.NoError(t, err, "list template versions")
require.GreaterOrEqual(t, len(versions), 2,
"two publishes must produce at least two version rows (got %d)", len(versions))
for i, v := range versions {
assert.Equal(t, tmpl.ID, v.TemplateID, "versions[%d].template_id must echo the template", i)
assert.NotEmpty(t, v.ID, "versions[%d].id must be populated", i)
assert.False(t, v.CreatedAt.IsZero(), "versions[%d].created_at must decode", i)
}

// 2. Get version detail — TemplateVersionDetail = TemplateVersion + Snapshot.
left := versions[len(versions)-1] // oldest (list is typically newest-first)
right := versions[0] // newest
detail, err := c.GetTemplateVersion(tmpl.ID, right.ID)
require.NoError(t, err, "get template version detail")
assert.Equal(t, right.ID, detail.ID, "detail must echo the requested version id")
assert.NotEmpty(t, detail.Snapshot.Template.Name, "snapshot.template.name must decode")
assert.Len(t, detail.Snapshot.Charts, 1, "snapshot must echo the one inline chart")
assert.Equal(t, "noop-a", detail.Snapshot.Charts[0].ChartName,
"snapshot chart name must round-trip")

// 3. Diff between the two snapshots — the description-only change is
// captured at the template level, not in chart_diffs, but the chart_diffs
// slice must decode (empty or not). What matters is the left/right
// envelope wire shape.
diff, err := c.DiffTemplateVersions(tmpl.ID, left.ID, right.ID)
require.NoError(t, err, "diff template versions")
require.NotNil(t, diff, "diff response must not be nil")
// diff.{left,right}.version mirrors the template-level Version which
// stackctl can't set today (CreateTemplateRequest has no version
// field — a follow-up gap). Assert on the snapshot.template.name
// instead, which always round-trips.
assert.NotEmpty(t, diff.Left.Snapshot.Template.Name, "diff.left.snapshot.template.name must decode")
assert.NotEmpty(t, diff.Right.Snapshot.Template.Name, "diff.right.snapshot.template.name must decode")
assert.NotNil(t, diff.ChartDiffs, "chart_diffs must be a populated slice, even if empty")
}
Loading