From 2f752f47b944df53e958e22732805179e631f8b2 Mon Sep 17 00:00:00 2001 From: Mahdi Mohammadi Date: Sun, 12 Nov 2017 17:26:23 +0000 Subject: [PATCH 1/3] Replace type switches in Rollback with Visitor pattern --- pkg/kubectl/BUILD | 1 + pkg/kubectl/rollback.go | 48 +++++++++++++++++++++++++++++------- pkg/kubectl/rollback_test.go | 46 ++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 9 deletions(-) create mode 100644 pkg/kubectl/rollback_test.go diff --git a/pkg/kubectl/BUILD b/pkg/kubectl/BUILD index fd1a9876c1a..e0777b438d4 100644 --- a/pkg/kubectl/BUILD +++ b/pkg/kubectl/BUILD @@ -23,6 +23,7 @@ go_test( "quota_test.go", "resource_filter_test.go", "rolebinding_test.go", + "rollback_test.go", "rolling_updater_test.go", "rollout_status_test.go", "run_test.go", diff --git a/pkg/kubectl/rollback.go b/pkg/kubectl/rollback.go index 7f6dfa80fb1..50236ea63f9 100644 --- a/pkg/kubectl/rollback.go +++ b/pkg/kubectl/rollback.go @@ -35,13 +35,13 @@ import ( "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" "k8s.io/kubernetes/pkg/api/legacyscheme" - "k8s.io/kubernetes/pkg/apis/apps" api "k8s.io/kubernetes/pkg/apis/core" apiv1 "k8s.io/kubernetes/pkg/apis/core/v1" "k8s.io/kubernetes/pkg/apis/extensions" "k8s.io/kubernetes/pkg/controller/daemon" deploymentutil "k8s.io/kubernetes/pkg/controller/deployment/util" "k8s.io/kubernetes/pkg/controller/statefulset" + kapps "k8s.io/kubernetes/pkg/kubectl/apps" sliceutil "k8s.io/kubernetes/pkg/kubectl/util/slice" printersinternal "k8s.io/kubernetes/pkg/printers/internalversion" ) @@ -56,16 +56,46 @@ type Rollbacker interface { Rollback(obj runtime.Object, updatedAnnotations map[string]string, toRevision int64, dryRun bool) (string, error) } +type RollbackVisitor struct { + clientset kubernetes.Interface + result Rollbacker +} + +func (v *RollbackVisitor) VisitDeployment(elem kapps.GroupKindElement) { + v.result = &DeploymentRollbacker{v.clientset} +} + +func (v *RollbackVisitor) VisitStatefulSet(kind kapps.GroupKindElement) { + v.result = &StatefulSetRollbacker{v.clientset} +} + +func (v *RollbackVisitor) VisitDaemonSet(kind kapps.GroupKindElement) { + v.result = &DaemonSetRollbacker{v.clientset} +} + +func (v *RollbackVisitor) VisitJob(kind kapps.GroupKindElement) {} +func (v *RollbackVisitor) VisitPod(kind kapps.GroupKindElement) {} +func (v *RollbackVisitor) VisitReplicaSet(kind kapps.GroupKindElement) {} +func (v *RollbackVisitor) VisitReplicationController(kind kapps.GroupKindElement) {} + +// RollbackerFor returns an implementation of Rollbacker interface for the given schema kind func RollbackerFor(kind schema.GroupKind, c kubernetes.Interface) (Rollbacker, error) { - switch kind { - case extensions.Kind("Deployment"), apps.Kind("Deployment"): - return &DeploymentRollbacker{c}, nil - case extensions.Kind("DaemonSet"), apps.Kind("DaemonSet"): - return &DaemonSetRollbacker{c}, nil - case apps.Kind("StatefulSet"): - return &StatefulSetRollbacker{c}, nil + elem := kapps.GroupKindElement(kind) + visitor := &RollbackVisitor{ + clientset: c, } - return nil, fmt.Errorf("no rollbacker has been implemented for %q", kind) + + err := elem.Accept(visitor) + + if err != nil { + return nil, fmt.Errorf("error retrieving rollbacker for %q, %v", kind.String(), err) + } + + if visitor.result == nil { + return nil, fmt.Errorf("no rollbacker has been implemented for %q", kind.String()) + } + + return visitor.result, nil } type DeploymentRollbacker struct { diff --git a/pkg/kubectl/rollback_test.go b/pkg/kubectl/rollback_test.go new file mode 100644 index 00000000000..1d41650ba39 --- /dev/null +++ b/pkg/kubectl/rollback_test.go @@ -0,0 +1,46 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package kubectl + +import ( + "reflect" + "testing" + + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/kubernetes/fake" +) + +var rollbacktests = map[schema.GroupKind]reflect.Type{ + {Group: "apps", Kind: "DaemonSet"}: reflect.TypeOf(&DaemonSetRollbacker{}), + {Group: "apps", Kind: "StatefulSet"}: reflect.TypeOf(&StatefulSetRollbacker{}), + {Group: "apps", Kind: "Deployment"}: reflect.TypeOf(&DeploymentRollbacker{}), +} + +func TestRollbackerFor(t *testing.T) { + fakeClientset := &fake.Clientset{} + + for kind, expectedType := range rollbacktests { + result, err := RollbackerFor(kind, fakeClientset) + if err != nil { + t.Fatalf("error getting Rollbacker for a %v: %v", kind.String(), err) + } + + if reflect.TypeOf(result) != expectedType { + t.Fatalf("unexpected output type (%v was expected but got %v)", expectedType, reflect.TypeOf(result)) + } + } +} From d814a5ad5139caab29c7b697ab33a39211182915 Mon Sep 17 00:00:00 2001 From: Mahdi Mohammadi Date: Sun, 19 Nov 2017 17:51:31 +0000 Subject: [PATCH 2/3] Update for cronJob --- pkg/kubectl/rollback.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/kubectl/rollback.go b/pkg/kubectl/rollback.go index 50236ea63f9..90971aeb0c5 100644 --- a/pkg/kubectl/rollback.go +++ b/pkg/kubectl/rollback.go @@ -77,6 +77,7 @@ func (v *RollbackVisitor) VisitJob(kind kapps.GroupKindElement) func (v *RollbackVisitor) VisitPod(kind kapps.GroupKindElement) {} func (v *RollbackVisitor) VisitReplicaSet(kind kapps.GroupKindElement) {} func (v *RollbackVisitor) VisitReplicationController(kind kapps.GroupKindElement) {} +func (v *RollbackVisitor) VisitCronJob(kind kapps.GroupKindElement) {} // RollbackerFor returns an implementation of Rollbacker interface for the given schema kind func RollbackerFor(kind schema.GroupKind, c kubernetes.Interface) (Rollbacker, error) { From 18ef4beb84840c7d69d655ea4408a4ce24addc1a Mon Sep 17 00:00:00 2001 From: Mahdi Mohammadi Date: Wed, 22 Nov 2017 06:17:44 +0000 Subject: [PATCH 3/3] Trying to make error message similar to what is expected in tests --- pkg/kubectl/rollback.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubectl/rollback.go b/pkg/kubectl/rollback.go index 90971aeb0c5..864d5a290dc 100644 --- a/pkg/kubectl/rollback.go +++ b/pkg/kubectl/rollback.go @@ -93,7 +93,7 @@ func RollbackerFor(kind schema.GroupKind, c kubernetes.Interface) (Rollbacker, e } if visitor.result == nil { - return nil, fmt.Errorf("no rollbacker has been implemented for %q", kind.String()) + return nil, fmt.Errorf("no rollbacker has been implemented for %q", kind) } return visitor.result, nil