From 9b484af1e70f77f19d00e61dbaeef77d8c89cec9 Mon Sep 17 00:00:00 2001 From: Yang Le Date: Sat, 9 May 2020 21:15:26 +0800 Subject: [PATCH 1/2] Generate work status conditions from manifest conditions --- pkg/cmd/spoke/agent.go | 2 +- pkg/helper/helper_test.go | 63 +++++ pkg/helper/helpers.go | 43 ++++ .../controllers/manifestwork_controller.go | 47 +++- .../manifestwork_controller_test.go | 233 ++++++++++++++++-- 5 files changed, 359 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/spoke/agent.go b/pkg/cmd/spoke/agent.go index 2cd167da9..06b103c9d 100644 --- a/pkg/cmd/spoke/agent.go +++ b/pkg/cmd/spoke/agent.go @@ -13,7 +13,7 @@ import ( func NewWorkloadAgent() *cobra.Command { o := spoke.NewWorkloadAgentOptions() cmd := controllercmd. - NewControllerCommandConfig("agent", version.Get(), o.RunWorkloadAgent). + NewControllerCommandConfig("work-agent", version.Get(), o.RunWorkloadAgent). NewCommand() cmd.Use = "agent" cmd.Short = "Start the Cluster Registration Agent" diff --git a/pkg/helper/helper_test.go b/pkg/helper/helper_test.go index fae8bf709..c090c8746 100644 --- a/pkg/helper/helper_test.go +++ b/pkg/helper/helper_test.go @@ -187,3 +187,66 @@ func TestSetManifestCondition(t *testing.T) { }) } } + +func TestMergeStatusConditions(t *testing.T) { + transitionTime := metav1.Now() + + cases := []struct { + name string + startingConditions []workapiv1.StatusCondition + newConditions []workapiv1.StatusCondition + expectedConditions []workapiv1.StatusCondition + }{ + { + name: "add status condition", + newConditions: []workapiv1.StatusCondition{ + newCondition("one", "True", "my-reason", "my-message", nil), + }, + expectedConditions: []workapiv1.StatusCondition{ + newCondition("one", "True", "my-reason", "my-message", nil), + }, + }, + { + name: "merge status condition", + startingConditions: []workapiv1.StatusCondition{ + newCondition("one", "True", "my-reason", "my-message", nil), + }, + newConditions: []workapiv1.StatusCondition{ + newCondition("one", "False", "my-reason", "my-message", nil), + newCondition("two", "True", "my-reason", "my-message", nil), + }, + expectedConditions: []workapiv1.StatusCondition{ + newCondition("one", "False", "my-reason", "my-message", nil), + newCondition("two", "True", "my-reason", "my-message", nil), + }, + }, + { + name: "remove useless status condition", + startingConditions: []workapiv1.StatusCondition{ + newCondition("one", "False", "my-reason", "my-message", &transitionTime), + newCondition("two", "True", "my-reason", "my-message", nil), + }, + newConditions: []workapiv1.StatusCondition{ + newCondition("one", "False", "my-reason", "my-message", nil), + }, + expectedConditions: []workapiv1.StatusCondition{ + newCondition("one", "False", "my-reason", "my-message", &transitionTime), + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + merged := MergeStatusConditions(c.startingConditions, c.newConditions) + for i, expect := range c.expectedConditions { + actual := merged[i] + if expect.LastTransitionTime == (metav1.Time{}) { + actual.LastTransitionTime = metav1.Time{} + } + if !equality.Semantic.DeepEqual(actual, expect) { + t.Errorf(diff.ObjectDiff(actual, expect)) + } + } + }) + } +} diff --git a/pkg/helper/helpers.go b/pkg/helper/helpers.go index 59637e657..3ffb04f85 100644 --- a/pkg/helper/helpers.go +++ b/pkg/helper/helpers.go @@ -41,6 +41,49 @@ func SetManifestCondition(conds *[]workapiv1.ManifestCondition, cond workapiv1.M existingCond.Conditions = cond.Conditions } +// MergeStatusConditions returns a new status condition array with merged status conditions. It is based on newConditions, +// and merges the corresponding existing conditions if exists. +func MergeStatusConditions(conditions []workapiv1.StatusCondition, newConditions []workapiv1.StatusCondition) []workapiv1.StatusCondition { + merged := []workapiv1.StatusCondition{} + + cm := map[string]workapiv1.StatusCondition{} + for _, condition := range conditions { + cm[condition.Type] = condition + } + + for _, newCondition := range newConditions { + // merge two conditions if necessary + if condition, ok := cm[newCondition.Type]; ok { + merged = append(merged, mergeStatusCondition(condition, newCondition)) + continue + } + + newCondition.LastTransitionTime = metav1.NewTime(time.Now()) + merged = append(merged, newCondition) + } + + return merged +} + +// mergeStatusCondition returns a new status condition which merges the properties from the two input conditions. +// It assumes the two conditions have the same condition type. +func mergeStatusCondition(condition, newCondition workapiv1.StatusCondition) workapiv1.StatusCondition { + merged := workapiv1.StatusCondition{ + Type: condition.Type, + Status: newCondition.Status, + Reason: newCondition.Reason, + Message: newCondition.Message, + } + + if condition.Status == newCondition.Status { + merged.LastTransitionTime = condition.LastTransitionTime + } else { + merged.LastTransitionTime = metav1.NewTime(time.Now()) + } + + return merged +} + // SetStatusCondition set a condition in conditons list func SetStatusCondition(conditions *[]workapiv1.StatusCondition, newCondition workapiv1.StatusCondition) { if conditions == nil { diff --git a/pkg/spoke/controllers/manifestwork_controller.go b/pkg/spoke/controllers/manifestwork_controller.go index 96f03478c..2a3a87f1c 100644 --- a/pkg/spoke/controllers/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestwork_controller.go @@ -170,7 +170,7 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac } if len(errs) > 0 { err = utilerrors.NewAggregate(errs) - klog.Errorf("Reconcile work %s fails with err %v: ", manifestWorkName, err) + klog.Errorf("Reconcile work %s fails with err: %v", manifestWorkName, err) } return err } @@ -295,10 +295,35 @@ func (m *ManifestWorkController) applyUnstructrued(data []byte, recorder events. return actual, true, err } +// generateUpdateStatusFunc returns a function which aggregates manifest conditions and generates work conditions. +// Rules to generate work status conditions from manifest conditions +// #1: Applied - work status condition (with type Applied) is applied if all manifest conditions (with type Applied) are applied +// TODO: add rules for other condition types, like Progressing, Available, Degraded func (m *ManifestWorkController) generateUpdateStatusFunc(manifestStatus workapiv1.ManifestResourceStatus) helper.UpdateManifestWorkStatusFunc { return func(oldStatus *workapiv1.ManifestWorkStatus) error { oldStatus.ResourceStatus = manifestStatus - // TODO aggreagae manifest condition to generate work condition + + // aggregate manifest condition to generate work condition + newConditions := []workapiv1.StatusCondition{} + + // handle condition type Applied + if inCondition, exists := isAllInCondition(string(workapiv1.ManifestApplied), manifestStatus.Manifests); exists { + appliedCondition := workapiv1.StatusCondition{ + Type: string(workapiv1.WorkApplied), + } + if inCondition { + appliedCondition.Status = metav1.ConditionTrue + appliedCondition.Reason = "AppliedManifestWorkComplete" + appliedCondition.Message = "Apply manifest work complete" + } else { + appliedCondition.Status = metav1.ConditionFalse + appliedCondition.Reason = "AppliedManifestWorkFailed" + appliedCondition.Message = "Failed to apply manifest work" + } + newConditions = append(newConditions, appliedCondition) + } + + oldStatus.Conditions = helper.MergeStatusConditions(oldStatus.Conditions, newConditions) return nil } } @@ -348,3 +373,21 @@ func isSameUnstructured(obj1, obj2 *unstructured.Unstructured) bool { return equality.Semantic.DeepEqual(obj1Copy.Object, obj2Copy.Object) } + +// isAllInCondition checks status of conditions with a particular type in ManifestCondition array. +// Return true, true only if conditions with the condition type exist and they are all in condition. +func isAllInCondition(conditionType string, manifests []workapiv1.ManifestCondition) (inCondition bool, exists bool) { + for _, manifest := range manifests { + for _, condition := range manifest.Conditions { + if condition.Type == conditionType { + exists = true + } + + if condition.Type == conditionType && condition.Status == metav1.ConditionFalse { + return false, true + } + } + } + + return exists, exists +} diff --git a/pkg/spoke/controllers/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestwork_controller_test.go index 87a499281..a08ddea2c 100644 --- a/pkg/spoke/controllers/manifestwork_controller_test.go +++ b/pkg/spoke/controllers/manifestwork_controller_test.go @@ -8,9 +8,11 @@ import ( "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/events/eventstesting" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/diff" fakedynamic "k8s.io/client-go/dynamic/fake" fakekube "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/restmapper" @@ -214,14 +216,15 @@ func assertManifestCondition( } type testCase struct { - name string - workManifest []*unstructured.Unstructured - spokeObject []runtime.Object - spokeDynamicObject []runtime.Object - expectedWorkAction []string - expectedKubeAction []string - expectedDynamicAction []string - expectedConditions []expectedCondition + name string + workManifest []*unstructured.Unstructured + spokeObject []runtime.Object + spokeDynamicObject []runtime.Object + expectedWorkAction []string + expectedKubeAction []string + expectedDynamicAction []string + expectedManifestConditions []expectedCondition + expectedWorkConditions []expectedCondition } type expectedCondition struct { @@ -231,14 +234,15 @@ type expectedCondition struct { func newTestCase(name string) *testCase { return &testCase{ - name: name, - workManifest: []*unstructured.Unstructured{}, - spokeObject: []runtime.Object{}, - spokeDynamicObject: []runtime.Object{}, - expectedWorkAction: []string{}, - expectedKubeAction: []string{}, - expectedDynamicAction: []string{}, - expectedConditions: []expectedCondition{}, + name: name, + workManifest: []*unstructured.Unstructured{}, + spokeObject: []runtime.Object{}, + spokeDynamicObject: []runtime.Object{}, + expectedWorkAction: []string{}, + expectedKubeAction: []string{}, + expectedDynamicAction: []string{}, + expectedManifestConditions: []expectedCondition{}, + expectedWorkConditions: []expectedCondition{}, } } @@ -272,8 +276,13 @@ func (t *testCase) withExpectedDynamicAction(actions ...string) *testCase { return t } -func (t *testCase) withExpectedCondition(conds ...expectedCondition) *testCase { - t.expectedConditions = conds +func (t *testCase) withExpectedManifestCondition(conds ...expectedCondition) *testCase { + t.expectedManifestConditions = conds + return t +} + +func (t *testCase) withExpectedWorkCondition(conds ...expectedCondition) *testCase { + t.expectedWorkConditions = conds return t } @@ -310,9 +319,32 @@ func (t *testCase) validate( ts.Errorf("Expected to get update action") } actualWork := actual.Object.(*workapiv1.ManifestWork) - for index, cond := range t.expectedConditions { + for index, cond := range t.expectedManifestConditions { assertManifestCondition(ts, actualWork.Status.ResourceStatus.Manifests, int32(index), cond.conditionType, cond.status) } + for _, cond := range t.expectedWorkConditions { + assertCondition(ts, actualWork.Status.Conditions, cond.conditionType, cond.status) + } +} + +func newCondition(name, status, reason, message string, lastTransition *metav1.Time) workapiv1.StatusCondition { + ret := workapiv1.StatusCondition{ + Type: name, + Status: metav1.ConditionStatus(status), + Reason: reason, + Message: message, + } + if lastTransition != nil { + ret.LastTransitionTime = *lastTransition + } + return ret +} + +func newManifestCondition(ordinal int32, resource string, conds ...workapiv1.StatusCondition) workapiv1.ManifestCondition { + return workapiv1.ManifestCondition{ + ResourceMeta: workapiv1.ManifestResourceMeta{Ordinal: ordinal, Resource: resource}, + Conditions: conds, + } } // TestSync test cases when running sync @@ -322,35 +354,41 @@ func TestSync(t *testing.T) { withWorkManifest(newUnstructured("v1", "Secret", "ns1", "test")). withExpectedWorkAction("get", "update"). withExpectedKubeAction("get", "create"). - withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), + withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}). + withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}), newTestCase("create single deployment resource"). withWorkManifest(newUnstructured("apps/v1", "Deployment", "ns1", "test")). withExpectedWorkAction("get", "update"). withExpectedDynamicAction("get", "create"). - withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), + withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), + withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}), newTestCase("update single resource"). withWorkManifest(newUnstructured("v1", "Secret", "ns1", "test")). withSpokeObject(newSecret("test", "ns1", "value2")). withExpectedWorkAction("get", "update"). withExpectedKubeAction("get", "delete", "create"). - withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), + withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}). + withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}), newTestCase("create single unstructured resource"). withWorkManifest(newUnstructured("v1", "NewObject", "ns1", "test")). withExpectedWorkAction("get", "update"). withExpectedDynamicAction("get", "create"). - withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), + withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}). + withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}), newTestCase("update single unstructured resource"). withWorkManifest(newUnstructuredWithContent("v1", "NewObject", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val1"}})). withSpokeDynamicObject(newUnstructuredWithContent("v1", "NewObject", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val2"}})). withExpectedWorkAction("get", "update"). withExpectedDynamicAction("get", "update"). - withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), + withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}). + withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}), newTestCase("multiple create&update resource"). withWorkManifest(newUnstructured("v1", "Secret", "ns1", "test"), newUnstructured("v1", "Secret", "ns2", "test")). withSpokeObject(newSecret("test", "ns1", "value2")). withExpectedWorkAction("get", "update"). withExpectedKubeAction("get", "delete", "create", "get", "create"). - withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}, expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), + withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}, expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}). + withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}), } for _, c := range cases { @@ -411,7 +449,8 @@ func TestFailedToApplyResource(t *testing.T) { withSpokeObject(newSecret("test", "ns1", "value2")). withExpectedWorkAction("get", "update"). withExpectedKubeAction("get", "delete", "create", "get", "create"). - withExpectedCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}, expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionFalse}) + withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}, expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionFalse}). + withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionFalse}) work, workKey := newManifestWork(0, tc.workManifest...) work.Finalizers = []string{manifestWorkFinalizer} @@ -490,3 +529,145 @@ func TestIsSameUnstructured(t *testing.T) { }) } } + +func TestGenerateUpdateStatusFunc(t *testing.T) { + transitionTime := metav1.Now() + + cases := []struct { + name string + startingStatusConditions []workapiv1.StatusCondition + manifestConditions []workapiv1.ManifestCondition + expectedStatusConditions []workapiv1.StatusCondition + }{ + { + name: "no manifest condition exists", + manifestConditions: []workapiv1.ManifestCondition{}, + expectedStatusConditions: []workapiv1.StatusCondition{}, + }, + { + name: "all manifests are applied successfully", + manifestConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource0", newCondition(string(workapiv1.ManifestApplied), string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(1, "resource1", newCondition(string(workapiv1.ManifestApplied), string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + }, + expectedStatusConditions: []workapiv1.StatusCondition{ + newCondition(string(workapiv1.WorkApplied), string(metav1.ConditionTrue), "AppliedManifestWorkComplete", "Apply manifest work complete", nil), + }, + }, + { + name: "one of manifests is not applied", + manifestConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource0", newCondition(string(workapiv1.ManifestApplied), string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(1, "resource1", newCondition(string(workapiv1.ManifestApplied), string(metav1.ConditionFalse), "my-reason", "my-message", nil)), + }, + expectedStatusConditions: []workapiv1.StatusCondition{ + newCondition(string(workapiv1.WorkApplied), string(metav1.ConditionFalse), "AppliedManifestWorkFailed", "Failed to apply manifest work", nil), + }, + }, + { + name: "update existing status condition", + startingStatusConditions: []workapiv1.StatusCondition{ + newCondition(string(workapiv1.WorkApplied), string(metav1.ConditionTrue), "AppliedManifestWorkComplete", "Apply manifest work complete", &transitionTime), + }, + manifestConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource0", newCondition(string(workapiv1.ManifestApplied), string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(1, "resource1", newCondition(string(workapiv1.ManifestApplied), string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + }, + expectedStatusConditions: []workapiv1.StatusCondition{ + newCondition(string(workapiv1.WorkApplied), string(metav1.ConditionTrue), "AppliedManifestWorkComplete", "Apply manifest work complete", &transitionTime), + }, + }, + { + name: "override existing status conditions", + startingStatusConditions: []workapiv1.StatusCondition{ + newCondition(string(workapiv1.WorkApplied), string(metav1.ConditionTrue), "AppliedManifestWorkComplete", "Apply manifest work complete", nil), + }, + manifestConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource0", newCondition(string(workapiv1.ManifestApplied), string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(1, "resource1", newCondition(string(workapiv1.ManifestApplied), string(metav1.ConditionFalse), "my-reason", "my-message", nil)), + }, + expectedStatusConditions: []workapiv1.StatusCondition{ + newCondition(string(workapiv1.WorkApplied), string(metav1.ConditionFalse), "AppliedManifestWorkFailed", "Failed to apply manifest work", nil), + }, + }, + } + + controller := &ManifestWorkController{} + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + updateStatusFunc := controller.generateUpdateStatusFunc(workapiv1.ManifestResourceStatus{Manifests: c.manifestConditions}) + manifestWorkStatus := &workapiv1.ManifestWorkStatus{ + Conditions: c.startingStatusConditions, + } + err := updateStatusFunc(manifestWorkStatus) + if err != nil { + t.Errorf("Should be success with no err: %v", err) + } + + for i, expect := range c.expectedStatusConditions { + actual := manifestWorkStatus.Conditions[i] + if expect.LastTransitionTime == (metav1.Time{}) { + actual.LastTransitionTime = metav1.Time{} + } + + if !equality.Semantic.DeepEqual(actual, expect) { + t.Errorf(diff.ObjectDiff(actual, expect)) + } + } + }) + } +} + +func TestIsAllInCondition(t *testing.T) { + cases := []struct { + name string + conditionType string + manifestConditions []workapiv1.ManifestCondition + expected []bool + }{ + { + name: "condition does not exist", + conditionType: "one", + manifestConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource0", newCondition("two", string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(1, "resource1", newCondition("two", string(metav1.ConditionFalse), "my-reason", "my-message", nil)), + }, + expected: []bool{false, false}, + }, + { + name: "all manifests are in the condition", + conditionType: "one", + manifestConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource0", newCondition("one", string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(1, "resource1", newCondition("one", string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(2, "resource0", newCondition("two", string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(3, "resource1", newCondition("two", string(metav1.ConditionFalse), "my-reason", "my-message", nil)), + }, + expected: []bool{true, true}, + }, + { + name: "one of manifests is not in the condition", + conditionType: "two", + manifestConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource0", newCondition("one", string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(1, "resource1", newCondition("one", string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(2, "resource0", newCondition("two", string(metav1.ConditionTrue), "my-reason", "my-message", nil)), + newManifestCondition(3, "resource1", newCondition("two", string(metav1.ConditionFalse), "my-reason", "my-message", nil)), + }, + expected: []bool{false, true}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + exists, actual := isAllInCondition(c.conditionType, c.manifestConditions) + if c.expected[0] != exists { + t.Errorf("expected %t, but %t", c.expected[0], exists) + } + + if c.expected[1] != actual { + t.Errorf("expected %t, but %t", c.expected[1], actual) + } + }) + } +} From c1a07f89840d6be0f187f9a16b5f63f3e1c4833e Mon Sep 17 00:00:00 2001 From: Yang Le Date: Mon, 18 May 2020 11:20:48 +0800 Subject: [PATCH 2/2] Rename func isAllInCondition and test case --- pkg/helper/helper_test.go | 2 +- pkg/spoke/controllers/manifestwork_controller.go | 6 +++--- .../controllers/manifestwork_controller_test.go | 16 ++++++++-------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/helper/helper_test.go b/pkg/helper/helper_test.go index c090c8746..615981691 100644 --- a/pkg/helper/helper_test.go +++ b/pkg/helper/helper_test.go @@ -221,7 +221,7 @@ func TestMergeStatusConditions(t *testing.T) { }, }, { - name: "remove useless status condition", + name: "remove old status condition", startingConditions: []workapiv1.StatusCondition{ newCondition("one", "False", "my-reason", "my-message", &transitionTime), newCondition("two", "True", "my-reason", "my-message", nil), diff --git a/pkg/spoke/controllers/manifestwork_controller.go b/pkg/spoke/controllers/manifestwork_controller.go index 2a3a87f1c..41969a98b 100644 --- a/pkg/spoke/controllers/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestwork_controller.go @@ -307,7 +307,7 @@ func (m *ManifestWorkController) generateUpdateStatusFunc(manifestStatus workapi newConditions := []workapiv1.StatusCondition{} // handle condition type Applied - if inCondition, exists := isAllInCondition(string(workapiv1.ManifestApplied), manifestStatus.Manifests); exists { + if inCondition, exists := allInCondition(string(workapiv1.ManifestApplied), manifestStatus.Manifests); exists { appliedCondition := workapiv1.StatusCondition{ Type: string(workapiv1.WorkApplied), } @@ -374,9 +374,9 @@ func isSameUnstructured(obj1, obj2 *unstructured.Unstructured) bool { return equality.Semantic.DeepEqual(obj1Copy.Object, obj2Copy.Object) } -// isAllInCondition checks status of conditions with a particular type in ManifestCondition array. +// allInCondition checks status of conditions with a particular type in ManifestCondition array. // Return true, true only if conditions with the condition type exist and they are all in condition. -func isAllInCondition(conditionType string, manifests []workapiv1.ManifestCondition) (inCondition bool, exists bool) { +func allInCondition(conditionType string, manifests []workapiv1.ManifestCondition) (inCondition bool, exists bool) { for _, manifest := range manifests { for _, condition := range manifest.Conditions { if condition.Type == conditionType { diff --git a/pkg/spoke/controllers/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestwork_controller_test.go index a08ddea2c..b6fd86fa6 100644 --- a/pkg/spoke/controllers/manifestwork_controller_test.go +++ b/pkg/spoke/controllers/manifestwork_controller_test.go @@ -360,8 +360,8 @@ func TestSync(t *testing.T) { withWorkManifest(newUnstructured("apps/v1", "Deployment", "ns1", "test")). withExpectedWorkAction("get", "update"). withExpectedDynamicAction("get", "create"). - withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}), - withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}), + withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}). + withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}), newTestCase("update single resource"). withWorkManifest(newUnstructured("v1", "Secret", "ns1", "test")). withSpokeObject(newSecret("test", "ns1", "value2")). @@ -618,7 +618,7 @@ func TestGenerateUpdateStatusFunc(t *testing.T) { } } -func TestIsAllInCondition(t *testing.T) { +func TestAllInCondition(t *testing.T) { cases := []struct { name string conditionType string @@ -660,13 +660,13 @@ func TestIsAllInCondition(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - exists, actual := isAllInCondition(c.conditionType, c.manifestConditions) - if c.expected[0] != exists { - t.Errorf("expected %t, but %t", c.expected[0], exists) + inCondition, exists := allInCondition(c.conditionType, c.manifestConditions) + if c.expected[0] != inCondition { + t.Errorf("expected %t, but %t", c.expected[0], inCondition) } - if c.expected[1] != actual { - t.Errorf("expected %t, but %t", c.expected[1], actual) + if c.expected[1] != exists { + t.Errorf("expected %t, but %t", c.expected[1], exists) } }) }