Merge pull request #931 from cappyzawa/deprecate-updated-template-field

Add deprecation handling for .Updated template field

Author: matheuscscp

api/v1beta2/condition_types.go

@@ -36,4 +36,7 @@ const (
 
 	// InvalidPolicySelectorReason represents an invalid policy selector.
 	InvalidPolicySelectorReason string = "InvalidPolicySelector"
+
+	// RemovedTemplateFieldReason represents usage of removed template field.
+	RemovedTemplateFieldReason string = "RemovedTemplateField"
 )

api/v1beta2/git.go

@@ -65,6 +65,7 @@ type CommitSpec struct {
 	SigningKey *SigningKey `json:"signingKey,omitempty"`
 	// MessageTemplate provides a template for the commit message,
 	// into which will be interpolated the details of the change made.
+	// Note: The `Updated` template field has been removed. Use `Changed` instead.
 	// +optional
 	MessageTemplate string `json:"messageTemplate,omitempty"`
 

config/crd/bases/image.toolkit.fluxcd.io_imageupdateautomations.yaml

@@ -440,6 +440,7 @@ spec:
                         description: |-
                           MessageTemplate provides a template for the commit message,
                           into which will be interpolated the details of the change made.
+                          Note: The `Updated` template field has been removed. Use `Changed` instead.
                         type: string
                       messageTemplateValues:
                         additionalProperties:

docs/api/v1beta2/image-automation.md

@@ -67,7 +67,8 @@ string
 <td>
 <em>(Optional)</em>
 <p>MessageTemplate provides a template for the commit message,
-into which will be interpolated the details of the change made.</p>
+into which will be interpolated the details of the change made.
+Note: The <code>Updated</code> template field has been removed. Use <code>Changed</code> instead.</p>
 </td>
 </tr>
 <tr>

docs/spec/v1beta2/imageupdateautomations.md

@@ -374,12 +374,12 @@ spec:
         Automated image update by Flux
 ```
 
-**Deprecation Note:** The `Updated` template data available in v1beta1 API is
-deprecated. `Changed` template data is recommended for template data, as it
-accommodates for all the updates, including partial updates to just the image
-name or the tag, not just full image with name and tag update. The old templates
-will continue to work in v1beta2 API as `Updated` has not been removed yet. In
-the next API version, `Updated` may be removed.
+**Removal Note:** The `Updated` template data has been removed from the API.
+Use `Changed` template data instead, as it accommodates for all the updates,
+including partial updates to just the image name or the tag, not just full image
+with name and tag update. Templates using `Updated` will result in an error and
+the ImageUpdateAutomation will be marked as Stalled.
+
 
 The message template also has access to the data related to the changes made by
 the automation. The template is a [Go text template][go-text-template]. The data
@@ -393,14 +393,14 @@ type TemplateData struct {
 	AutomationObject struct {
 	  Name, Namespace string
 	}
-	Changed update.ResultV2
+	Changed update.Result
 	Values map[string]string
 }
 
-// ResultV2 contains the file changes made during the update. It contains
+// Result contains the file changes made during the update. It contains
 // details about the exact changes made to the files and the objects in them. It
 // has a nested structure file->objects->changes.
-type ResultV2 struct {
+type Result struct {
 	FileChanges map[string]ObjectChanges
 }
 
@@ -427,10 +427,10 @@ over the changed objects and changes:
 
 ```go
 // Changes returns all the changes that were made in at least one update.
-func (r ResultV2) Changes() []Change
+func (r Result) Changes() []Change
 
 // Objects returns ObjectChanges, regardless of which file they appear in.
-func (r ResultV2) Objects() ObjectChanges
+func (r Result) Objects() ObjectChanges
 ```
 
 Example of using the methods in a template:

internal/controller/imageupdateautomation_controller.go

@@ -481,6 +481,14 @@ func (r *ImageUpdateAutomationReconciler) reconcile(ctx context.Context, sp *pat
 
 	pushResult, err = sm.CommitAndPush(ctx, obj, policyResult, pushCfg...)
 	if err != nil {
+		// Check if error is due to removed template field usage.
+		// Set Stalled condition and return nil error to prevent requeue, allowing user to fix template.
+		if errors.Is(err, source.ErrRemovedTemplateField) {
+			conditions.MarkStalled(obj, imagev1.RemovedTemplateFieldReason, "%s", err)
+			result, retErr = ctrl.Result{}, nil
+			return
+		}
+
 		e := fmt.Errorf("failed to update source: %w", err)
 		conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.GitOperationFailedReason, "%s", e)
 		result, retErr = ctrl.Result{}, e

internal/controller/imageupdateautomation_controller_test.go

@@ -71,12 +71,12 @@ const (
 Automation: {{ .AutomationObject }}
 
 Files:
-{{ range $filename, $_ := .Updated.Files -}}
+{{ range $filename, $_ := .Changed.FileChanges -}}
 - {{ $filename }}
 {{ end -}}
 
 Objects:
-{{ range $resource, $_ := .Updated.Objects -}}
+{{ range $resource, $_ := .Changed.Objects -}}
 {{ if eq $resource.Kind "Deployment" -}}
 - {{ $resource.Kind | lower }} {{ $resource.Name | lower }}
 {{ else -}}
@@ -85,8 +85,10 @@ Objects:
 {{ end -}}
 
 Images:
-{{ range .Updated.Images -}}
-- {{.}} ({{.Policy.Name}})
+{{ range .Changed.Changes -}}
+{{ if .Setter -}}
+- {{.NewValue}} ({{.Setter}})
+{{ end -}}
 {{ end -}}
 `
 	testCommitMessageFmt = `Commit summary
@@ -98,7 +100,7 @@ Files:
 Objects:
 - deployment test
 Images:
-- helloworld:v1.0.0 (%s)
+- helloworld:v1.0.0 (%s:%s)
 `
 )
 
