harden helm pending checks and CRD coverage

docs/comparison.md

@@ -36,8 +36,11 @@ Kubernetes work. They **move it** to a platform team or to a server that runs
 inside your cluster.
 
 > **An honest note:** you do not need Helm to *use* Deployah. But the output is a
-> real Helm release, so a little Helm knowledge helps if you want to debug deeply.
-> For example, you can run `helm history` or `helm get` on what Deployah installed.
+> real Helm release, so a little Helm knowledge helps when you debug the release
+> itself (for example `helm history` or `helm get`). Custom CRDs under
+> `.deployah/crds/` are another place the abstraction thins: they are applied
+> outside the Helm release, with their own install policy. See
+> [Custom manifests and CRDs](custom-manifests-and-crds.md).
 
 ## How much Helm you need (from most to least)
 

docs/custom-manifests-and-crds.md

@@ -126,4 +126,14 @@ then applies the Helm release. If the Helm plan has no changes but
 left alone). CRDs are never pruned and are never deleted on uninstall. Extra
 manifests leave with the release.
 
+## Failure modes
+
+CRD apply and the Helm release are **not** one atomic operation. Deployah
+applies CRDs first, waits for `Established`, then installs or upgrades the
+release. If the Helm step fails after CRDs succeed, those CRDs stay in the
+cluster (Deployah never rolls them back). If CRD apply fails, Deployah does
+not call Helm. Re-run `deployah deploy` after fixing the failure; already
+present CRDs are left alone under `--crds create`, or updated under
+`--crds create-replace`.
+
 See the [README](../README.md) for the project overview and the other guides.

internal/cmd/deploy/deploy_flow_test.go

