Do not create change when app-changes-max-to-keep=0

Existing behaviour for --app-changes-max-to-keep=0: - Create an app change before deployment begins, and garbage collect it at the end New behaviour: Do not create an app change at all, but still keep the last change details in the meta configmap. Signed-off-by: Praveen Rewar <[email protected]>
carvel-dev · Aug 28, 2023 · f3a88d5 · f3a88d5
1 parent 0d3d1df
commit f3a88d5
Show file tree

Hide file tree

Showing 8 changed files with 119 additions and 14 deletions.
diff --git a/pkg/kapp/app/change.go b/pkg/kapp/app/change.go
@@ -20,6 +20,8 @@ type ChangeImpl struct {
 	meta       ChangeMeta
 
 	createdAt time.Time
+
+	create bool
 }
 
 var _ Change = &ChangeImpl{}
@@ -55,6 +57,11 @@ func (c *ChangeImpl) Delete() error {
 }
 
 func (c *ChangeImpl) update(doFunc func(*ChangeMeta)) error {
+	if !c.create {
+		doFunc(&c.meta)
+		return nil
+	}
+
 	change, err := c.coreClient.CoreV1().ConfigMaps(c.nsName).Get(context.TODO(), c.name, metav1.GetOptions{})
 	if err != nil {
 		return fmt.Errorf("Getting app change: %w", err)

diff --git a/pkg/kapp/app/interfaces.go b/pkg/kapp/app/interfaces.go
@@ -30,7 +30,7 @@ type App interface {
 	// Sorted as first is oldest
 	Changes() ([]Change, error)
 	LastChange() (Change, error)
-	BeginChange(ChangeMeta) (Change, error)
+	BeginChange(ChangeMeta, bool) (Change, error)
 	GCChanges(max int, reviewFunc func(changesToDelete []Change) error) (int, int, error)
 }
 

diff --git a/pkg/kapp/app/labeled_app.go b/pkg/kapp/app/labeled_app.go
@@ -75,9 +75,9 @@ func (a *LabeledApp) Rename(_ string, _ string) error { return fmt.Errorf("Not s
 
 func (a *LabeledApp) Meta() (Meta, error) { return Meta{}, nil }
 
-func (a *LabeledApp) Changes() ([]Change, error)             { return nil, nil }
-func (a *LabeledApp) LastChange() (Change, error)            { return nil, nil }
-func (a *LabeledApp) BeginChange(ChangeMeta) (Change, error) { return NoopChange{}, nil }
+func (a *LabeledApp) Changes() ([]Change, error)                   { return nil, nil }
+func (a *LabeledApp) LastChange() (Change, error)                  { return nil, nil }
+func (a *LabeledApp) BeginChange(ChangeMeta, bool) (Change, error) { return NoopChange{}, nil }
 func (a *LabeledApp) GCChanges(_ int, _ func(changesToDelete []Change) error) (int, int, error) {
 	return 0, 0, nil
 }
diff --git a/pkg/kapp/app/recorded_app.go b/pkg/kapp/app/recorded_app.go
@@ -510,7 +510,7 @@ func (a *RecordedApp) LastChange() (Change, error) {
 		return nil, err
 	}
 
-	if len(meta.LastChangeName) == 0 {
+	if meta.LastChange.Successful == nil {
 		return nil, nil
 	}
 
@@ -524,13 +524,13 @@ func (a *RecordedApp) LastChange() (Change, error) {
 	return change, nil
 }
 
-func (a *RecordedApp) BeginChange(meta ChangeMeta) (Change, error) {
+func (a *RecordedApp) BeginChange(meta ChangeMeta, createChange bool) (Change, error) {
 	appMeta, err := a.meta()
 	if err != nil {
 		return nil, err
 	}
 
-	change, err := NewRecordedAppChanges(a.nsName, a.name, appMeta.LabelValue, a.appChangesUseAppLabel, a.coreClient).Begin(meta)
+	change, err := NewRecordedAppChanges(a.nsName, a.name, appMeta.LabelValue, a.appChangesUseAppLabel, a.coreClient).Begin(meta, createChange)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/kapp/app/recorded_app_changes.go b/pkg/kapp/app/recorded_app_changes.go
@@ -112,7 +112,7 @@ func (a RecordedAppChanges) DeleteAll() error {
 	return nil
 }
 
-func (a RecordedAppChanges) Begin(meta ChangeMeta) (*ChangeImpl, error) {
+func (a RecordedAppChanges) Begin(meta ChangeMeta, createChange bool) (*ChangeImpl, error) {
 	newMeta := ChangeMeta{
 		StartedAt:   time.Now().UTC(),
 		Description: meta.Description,
@@ -137,16 +137,22 @@ func (a RecordedAppChanges) Begin(meta ChangeMeta) (*ChangeImpl, error) {
 		configMap.ObjectMeta.Labels[legacyChangeLabelKey] = a.appName
 	}
 
-	createdChange, err := a.coreClient.CoreV1().ConfigMaps(a.nsName).Create(context.TODO(), configMap, metav1.CreateOptions{})
-	if err != nil {
-		return nil, fmt.Errorf("Creating app change: %w", err)
+	changeName := ""
+
+	if createChange {
+		createdChange, err := a.coreClient.CoreV1().ConfigMaps(a.nsName).Create(context.TODO(), configMap, metav1.CreateOptions{})
+		if err != nil {
+			return nil, fmt.Errorf("Creating app change: %w", err)
+		}
+		changeName = createdChange.Name
 	}
 
 	change := &ChangeImpl{
-		name:       createdChange.Name,
-		nsName:     createdChange.Namespace,
+		name:       changeName,
+		nsName:     a.nsName,
 		coreClient: a.coreClient,
 		meta:       newMeta,
+		create:     createChange,
 	}
 
 	return change, nil

diff --git a/pkg/kapp/app/touch.go b/pkg/kapp/app/touch.go
@@ -8,6 +8,7 @@ type Touch struct {
 	Description      string
 	Namespaces       []string
 	IgnoreSuccessErr bool
+	CreateChange     bool
 }
 
 func (t Touch) Do(doFunc func() error) error {
@@ -16,7 +17,7 @@ func (t Touch) Do(doFunc func() error) error {
 		Namespaces:  t.Namespaces,
 	}
 
-	change, err := t.App.BeginChange(meta)
+	change, err := t.App.BeginChange(meta, t.CreateChange)
 	if err != nil {
 		return err
 	}

diff --git a/pkg/kapp/cmd/app/deploy.go b/pkg/kapp/cmd/app/deploy.go
@@ -224,6 +224,7 @@ func (o *DeployOptions) Run() error {
 		Description:      "update: " + changeSummary,
 		Namespaces:       nsNames,
 		IgnoreSuccessErr: true,
+		CreateChange:     o.DeployFlags.AppChangesMaxToKeep > 0,
 	}
 
 	err = touch.Do(func() error {

diff --git a/test/e2e/app_change_test.go b/test/e2e/app_change_test.go
@@ -13,6 +13,7 @@ import (
 	uitest "github.com/cppforlife/go-cli-ui/ui/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"github.com/vmware-tanzu/carvel-kapp/pkg/kapp/app"
 	"gopkg.in/yaml.v2"
 )
 
@@ -222,6 +223,95 @@ metadata:
 	require.Equal(t, yamlSubset{LastChange: lastChange{Namespaces: []string{env.Namespace}}, UsedGKs: []usedGK{{Group: "", Kind: "Secret"}}}, thirdConfigMap)
 }
 
+func TestAppChangesMaxToKeep(t *testing.T) {
+	env := BuildEnv(t)
+	logger := Logger{}
+	kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger}
+	kubectl := Kubectl{t, env.Namespace, Logger{}}
+
+	rbacName := "test-e2e-rbac-app"
+	scopedContext := "scoped-context"
+	scopedUser := "scoped-user"
+	name := "test-app-changes-max-to-keep"
+
+	cleanUp := func() {
+		kapp.Run([]string{"delete", "-a", rbacName})
+		kapp.Run([]string{"delete", "-a", name})
+	}
+
+	cleanUp()
+	defer cleanUp()
+
+	rbac := fmt.Sprintf(`
+---
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: scoped-sa
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: scoped-sa
+  annotations:
+    kubernetes.io/service-account.name: scoped-sa
+type: kubernetes.io/service-account-token
+---
+kind: Role
+apiVersion: rbac.authorization.k8s.io/v1
+metadata:
+  name: scoped-role
+rules:
+- apiGroups: [""]
+  resources: ["configmaps"]
+  verbs: ["get", "list", "watch", "patch", "update", "create"] # no delete permission
+- apiGroups: [""]
+  resources: ["configmaps"]
+  resourceNames: ["%s", "%s"]
+  verbs: ["delete"] # delete permission for meta configmap only
+- apiGroups: [""]
+  resources: ["secrets"]
+  verbs: ["*"]
+---
+kind: RoleBinding
+apiVersion: rbac.authorization.k8s.io/v1
+metadata:
+  name: scoped-role-binding
+subjects:
+- kind: ServiceAccount
+  name: scoped-sa
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: scoped-role
+`, name, name+app.AppSuffix)
+
+	kapp.RunWithOpts([]string{"deploy", "-a", rbacName, "-f", "-"}, RunOpts{IntoNs: true, StdinReader: strings.NewReader(rbac)})
+	cleanUpContext := ScopedContext(t, kubectl, "scoped-sa", scopedContext, scopedUser)
+	defer cleanUpContext()
+
+	yaml1 := `
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: redis-config
+`
+	logger.Section("Setting app-changes-max-to-keep to 0 doesn't create new app-changes", func() {
+		out, _ := kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--app-changes-max-to-keep=0", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)},
+			RunOpts{IntoNs: true, StdinReader: strings.NewReader(yaml1)})
+
+		out, _ = kapp.RunWithOpts([]string{"app-change", "ls", "-a", name, "--json"}, RunOpts{})
+
+		resp := uitest.JSONUIFromBytes(t, []byte(out))
+
+		require.Equal(t, 0, len(resp.Tables[0].Rows), "Expected to have 0 app changes")
+
+		out = kapp.Run([]string{"delete", "-a", name, fmt.Sprintf("--kubeconfig-context=%s", scopedContext)})
+	})
+
+}
+
 type usedGK struct {
 	Group string `yaml:"Group"`
 	Kind  string `yaml:"Kind"`