@@ -727,7 +729,7 @@ func TestImageUpdateAutomationReconciler_commitMessage(t *testing.T) {
 			name:     "template with update Result",
 			template: testCommitTemplate,
 			wantCommitMsg: func(policyName, policyNS string) string {
-				return fmt.Sprintf(testCommitMessageFmt, policyNS, policyName)
+				return fmt.Sprintf(testCommitMessageFmt, policyNS, policyNS, policyName)
 			},
 		},
 		{
@@ -841,6 +843,134 @@ Automation: %s/update-test
 	}
 }
 
+// TestImageUpdateAutomationReconciler_removedTemplateField tests removed template field usage (.Updated and .Changed.ImageResult).
+func TestImageUpdateAutomationReconciler_removedTemplateField(t *testing.T) {
+	policySpec := imagev1_reflect.ImagePolicySpec{
+		ImageRepositoryRef: meta.NamespacedObjectReference{
+			Name: "not-expected-to-exist",
+		},
+		Policy: imagev1_reflect.ImagePolicyChoice{
+			SemVer: &imagev1_reflect.SemVerPolicy{
+				Range: "1.x",
+			},
+		},
+	}
+	fixture := "testdata/appconfig"
+	latest := "helloworld:v1.0.0"
+
+	testCases := []struct {
+		name           string
+		template       string
+		expectedErrMsg string
+	}{
+		{
+			name: ".Updated field",
+			template: `Commit summary
+
+Automation: {{ .AutomationObject }}
+
+Files:
+{{ range $filename, $_ := .Updated.Files -}}
+- {{ $filename }}
+{{ end -}}
+
+Objects:
+{{ range $resource, $_ := .Updated.Objects -}}
+{{ if eq $resource.Kind "Deployment" -}}
+- {{ $resource.Kind | lower }} {{ $resource.Name | lower }}
+{{ else -}}
+- {{ $resource.Kind }} {{ $resource.Name }}
+{{ end -}}
+{{ end -}}
+
+Images:
+{{ range .Updated.Images -}}
+- {{.}} ({{.Policy.Name}})
+{{ end -}}
+`,
+			expectedErrMsg: "template uses removed '.Updated' field",
+		},
+		{
+			name: ".Changed.ImageResult field",
+			template: `Commit summary
+
+Automation: {{ .AutomationObject }}
+
+Files:
+{{ range $filename, $_ := .Changed.ImageResult.Files -}}
+- {{ $filename }}
+{{ end -}}
+
+Objects:
+{{ range $resource, $_ := .Changed.ImageResult.Objects -}}
+- {{ $resource.Kind }} {{ $resource.Name }}
+{{ end -}}
+
+Images:
+{{ range .Changed.ImageResult.Images -}}
+- {{.}} ({{.Policy.Name}})
+{{ end -}}
+`,
+			expectedErrMsg: "template uses removed '.Changed.ImageResult' field",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			ctx := context.TODO()
+
+			namespace, err := testEnv.CreateNamespace(ctx, "image-auto-test")
+			g.Expect(err).ToNot(HaveOccurred())
+			defer func() { g.Expect(testEnv.Delete(ctx, namespace)).To(Succeed()) }()
+
+			testWithRepoAndImagePolicy(
+				ctx, g, testEnv, namespace.Name, fixture, policySpec, latest,
+				func(g *WithT, s repoAndPolicyArgs, repoURL string, localRepo *extgogit.Repository) {
+					policyKey := types.NamespacedName{
+						Name:      s.imagePolicyName,
+						Namespace: s.namespace,
+					}
+					_ = testutil.CommitInRepo(ctx, g, repoURL, s.branch, originRemote, "Install setter marker", func(tmp string) {
+						g.Expect(testutil.ReplaceMarker(filepath.Join(tmp, "deploy.yaml"), policyKey)).To(Succeed())
+					})
+
+					preChangeCommitId := testutil.CommitIdFromBranch(localRepo, s.branch)
+					waitForNewHead(g, localRepo, s.branch, preChangeCommitId)
+					preChangeCommitId = testutil.CommitIdFromBranch(localRepo, s.branch)
+
+					updateStrategy := &imagev1.UpdateStrategy{
+						Strategy: imagev1.UpdateStrategySetters,
+					}
+					err := createImageUpdateAutomation(ctx, testEnv, "update-test", s.namespace, s.gitRepoName, s.gitRepoNamespace, s.branch, s.branch, "", tc.template, "", updateStrategy)
+					g.Expect(err).ToNot(HaveOccurred())
+					defer func() {
+						g.Expect(deleteImageUpdateAutomation(ctx, testEnv, "update-test", s.namespace)).To(Succeed())
+					}()
+
+					imageUpdateKey := types.NamespacedName{
+						Namespace: s.namespace,
+						Name:      "update-test",
+					}
+
+					g.Eventually(func() bool {
+						var imageUpdate imagev1.ImageUpdateAutomation
+						_ = testEnv.Get(context.TODO(), imageUpdateKey, &imageUpdate)
+						stalledCondition := apimeta.FindStatusCondition(imageUpdate.Status.Conditions, meta.StalledCondition)
+						return stalledCondition != nil &&
+							stalledCondition.Status == metav1.ConditionTrue &&
+							stalledCondition.Reason == imagev1.RemovedTemplateFieldReason &&
+							strings.Contains(stalledCondition.Message, tc.expectedErrMsg)
+					}, timeout).Should(BeTrue())
+
+					head, _ := localRepo.Head()
+					g.Expect(head.Hash().String()).To(Equal(preChangeCommitId))
+				},
+			)
+		})
+	}
+}
+
 func TestImageUpdateAutomationReconciler_crossNamespaceRef(t *testing.T) {
 	g := NewWithT(t)
 	policySpec := imagev1_reflect.ImagePolicySpec{
@@ -875,7 +1005,7 @@ func TestImageUpdateAutomationReconciler_crossNamespaceRef(t *testing.T) {
 	testWithCustomRepoAndImagePolicy(
 		ctx, g, testEnv, fixture, policySpec, latest, args,
 		func(g *WithT, s repoAndPolicyArgs, repoURL string, localRepo *extgogit.Repository) {
-			commitMessage := fmt.Sprintf(testCommitMessageFmt, s.namespace, s.imagePolicyName)
+			commitMessage := fmt.Sprintf(testCommitMessageFmt, s.namespace, s.namespace, s.imagePolicyName)
 
 			// Update the setter marker in the repo.
 			policyKey := types.NamespacedName{

internal/policy/applier.go

@@ -42,8 +42,8 @@ var (
 
 // ApplyPolicies applies the given set of policies on the source present in the
 // workDir based on the provided ImageUpdateAutomation configuration.
-func ApplyPolicies(ctx context.Context, workDir string, obj *imagev1.ImageUpdateAutomation, policies []imagev1_reflect.ImagePolicy) (update.ResultV2, error) {
-	var result update.ResultV2
+func ApplyPolicies(ctx context.Context, workDir string, obj *imagev1.ImageUpdateAutomation, policies []imagev1_reflect.ImagePolicy) (update.Result, error) {
+	var result update.Result
 	if obj.Spec.Update == nil {
 		return result, ErrNoUpdateStrategy
 	}
@@ -62,5 +62,5 @@ func ApplyPolicies(ctx context.Context, workDir string, obj *imagev1.ImageUpdate
 	}
 
 	tracelog := log.FromContext(ctx).V(logger.TraceLevel)
-	return update.UpdateV2WithSetters(tracelog, manifestPath, manifestPath, policies)
+	return update.UpdateWithSetters(tracelog, manifestPath, manifestPath, policies)
 }

internal/policy/applier_test.go

@@ -31,7 +31,6 @@ import (
 	imagev1 "github.com/fluxcd/image-automation-controller/api/v1beta2"
 	"github.com/fluxcd/image-automation-controller/internal/testutil"
 	"github.com/fluxcd/image-automation-controller/pkg/test"
-	"github.com/fluxcd/image-automation-controller/pkg/update"
 )
 
 func testdataPath(path string) string {
@@ -48,7 +47,6 @@ func Test_applyPolicies(t *testing.T) {
 		inputPath          string
 		expectedPath       string
 		wantErr            bool
-		wantResult         update.Result
 	}{
 		{
 			name: "valid update strategy and one policy",