@@ -15,6 +15,7 @@
 package deploy
 
 import (
+	"errors"
 	"path/filepath"
 	"testing"
 
@@ -396,6 +397,34 @@ func TestApplyDeploy_CallsInstallAfterEmptyCRDs(t *testing.T) {
 	assert.Contains(t, stderr.String(), "Deployed")
 }
 
+// TestApplyDeploy_PropagatesInstallErrorAfterCRDStep locks ordering: the CRD
+// step runs first (empty list is a successful no-op), then InstallApp. A
+// Helm failure after that step is returned to the caller. CRD survival
+// across deployah delete is covered by the e2e CRD lifecycle test.
+func TestApplyDeploy_PropagatesInstallErrorAfterCRDStep(t *testing.T) {
+	t.Parallel()
+	manifest := deployFlowManifestV1
+	stub := &stubHelmClient{
+		renderResults: []*render.RenderResult{testRenderResult(manifest)},
+		installErr:    errors.New("helm boom"),
+	}
+	cluster := newClusterWithStub(t, stub, nil)
+	sess := cluster.Session
+	planned := &deployPlan{
+		diff:    &planengine.Plan{Header: planengine.Header{Release: "web-production", Revision: 1}},
+		result:  testRenderResult(manifest),
+		cleanup: func() {},
+	}
+	c := nabatContext(t)
+	opts := &Options{Environment: "production", CRDs: string(extras.PolicyCreate)}
+
+	err := applyDeploy(c, sess, cluster, stub, nil, &spec.Spec{Project: "web"}, opts, nil, planned, nil, assertNever{}, &extras.Bundle{}, nil, nil)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "deploy failed")
+	assert.Contains(t, err.Error(), "helm boom")
+	assert.Equal(t, 1, stub.installCallCount, "InstallApp must run after the CRD step")
+}
+
 // TestApplyDeploy_PropagatesCRDApplyError skips InstallApp when CRDs fail.
 func TestApplyDeploy_PropagatesCRDApplyError(t *testing.T) {
 	t.Parallel()

internal/e2e/e2e_test.go

@@ -17,9 +17,11 @@
 package e2e_test
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -45,6 +47,8 @@ import (
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -245,6 +249,84 @@ func (s *E2ESuite) TestStatefulScale() {
 	assert.GreaterOrEqual(t, matched, 2, "expected per-pod PVCs after scale-up")
 }
 
+const crdLifecycleName = "clusterwidgets.example.com"
+
+// TestCRDLifecycle covers CRD apply outside the Helm release: Established
+// before install, idle-Helm re-apply, create vs create-replace, and survival
+// across deployah delete.
+func (s *E2ESuite) TestCRDLifecycle() {
+	t := s.T()
+	src := filepath.Join(s.testdataDir, "crd-lifecycle")
+	require.DirExists(t, src)
+
+	dir := t.TempDir()
+	copyTree(t, src, dir)
+	t.Chdir(dir)
+
+	ext := newApiextensionsClient(t, s.kcPath, "kind-deployah")
+	t.Cleanup(func() {
+		// t.Context() is canceled just before Cleanup runs (Go 1.24+), so
+		// teardown API calls need an independent context.
+		cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+		defer cancel()
+		// Best-effort: remove the fixture CRD so later suite runs stay clean.
+		if delCRDErr := ext.ApiextensionsV1().CustomResourceDefinitions().Delete(
+			cleanupCtx, crdLifecycleName, metav1.DeleteOptions{}); delCRDErr != nil {
+			t.Logf("cleanup CRD delete failed (non-fatal): %v", delCRDErr)
+		}
+		if delErr := runErr(t, "delete", "crd-lifecycle", "dev",
+			"--yes", "--wait", "--allow-missing-platform",
+			"--context", "kind-deployah"); delErr != nil {
+			t.Logf("cleanup delete failed (non-fatal): %v", delErr)
+		}
+	})
+
+	// First deploy installs the CRD and the release.
+	run(t, "deploy", "dev", "--context", "kind-deployah", "--yes", "--crds", "create")
+	crd := waitCRDEstablished(t, ext, crdLifecycleName)
+	assert.Equal(t, "crd-lifecycle", crd.Labels["e2e-marker"])
+
+	// Idle Helm plan must still visit CRDs (already present). Success messages
+	// go to stderr via nabat, so assert via a dedicated IO capture.
+	_, stderr := runCapture(t, "deploy", "dev", "--context", "kind-deployah", "--yes", "--crds", "create")
+	assert.Contains(t, stderr, "already present")
+	waitCRDEstablished(t, ext, crdLifecycleName)
+
+	// --crds create leaves an existing CRD alone when the file changes.
+	patched := strings.Replace(
+		readFixtureFile(t, filepath.Join(dir, ".deployah", "crds", "clusterwidget.yaml")),
+		`e2e-marker: "crd-lifecycle"`,
+		`e2e-marker: "create-skipped"`,
+		1,
+	)
+	require.NoError(t, os.WriteFile(
+		filepath.Join(dir, ".deployah", "crds", "clusterwidget.yaml"),
+		[]byte(patched), 0o600))
+	run(t, "deploy", "dev", "--context", "kind-deployah", "--yes", "--crds", "create")
+	crd = getCRD(t, ext, crdLifecycleName)
+	assert.Equal(t, "crd-lifecycle", crd.Labels["e2e-marker"],
+		"--crds create must not replace an existing CRD")
+
+	// --crds create-replace server-side-applies over the existing CRD.
+	run(t, "deploy", "dev", "--context", "kind-deployah", "--yes", "--crds", "create-replace")
+	require.NoError(t, wait.For(func(ctx context.Context) (bool, error) {
+		live, getErr := ext.ApiextensionsV1().CustomResourceDefinitions().Get(
+			ctx, crdLifecycleName, metav1.GetOptions{})
+		if getErr != nil {
+			return false, getErr
+		}
+		return live.Labels["e2e-marker"] == "create-skipped", nil
+	}, wait.WithTimeout(2*time.Minute), wait.WithInterval(time.Second)))
+
+	// CRDs are never pruned on uninstall.
+	run(t, "delete", "crd-lifecycle", "dev",
+		"--yes", "--wait", "--allow-missing-platform",
+		"--context", "kind-deployah")
+	_, err := ext.ApiextensionsV1().CustomResourceDefinitions().Get(
+		t.Context(), crdLifecycleName, metav1.GetOptions{})
+	require.NoError(t, err, "CRD must survive deployah delete")
+}
+
 // TestDeployScenarios deploys each discovered fixture and asserts expect.yaml.
 func (s *E2ESuite) TestDeployScenarios() {
 	for _, sc := range s.scenarios {
@@ -492,18 +574,108 @@ func loadExpectations(t testing.TB, dir string) expectations {
 
 func run(t testing.TB, args ...string) string {
 	t.Helper()
-	io, _, out, errOut := nabattest.NewIO()
-	app := cmd.NewApp(nabat.WithIO(io))
+	stdout, _ := runCapture(t, args...)
+	return stdout
+}
+
+// runCapture runs deployah and returns stdout and stderr on success.
+func runCapture(t testing.TB, args ...string) (stdout, stderr string) {
+	t.Helper()
+	appIO, _, out, errOut := nabattest.NewIO()
+	app := cmd.NewApp(nabat.WithIO(appIO))
 	err := nabattest.Run(t, app, args)
 	require.NoErrorf(t, err, "deployah %s\nstderr:\n%s",
 		strings.Join(args, " "), errOut.String())
-	return out.String()
+	return out.String(), errOut.String()
+}
+
+func copyTree(t testing.TB, src, dst string) {
+	t.Helper()
+	err := filepath.WalkDir(src, func(path string, d os.DirEntry, walkErr error) error {
+		if walkErr != nil {
+			return walkErr
+		}
+		rel, relErr := filepath.Rel(src, path)
+		if relErr != nil {
+			return relErr
+		}
+		target := filepath.Join(dst, rel)
+		if d.IsDir() {
+			return os.MkdirAll(target, 0o750)
+		}
+		in, openErr := os.Open(path) // #nosec G304 -- path under testdata/
+		if openErr != nil {
+			return openErr
+		}
+		defer in.Close()                                                                 //nolint:errcheck // read-only copy helper
+		out, createErr := os.OpenFile(target, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) // #nosec G304 -- temp fixture copy
+		if createErr != nil {
+			return createErr
+		}
+		_, copyErr := io.Copy(out, in)
+		closeErr := out.Close()
+		if copyErr != nil {
+			return copyErr
+		}
+		return closeErr
+	})
+	require.NoError(t, err)
+}
+
+func readFixtureFile(t testing.TB, path string) string {
+	t.Helper()
+	raw, err := os.ReadFile(path) // #nosec G304 -- path under test-controlled temp dir
+	require.NoError(t, err)
+	return string(raw)
+}
+
+func newApiextensionsClient(t testing.TB, kubeconfigPath, contextName string) apiextensionsclient.Interface {
+	t.Helper()
+	rules := clientcmd.NewDefaultClientConfigLoadingRules()
+	rules.ExplicitPath = kubeconfigPath
+	overrides := &clientcmd.ConfigOverrides{CurrentContext: contextName}
+	restCfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
+		rules, overrides).ClientConfig()
+	require.NoError(t, err)
+	cs, err := apiextensionsclient.NewForConfig(restCfg)
+	require.NoError(t, err)
+	return cs
+}
+
+func getCRD(t testing.TB, ext apiextensionsclient.Interface, name string) *apiextensionsv1.CustomResourceDefinition {
+	t.Helper()
+	crd, err := ext.ApiextensionsV1().CustomResourceDefinitions().Get(
+		t.Context(), name, metav1.GetOptions{})
+	require.NoError(t, err)
+	return crd
+}
+
+func waitCRDEstablished(t testing.TB, ext apiextensionsclient.Interface, name string) *apiextensionsv1.CustomResourceDefinition {
+	t.Helper()
+	var latest *apiextensionsv1.CustomResourceDefinition
+	require.NoError(t, wait.For(func(ctx context.Context) (bool, error) {
+		crd, err := ext.ApiextensionsV1().CustomResourceDefinitions().Get(
+			ctx, name, metav1.GetOptions{})
+		if err != nil {
+			return false, err
+		}
+		latest = crd
+		for _, cond := range crd.Status.Conditions {
+			if cond.Type == apiextensionsv1.Established &&
+				cond.Status == apiextensionsv1.ConditionTrue {
+				return true, nil
+			}
+		}
+		return false, nil
+	}, wait.WithTimeout(2*time.Minute), wait.WithInterval(time.Second)))
+	require.NotNil(t, latest)
+	return latest
 }
 
 func runErr(t testing.TB, args ...string) error {
 	t.Helper()
-	io, _, _, errOut := nabattest.NewIO()
-	app := cmd.NewApp(nabat.WithIO(io))
+	appIO, _, _, errOut := nabattest.NewIO()
+	app := cmd.NewApp(nabat.WithIO(appIO))
 	err := nabattest.Run(t, app, args)
 	if err == nil {
 		return nil

internal/e2e/testdata/crd-lifecycle/.deployah/crds/clusterwidget.yaml

@@ -0,0 +1,24 @@
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: clusterwidgets.example.com
+  labels:
+    e2e-marker: "crd-lifecycle"
+spec:
+  group: example.com
+  scope: Cluster
+  names:
+    kind: ClusterWidget
+    plural: clusterwidgets
+    singular: clusterwidget
+  versions:
+    - name: v1
+      served: true
+      storage: true
+      schema:
+        openAPIV3Schema:
+          type: object
+          properties:
+            spec:
+              type: object
+              x-kubernetes-preserve-unknown-fields: true

internal/e2e/testdata/crd-lifecycle/deployah.yaml

@@ -0,0 +1,10 @@
+apiVersion: v1-alpha.4
+project: crd-lifecycle
+components:
+  web:
+    image: nginx:latest
+    port: 80
+    environments: [dev]
+    resourcePreset: small
+environments:
+  dev: {}

internal/helm/cache_test.go

@@ -194,3 +194,49 @@ func TestPrepareChart_CanceledContextReturnsImmediately(t *testing.T) {
 	_, err := PrepareChart(ctx, &spec.Spec{Project: "x"}, "prod", nil, NewChartCache(time.Hour))
 	require.ErrorIs(t, err, context.Canceled)
 }
+
+// TestGenerateKey_ResolvedSpecContentInvalidates verifies that when resolved
+// is non-nil, changes to resolved.Spec change the cache key. Callers must
+// keep the separate manifest parameter consistent with resolved.Spec (see
+// [PrepareChart]); a mismatched manifest would not invalidate the key on
+// its own.
+func TestGenerateKey_ResolvedSpecContentInvalidates(t *testing.T) {
+	t.Parallel()
+	cache := NewChartCache(time.Hour)
+
+	base := &spec.Spec{
+		APIVersion: "v1-alpha.4",
+		Project:    "cache-key",
+		Components: map[string]spec.Component{"web": serviceComponent()},
+	}
+	require.NoError(t, spec.FillSpecWithDefaults(base, "v1-alpha.4"))
+
+	changed := &spec.Spec{
+		APIVersion: "v1-alpha.4",
+		Project:    "cache-key",
+		Components: map[string]spec.Component{
+			"web": {
+				Role:  spec.ComponentRoleService,
+				Image: "my-app:v2",
+				Port:  8080,
+			},
+		},
+	}
+	require.NoError(t, spec.FillSpecWithDefaults(changed, "v1-alpha.4"))
+
+	resolvedA := &spec.ResolvedSpec{Spec: base, Env: spec.NormalizeEnv("production")}
+	resolvedB := &spec.ResolvedSpec{Spec: changed, Env: spec.NormalizeEnv("production")}
+
+	keyA, err := cache.GenerateKey(base, "production", resolvedA)
+	require.NoError(t, err)
+	keyB, err := cache.GenerateKey(base, "production", resolvedB)
+	require.NoError(t, err)
+	assert.NotEqual(t, keyA, keyB, "resolved.Spec content must be part of the cache key")
+
+	// Same resolved, different manifest argument: key is unchanged. This is
+	// why PrepareChart documents that manifest must match resolved.Spec.
+	keySameResolved, err := cache.GenerateKey(changed, "production", resolvedA)
+	require.NoError(t, err)
+	assert.Equal(t, keyA, keySameResolved,
+		"GenerateKey hashes resolved when non-nil, not the separate manifest parameter")
+}

internal/helm/generate.go

@@ -88,6 +88,12 @@ func GenerateReleaseName(projectName, environmentName string) string {
 // PrepareChart returns [context.Canceled] or [context.DeadlineExceeded]
 // immediately; chart expansion itself is not interrupted mid-flight.
 //
+// When resolved is non-nil, the cache key hashes resolved (including
+// [spec.ResolvedSpec.Spec]), not the separate manifest parameter. Callers
+// must pass a manifest consistent with resolved.Spec: chart rendering still
+// reads component names and project from manifest, so a mismatched pair
+// could reuse a stale chart.
+//
 // On a cache miss, every 10th entry may start a background goroutine that
 // removes expired cache directories; that work outlives this call.
 //