fix revision will change when add new trait with skiprevisionaffect to application (#3032)

Signed-off-by: chwetion <chwetion@foxmail.com>
Co-authored-by: chwetion <chwetion@foxmail.com>

pkg/controller/core.oam.dev/v1alpha2/application/revision.go

@@ -311,7 +311,7 @@ func ComputeAppRevisionHash(appRevision *v1beta1.ApplicationRevision) (string, e
 		}
 		appRevisionHash.ComponentDefinitionHash[key] = hash
 	}
-	for key, td := range appRevision.Spec.TraitDefinitions {
+	for key, td := range filterSkipAffectAppRevTraitDefinitions(appRevision.Spec.TraitDefinitions) {
 		hash, err := utils.ComputeSpecHash(&td.Spec)
 		if err != nil {
 			return "", err
@@ -376,7 +376,11 @@ func (h *AppHandler) currentAppRevIsNew(ctx context.Context) (bool, bool, error)
 
 	// diff the latest revision first
 	if h.app.Status.LatestRevision.RevisionHash == h.currentRevHash && DeepEqualRevision(h.latestAppRev, h.currentAppRev) {
+		appSpec := h.currentAppRev.Spec.Application.Spec
+		traitDef := h.currentAppRev.Spec.TraitDefinitions
 		h.currentAppRev = h.latestAppRev.DeepCopy()
+		h.currentAppRev.Spec.Application.Spec = appSpec
+		h.currentAppRev.Spec.TraitDefinitions = traitDef
 		return false, false, nil
 	}
 
@@ -409,7 +413,9 @@ func DeepEqualRevision(old, new *v1beta1.ApplicationRevision) bool {
 	if len(old.Spec.WorkloadDefinitions) != len(new.Spec.WorkloadDefinitions) {
 		return false
 	}
-	if len(old.Spec.TraitDefinitions) != len(new.Spec.TraitDefinitions) {
+	oldTraitDefinitions := filterSkipAffectAppRevTraitDefinitions(old.Spec.TraitDefinitions)
+	newTraitDefinitions := filterSkipAffectAppRevTraitDefinitions(new.Spec.TraitDefinitions)
+	if len(oldTraitDefinitions) != len(newTraitDefinitions) {
 		return false
 	}
 	if len(old.Spec.ComponentDefinitions) != len(new.Spec.ComponentDefinitions) {
@@ -428,8 +434,8 @@ func DeepEqualRevision(old, new *v1beta1.ApplicationRevision) bool {
 			return false
 		}
 	}
-	for key, td := range new.Spec.TraitDefinitions {
-		if !apiequality.Semantic.DeepEqual(old.Spec.TraitDefinitions[key].Spec, td.Spec) {
+	for key, td := range newTraitDefinitions {
+		if !apiequality.Semantic.DeepEqual(oldTraitDefinitions[key].Spec, td.Spec) {
 			return false
 		}
 	}
@@ -820,6 +826,17 @@ func filterSkipAffectAppRevTrait(appSpec v1beta1.ApplicationSpec, tds map[string
 	return *res
 }
 
+// before computing hash or deepEqual, filterSkipAffectAppRevTraitDefinitions filter can ignore `SkipAffectRevision` trait definition from appRev
+func filterSkipAffectAppRevTraitDefinitions(tds map[string]v1beta1.TraitDefinition) map[string]v1beta1.TraitDefinition {
+	res := make(map[string]v1beta1.TraitDefinition)
+	for key, td := range tds {
+		if !td.Spec.SkipRevisionAffect {
+			res[key] = td
+		}
+	}
+	return res
+}
+
 type historiesByRevision []v1beta1.ApplicationRevision
 
 func (h historiesByRevision) Len() int      { return len(h) }

pkg/controller/core.oam.dev/v1alpha2/application/revision_test.go

@@ -151,7 +151,6 @@ var _ = Describe("test generate revision ", func() {
 		appRevision1.Spec.ComponentDefinitions[cd.Name] = cd
 		appRevision1.Spec.WorkloadDefinitions[wd.Name] = wd
 		appRevision1.Spec.TraitDefinitions[td.Name] = td
-		appRevision1.Spec.TraitDefinitions[rolloutTd.Name] = rolloutTd
 		appRevision1.Spec.ScopeDefinitions[sd.Name] = sd
 
 		appRevision2 = *appRevision1.DeepCopy()
@@ -168,6 +167,10 @@ var _ = Describe("test generate revision ", func() {
 		Expect(k8sClient.Delete(context.TODO(), &ns)).Should(Succeed())
 	})
 
+	verifyDeepEqualRevision := func() {
+		Expect(DeepEqualRevision(&appRevision1, &appRevision2)).Should(BeTrue())
+	}
+
 	verifyEqual := func() {
 		appHash1, err := ComputeAppRevisionHash(&appRevision1)
 		Expect(err).Should(Succeed())
@@ -228,7 +231,7 @@ var _ = Describe("test generate revision ", func() {
 		verifyNotEqual()
 	})
 
-	It("Test appliction contain a SkipAppRevision tait will have same hash", func() {
+	It("Test appliction contain a SkipAppRevision tait will have same hash and revision will equal", func() {
 		rolloutTrait := common.ApplicationTrait{
 			Type: "rollout",
 			Properties: &runtime.RawExtension{
@@ -236,7 +239,10 @@ var _ = Describe("test generate revision ", func() {
 			},
 		}
 		appRevision2.Spec.Application.Spec.Components[0].Traits = append(appRevision2.Spec.Application.Spec.Components[0].Traits, rolloutTrait)
+		// appRevision will have no traitDefinition of rollout
+		appRevision2.Spec.TraitDefinitions[rolloutTd.Name] = rolloutTd
 		verifyEqual()
+		verifyDeepEqualRevision()
 	})
 
 	It("Test apply success for none rollout case", func() {