From 94d99c85d0418a4f98205cf8279cc2bee56b6647 Mon Sep 17 00:00:00 2001 From: Ruslan Aliev Date: Wed, 19 Aug 2020 02:11:10 -0500 Subject: [PATCH] Refactor cluster status command This patch moves all cluster status functionality from cmd level to pkg level as well as unit tests, making code cleaner and improving actual test coverage. Change-Id: Ia811887b684b2129ca30dd90b5afc72e726271ff Signed-off-by: Ruslan Aliev --- cmd/cluster/status.go | 58 ++------ cmd/cluster/status_test.go | 133 +----------------- .../check-status-no-resources.golden | 1 - .../check-status-with-resources.golden | 3 - cmd/cluster/testdata/statusmap/crd.yaml | 40 ------ .../testdata/statusmap/kustomization.yaml | 8 -- .../testdata/statusmap/legacy-crd.yaml | 42 ------ .../testdata/statusmap/legacy-resource.yaml | 7 - cmd/cluster/testdata/statusmap/missing.yaml | 7 - .../testdata/statusmap/pending-resource.yaml | 7 - .../testdata/statusmap/stable-resource.yaml | 7 - cmd/cluster/testdata/statusmap/unknown.yaml | 8 -- pkg/cluster/command.go | 52 +++++++ pkg/cluster/command_test.go | 62 ++++++++ pkg/cluster/status.go | 54 +++++++ pkg/cluster/status_test.go | 62 ++++++++ 16 files changed, 244 insertions(+), 307 deletions(-) delete mode 100644 cmd/cluster/testdata/TestNewClusterStatusCmdGoldenOutput/check-status-no-resources.golden delete mode 100644 cmd/cluster/testdata/TestNewClusterStatusCmdGoldenOutput/check-status-with-resources.golden delete mode 100644 cmd/cluster/testdata/statusmap/crd.yaml delete mode 100644 cmd/cluster/testdata/statusmap/kustomization.yaml delete mode 100644 cmd/cluster/testdata/statusmap/legacy-crd.yaml delete mode 100644 cmd/cluster/testdata/statusmap/legacy-resource.yaml delete mode 100644 cmd/cluster/testdata/statusmap/missing.yaml delete mode 100644 cmd/cluster/testdata/statusmap/pending-resource.yaml delete mode 100644 cmd/cluster/testdata/statusmap/stable-resource.yaml delete mode 100644 cmd/cluster/testdata/statusmap/unknown.yaml create mode 100755 pkg/cluster/command.go create mode 100755 pkg/cluster/command_test.go diff --git a/cmd/cluster/status.go b/cmd/cluster/status.go index da412dd83..ecc9fb4b6 100644 --- a/cmd/cluster/status.go +++ b/cmd/cluster/status.go @@ -15,68 +15,26 @@ package cluster import ( - "fmt" - "github.com/spf13/cobra" "opendev.org/airship/airshipctl/pkg/cluster" "opendev.org/airship/airshipctl/pkg/config" - "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/k8s/client" - "opendev.org/airship/airshipctl/pkg/log" - "opendev.org/airship/airshipctl/pkg/util" ) // NewStatusCommand creates a command which reports the statuses of a cluster's deployed components. func NewStatusCommand(cfgFactory config.Factory, factory client.Factory) *cobra.Command { + o := cluster.NewStatusOptions(cfgFactory, factory) cmd := &cobra.Command{ Use: "status", Short: "Retrieve statuses of deployed cluster components", - RunE: func(cmd *cobra.Command, args []string) error { - conf, err := cfgFactory() - if err != nil { - return err - } - - manifest, err := conf.CurrentContextManifest() - if err != nil { - return err - } - - docBundle, err := document.NewBundleByPath(manifest.TargetPath) - if err != nil { - return err - } - - docs, err := docBundle.GetAllDocuments() - if err != nil { - return err - } - - client, err := factory(conf) - if err != nil { - return err - } - - statusMap, err := cluster.NewStatusMap(client) - if err != nil { - return err - } - - tw := util.NewTabWriter(cmd.OutOrStdout()) - fmt.Fprintf(tw, "Kind\tName\tStatus\n") - for _, doc := range docs { - status, err := statusMap.GetStatusForResource(doc) - if err != nil { - log.Debug(err) - } else { - fmt.Fprintf(tw, "%s\t%s\t%s\n", doc.GetKind(), doc.GetName(), status) - } - } - tw.Flush() - return nil - }, + RunE: clusterStatusRunE(o), } - return cmd } + +func clusterStatusRunE(o cluster.StatusOptions) func(cmd *cobra.Command, args []string) error { + return func(cmd *cobra.Command, args []string) error { + return cluster.StatusRunner(o, cmd.OutOrStdout()) + } +} diff --git a/cmd/cluster/status_test.go b/cmd/cluster/status_test.go index e3217f889..36d7bd522 100644 --- a/cmd/cluster/status_test.go +++ b/cmd/cluster/status_test.go @@ -17,140 +17,19 @@ package cluster_test import ( "testing" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "opendev.org/airship/airshipctl/cmd/cluster" - "opendev.org/airship/airshipctl/pkg/config" - "opendev.org/airship/airshipctl/pkg/k8s/client" - "opendev.org/airship/airshipctl/pkg/k8s/client/fake" "opendev.org/airship/airshipctl/testutil" ) -const ( - fixturesPath = "testdata/statusmap" -) - func TestNewClusterStatusCmd(t *testing.T) { - tests := []struct { - cmdTest *testutil.CmdTest - resources []runtime.Object - CRDs []runtime.Object - }{ + tests := []*testutil.CmdTest{ { - cmdTest: &testutil.CmdTest{ - Name: "check-status-no-resources", - CmdLine: "", - }, - }, - { - cmdTest: &testutil.CmdTest{ - Name: "cluster-status-cmd-with-help", - CmdLine: "--help", - }, - }, - { - cmdTest: &testutil.CmdTest{ - Name: "check-status-with-resources", - CmdLine: "", - }, - resources: []runtime.Object{ - makeResource("Resource", "stable-resource", "stable"), - makeResource("Resource", "pending-resource", "pending"), - }, - CRDs: []runtime.Object{ - makeResourceCRD(annotationValidStatusCheck()), - }, + Name: "cluster-status-cmd-with-help", + CmdLine: "--help", + Cmd: cluster.NewStatusCommand(nil, nil), }, } - - for _, tt := range tests { - tt := tt - testClientFactory := func(_ *config.Config) (client.Interface, error) { - return fake.NewClient( - fake.WithDynamicObjects(tt.resources...), - fake.WithCRDs(tt.CRDs...), - ), nil - } - tt.cmdTest.Cmd = cluster.NewStatusCommand(clusterStatusTestSettings(), testClientFactory) - testutil.RunTest(t, tt.cmdTest) - } -} - -func clusterStatusTestSettings() config.Factory { - return func() (*config.Config, error) { - return &config.Config{ - Contexts: map[string]*config.Context{ - "testContext": {Manifest: "testManifest"}, - }, - Manifests: map[string]*config.Manifest{ - "testManifest": {TargetPath: fixturesPath}, - }, - CurrentContext: "testContext", - }, nil - } -} - -func makeResource(kind, name, state string) *unstructured.Unstructured { - return &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "example.com/v1", - "kind": kind, - "metadata": map[string]interface{}{ - "name": name, - "namespace": "default", - }, - "status": map[string]interface{}{ - "state": state, - }, - }, - } -} - -func annotationValidStatusCheck() map[string]string { - return map[string]string{ - "airshipit.org/status-check": ` -[ - { - "status": "Stable", - "condition": "@.status.state==\"stable\"" - }, - { - "status": "Pending", - "condition": "@.status.state==\"pending\"" - } -]`, - } -} - -func makeResourceCRD(annotations map[string]string) *apiextensionsv1.CustomResourceDefinition { - return &apiextensionsv1.CustomResourceDefinition{ - TypeMeta: metav1.TypeMeta{ - Kind: "CustomResourceDefinition", - APIVersion: "apiextensions.k8s.io/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "resources.example.com", - Annotations: annotations, - }, - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Group: "example.com", - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1", - Served: true, - Storage: true, - }, - }, - // omitting the openAPIV3Schema for brevity - Scope: "Namespaced", - Names: apiextensionsv1.CustomResourceDefinitionNames{ - Kind: "Resource", - Plural: "resources", - Singular: "resource", - }, - }, + for _, testcase := range tests { + testutil.RunTest(t, testcase) } } diff --git a/cmd/cluster/testdata/TestNewClusterStatusCmdGoldenOutput/check-status-no-resources.golden b/cmd/cluster/testdata/TestNewClusterStatusCmdGoldenOutput/check-status-no-resources.golden deleted file mode 100644 index c6c460a71..000000000 --- a/cmd/cluster/testdata/TestNewClusterStatusCmdGoldenOutput/check-status-no-resources.golden +++ /dev/null @@ -1 +0,0 @@ -Kind Name Status diff --git a/cmd/cluster/testdata/TestNewClusterStatusCmdGoldenOutput/check-status-with-resources.golden b/cmd/cluster/testdata/TestNewClusterStatusCmdGoldenOutput/check-status-with-resources.golden deleted file mode 100644 index 54a9f0dbb..000000000 --- a/cmd/cluster/testdata/TestNewClusterStatusCmdGoldenOutput/check-status-with-resources.golden +++ /dev/null @@ -1,3 +0,0 @@ -Kind Name Status -Resource pending-resource Pending -Resource stable-resource Stable diff --git a/cmd/cluster/testdata/statusmap/crd.yaml b/cmd/cluster/testdata/statusmap/crd.yaml deleted file mode 100644 index 929eeb969..000000000 --- a/cmd/cluster/testdata/statusmap/crd.yaml +++ /dev/null @@ -1,40 +0,0 @@ -# this CRD defines a type whose status can be checked using the condition in -# the annotations -apiVersion: apiextensions.k8s.io/v1 -kind: CustomResourceDefinition -metadata: - name: resources.example.com - annotations: - airshipit.org/status-check: | - [ - { - "status": "Stable", - "condition": "@.status.state==\"stable\"" - }, - { - "status": "Pending", - "condition": "@.status.state==\"pending\"" - } - ] -spec: - group: example.com - versions: - - name: v1 - served: true - storage: true - schema: - openAPIV3Schema: - type: object - properties: - status: - type: object - properties: - state: - type: string - scope: Namespaced - names: - plural: resources - singular: resource - kind: Resource - shortNames: - - rsc diff --git a/cmd/cluster/testdata/statusmap/kustomization.yaml b/cmd/cluster/testdata/statusmap/kustomization.yaml deleted file mode 100644 index f4b156e41..000000000 --- a/cmd/cluster/testdata/statusmap/kustomization.yaml +++ /dev/null @@ -1,8 +0,0 @@ -resources: - - crd.yaml - - stable-resource.yaml - - pending-resource.yaml - - missing.yaml - - unknown.yaml - - legacy-crd.yaml - - legacy-resource.yaml diff --git a/cmd/cluster/testdata/statusmap/legacy-crd.yaml b/cmd/cluster/testdata/statusmap/legacy-crd.yaml deleted file mode 100644 index b9e250046..000000000 --- a/cmd/cluster/testdata/statusmap/legacy-crd.yaml +++ /dev/null @@ -1,42 +0,0 @@ -# this is a legacy CRD which defines a type whose status can be checked using -# the condition in the annotations -# It is included in tests to assure backward compatibility -apiVersion: apiextensions.k8s.io/v1beta1 -kind: CustomResourceDefinition -metadata: - name: legacies.example.com - annotations: - airshipit.org/status-check: | - [ - { - "status": "Stable", - "condition": "@.status.state==\"stable\"" - }, - { - "status": "Pending", - "condition": "@.status.state==\"pending\"" - } - ] -spec: - group: example.com - versions: - - name: v1 - served: true - storage: true - scope: Namespaced - names: - plural: legacies - singular: legacy - kind: Legacy - shortNames: - - lgc - preserveUnknownFields: false - validation: - openAPIV3Schema: - type: object - properties: - status: - type: object - properties: - state: - type: string diff --git a/cmd/cluster/testdata/statusmap/legacy-resource.yaml b/cmd/cluster/testdata/statusmap/legacy-resource.yaml deleted file mode 100644 index 2aca6df17..000000000 --- a/cmd/cluster/testdata/statusmap/legacy-resource.yaml +++ /dev/null @@ -1,7 +0,0 @@ -# this legacy-resource is stable because the fake version in the cluster will -# have .status.state == "stable" -apiVersion: "example.com/v1" -kind: Legacy -metadata: - name: stable-legacy - namespace: default diff --git a/cmd/cluster/testdata/statusmap/missing.yaml b/cmd/cluster/testdata/statusmap/missing.yaml deleted file mode 100644 index 43729dd45..000000000 --- a/cmd/cluster/testdata/statusmap/missing.yaml +++ /dev/null @@ -1,7 +0,0 @@ -# This resource doesn't have a status-check defined by its CRD (which is also -# missing for brevity). Requesting its status is an error -apiVersion: "example.com/v1" -kind: Missing -metadata: - name: missing-resource - namespace: default diff --git a/cmd/cluster/testdata/statusmap/pending-resource.yaml b/cmd/cluster/testdata/statusmap/pending-resource.yaml deleted file mode 100644 index b1373a07a..000000000 --- a/cmd/cluster/testdata/statusmap/pending-resource.yaml +++ /dev/null @@ -1,7 +0,0 @@ -# this resource is pending because the fake version in the cluster will -# have .status.state == "pending" -apiVersion: "example.com/v1" -kind: Resource -metadata: - name: pending-resource - namespace: default diff --git a/cmd/cluster/testdata/statusmap/stable-resource.yaml b/cmd/cluster/testdata/statusmap/stable-resource.yaml deleted file mode 100644 index a6fff614f..000000000 --- a/cmd/cluster/testdata/statusmap/stable-resource.yaml +++ /dev/null @@ -1,7 +0,0 @@ -# this resource is stable because the fake version in the cluster will have -# .status.state == "stable" -apiVersion: "example.com/v1" -kind: Resource -metadata: - name: stable-resource - namespace: default diff --git a/cmd/cluster/testdata/statusmap/unknown.yaml b/cmd/cluster/testdata/statusmap/unknown.yaml deleted file mode 100644 index 58c65e17f..000000000 --- a/cmd/cluster/testdata/statusmap/unknown.yaml +++ /dev/null @@ -1,8 +0,0 @@ -# this resource is in an unknown state because the fake version in the cluster -# will have .status.state == "unknown", which does not correlate to any of the -# status checks in the CRD. -apiVersion: "example.com/v1" -kind: Resource -metadata: - name: unknown - namespace: default diff --git a/pkg/cluster/command.go b/pkg/cluster/command.go new file mode 100755 index 000000000..55bfa5f02 --- /dev/null +++ b/pkg/cluster/command.go @@ -0,0 +1,52 @@ +/* + 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 + + https://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 cluster + +import ( + "fmt" + "io" + + "opendev.org/airship/airshipctl/pkg/log" + "opendev.org/airship/airshipctl/pkg/util" +) + +// StatusRunner runs internal logic of cluster status command +func StatusRunner(o StatusOptions, w io.Writer) error { + statusMap, docs, err := o.GetStatusMapDocs() + if err != nil { + return err + } + + var errors []error + tw := util.NewTabWriter(w) + fmt.Fprintf(tw, "Kind\tName\tStatus\n") + for _, doc := range docs { + status, err := statusMap.GetStatusForResource(doc) + if err != nil { + errors = append(errors, err) + } else { + fmt.Fprintf(tw, "%s\t%s\t%s\n", doc.GetKind(), doc.GetName(), status) + } + } + tw.Flush() + + if len(errors) > 0 { + log.Debug("The following errors occurred while requesting the status:") + for _, statusErr := range errors { + log.Debug(statusErr) + } + } + return nil +} diff --git a/pkg/cluster/command_test.go b/pkg/cluster/command_test.go new file mode 100755 index 000000000..60e0baa7e --- /dev/null +++ b/pkg/cluster/command_test.go @@ -0,0 +1,62 @@ +/* + 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 + + https://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 cluster_test + +import ( + "bytes" + "fmt" + "regexp" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "opendev.org/airship/airshipctl/pkg/cluster" + "opendev.org/airship/airshipctl/pkg/document" + "opendev.org/airship/airshipctl/pkg/k8s/client/fake" +) + +type mockStatusOptions struct{} + +func (o mockStatusOptions) GetStatusMapDocs() (*cluster.StatusMap, []document.Document, error) { + fakeClient := fake.NewClient( + fake.WithCRDs(makeResourceCRD(annotationValidStatusCheck())), + fake.WithDynamicObjects(makeResource("stable-resource", "stable"))) + fakeSM, err := cluster.NewStatusMap(fakeClient) + if err != nil { + return nil, nil, err + } + fakeDocBundle, err := document.NewBundleByPath("testdata/statusmap") + if err != nil { + return nil, nil, err + } + + fakeDocs, err := fakeDocBundle.GetAllDocuments() + if err != nil { + return nil, nil, err + } + return fakeSM, fakeDocs, nil +} + +func TestStatusRunner(t *testing.T) { + statusOptions := mockStatusOptions{} + b := bytes.NewBuffer(nil) + err := cluster.StatusRunner(statusOptions, b) + require.NoError(t, err) + expectedOutput := fmt.Sprintf("Kind Name Status Resource stable-resource Stable ") + space := regexp.MustCompile(`\s+`) + str := space.ReplaceAllString(b.String(), " ") + assert.Equal(t, expectedOutput, str) +} diff --git a/pkg/cluster/status.go b/pkg/cluster/status.go index c8656582c..438f9d946 100644 --- a/pkg/cluster/status.go +++ b/pkg/cluster/status.go @@ -29,10 +29,64 @@ import ( "sigs.k8s.io/cli-utils/pkg/kstatus/status" "sigs.k8s.io/cli-utils/pkg/object" + "opendev.org/airship/airshipctl/pkg/config" "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/k8s/client" ) +// A Status represents a kubernetes resource's state. +type Status string + +// StatusOptions provides a way to get status map within all the documents in the bundle +type StatusOptions interface { + GetStatusMapDocs() (*StatusMap, []document.Document, error) +} + +type statusOptions struct { + ConfigFactory config.Factory + ClientFactory client.Factory +} + +// NewStatusOptions constructs a new StatusOptions interface based on inner struct +func NewStatusOptions(cfgFactory config.Factory, clientFactory client.Factory) StatusOptions { + return &statusOptions{ConfigFactory: cfgFactory, ClientFactory: clientFactory} +} + +// GetStatusMapDocs returns status map within all the documents in the bundle +func (o *statusOptions) GetStatusMapDocs() (*StatusMap, []document.Document, error) { + conf, err := o.ConfigFactory() + if err != nil { + return nil, nil, err + } + + manifest, err := conf.CurrentContextManifest() + if err != nil { + return nil, nil, err + } + + docBundle, err := document.NewBundleByPath(manifest.TargetPath) + if err != nil { + return nil, nil, err + } + + docs, err := docBundle.GetAllDocuments() + if err != nil { + return nil, nil, err + } + + client, err := o.ClientFactory(conf) + if err != nil { + return nil, nil, err + } + + statusMap, err := NewStatusMap(client) + if err != nil { + return nil, nil, err + } + + return statusMap, docs, nil +} + // StatusMap holds a mapping of schema.GroupVersionResource to various statuses // a resource may be in, as well as the Expression used to check for that // status. diff --git a/pkg/cluster/status_test.go b/pkg/cluster/status_test.go index d381018f9..d1cfd8068 100644 --- a/pkg/cluster/status_test.go +++ b/pkg/cluster/status_test.go @@ -23,16 +23,78 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" 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/runtime/schema" "sigs.k8s.io/cli-utils/pkg/kstatus/status" "sigs.k8s.io/cli-utils/pkg/object" "opendev.org/airship/airshipctl/pkg/cluster" + "opendev.org/airship/airshipctl/pkg/config" "opendev.org/airship/airshipctl/pkg/document" + "opendev.org/airship/airshipctl/pkg/k8s/client" "opendev.org/airship/airshipctl/pkg/k8s/client/fake" "opendev.org/airship/airshipctl/testutil" ) +func TestGetStatusMapDocs(t *testing.T) { + tests := []struct { + name string + resources []runtime.Object + CRDs []runtime.Object + }{ + { + name: "get-status-map-docs-no-resources", + }, + { + name: "get-status-map-docs-with-resources", + resources: []runtime.Object{ + makeResource("stable-resource", "stable"), + makeResource("pending-resource", "pending"), + }, + CRDs: []runtime.Object{ + makeResourceCRD(annotationValidStatusCheck()), + }, + }, + } + + for _, tt := range tests { + tt := tt + settings := clusterStatusTestSettings() + fakeClient := fake.NewClient( + fake.WithDynamicObjects(tt.resources...), + fake.WithCRDs(tt.CRDs...)) + statusOptions := cluster.NewStatusOptions(func() (*config.Config, error) { + return settings, nil + }, func(_ *config.Config) (client.Interface, error) { + return fakeClient, nil + }) + + expectedSM, err := cluster.NewStatusMap(fakeClient) + require.NoError(t, err) + docBundle, err := document.NewBundleByPath(settings.Manifests["testManifest"].TargetPath) + require.NoError(t, err) + expectedDocs, err := docBundle.GetAllDocuments() + require.NoError(t, err) + + sm, docs, err := statusOptions.GetStatusMapDocs() + require.NoError(t, err) + assert.Equal(t, expectedSM, sm) + assert.Equal(t, expectedDocs, docs) + } +} + +func clusterStatusTestSettings() *config.Config { + return &config.Config{ + Contexts: map[string]*config.Context{ + "testContext": {Manifest: "testManifest"}, + }, + Manifests: map[string]*config.Manifest{ + "testManifest": {TargetPath: "testdata/statusmap"}, + }, + CurrentContext: "testContext", + } +} + func TestNewStatusMap(t *testing.T) { tests := []struct { name string