From b85f8fa8f912a56ab3e8f5d573c14e15f5d2d348 Mon Sep 17 00:00:00 2001 From: Ruslan Aliev Date: Thu, 10 Dec 2020 09:27:55 -0600 Subject: [PATCH] Move kubernetes-apply phase executor to a separate package Having an executor within applier package creates potential import cycling. This patch moves it to a separate package which can be used to conveniently store all the executors at one place. Change-Id: I68d909489b691e4fb7129446ef9a3fb085f8683c Signed-off-by: Ruslan Aliev Relates-To: #432 --- pkg/phase/client.go | 3 +- .../executors/k8s_applier.go} | 19 ++++---- .../executors/k8s_applier_test.go} | 48 ++++++++----------- 3 files changed, 32 insertions(+), 38 deletions(-) rename pkg/{k8s/applier/executor.go => phase/executors/k8s_applier.go} (87%) rename pkg/{k8s/applier/executor_test.go => phase/executors/k8s_applier_test.go} (86%) diff --git a/pkg/phase/client.go b/pkg/phase/client.go index 88cbe0ee5..819610b2e 100644 --- a/pkg/phase/client.go +++ b/pkg/phase/client.go @@ -24,7 +24,6 @@ import ( "opendev.org/airship/airshipctl/pkg/container" "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/events" - "opendev.org/airship/airshipctl/pkg/k8s/applier" "opendev.org/airship/airshipctl/pkg/k8s/kubeconfig" "opendev.org/airship/airshipctl/pkg/k8s/utils" "opendev.org/airship/airshipctl/pkg/log" @@ -42,7 +41,7 @@ func DefaultExecutorRegistry() map[schema.GroupVersionKind]ifc.ExecutorFactory { if err := executors.RegisterExecutor(execMap); err != nil { log.Fatal(ErrExecutorRegistration{ExecutorName: "clusterctl", Err: err}) } - if err := applier.RegisterExecutor(execMap); err != nil { + if err := executors.RegisterKubeApplierExecutor(execMap); err != nil { log.Fatal(ErrExecutorRegistration{ExecutorName: "kubernetes-apply", Err: err}) } if err := executors.RegisterIsogenExecutor(execMap); err != nil { diff --git a/pkg/k8s/applier/executor.go b/pkg/phase/executors/k8s_applier.go similarity index 87% rename from pkg/k8s/applier/executor.go rename to pkg/phase/executors/k8s_applier.go index 3da91ee58..602d66e5e 100644 --- a/pkg/k8s/applier/executor.go +++ b/pkg/phase/executors/k8s_applier.go @@ -12,7 +12,7 @@ limitations under the License. */ -package applier +package executors import ( "io" @@ -26,6 +26,7 @@ import ( "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/errors" "opendev.org/airship/airshipctl/pkg/events" + k8sapplier "opendev.org/airship/airshipctl/pkg/k8s/applier" "opendev.org/airship/airshipctl/pkg/k8s/kubeconfig" "opendev.org/airship/airshipctl/pkg/k8s/utils" "opendev.org/airship/airshipctl/pkg/log" @@ -46,8 +47,8 @@ type ExecutorOptions struct { var _ ifc.Executor = &Executor{} -// RegisterExecutor adds executor to phase executor registry -func RegisterExecutor(registry map[schema.GroupVersionKind]ifc.ExecutorFactory) error { +// RegisterKubeApplierExecutor adds executor to phase executor registry +func RegisterKubeApplierExecutor(registry map[schema.GroupVersionKind]ifc.ExecutorFactory) error { obj := &airshipv1.KubernetesApply{} gvks, _, err := airshipv1.Scheme.ObjectKinds(obj) if err != nil { @@ -59,7 +60,7 @@ func RegisterExecutor(registry map[schema.GroupVersionKind]ifc.ExecutorFactory) // registerExecutor is here so that executor in theory can be used outside phases func registerExecutor(cfg ifc.ExecutorConfig) (ifc.Executor, error) { - return NewExecutor(ExecutorOptions{ + return NewKubeApplierExecutor(ExecutorOptions{ ClusterName: cfg.ClusterName, BundleName: cfg.PhaseName, Helper: cfg.Helper, @@ -79,8 +80,8 @@ type Executor struct { cleanup kubeconfig.Cleanup } -// NewExecutor returns instance of executor -func NewExecutor(opts ExecutorOptions) (*Executor, error) { +// NewKubeApplierExecutor returns instance of executor +func NewKubeApplierExecutor(opts ExecutorOptions) (*Executor, error) { apiObj := &airshipv1.KubernetesApply{} err := opts.ExecutorDocument.ToAPIObject(apiObj, airshipv1.Scheme) if err != nil { @@ -116,7 +117,7 @@ func (e *Executor) Run(ch chan events.Event, runOpts ifc.RunOptions) { } log.Debugf("WaitTimeout: %v", timeout) - applyOptions := ApplyOptions{ + applyOptions := k8sapplier.ApplyOptions{ DryRunStrategy: dryRunStrategy, Prune: e.apiObject.Config.PruneOptions.Prune, BundleName: e.Options.BundleName, @@ -125,7 +126,7 @@ func (e *Executor) Run(ch chan events.Event, runOpts ifc.RunOptions) { applier.ApplyBundle(filteredBundle, applyOptions) } -func (e *Executor) prepareApplier(ch chan events.Event) (*Applier, document.Bundle, error) { +func (e *Executor) prepareApplier(ch chan events.Event) (*k8sapplier.Applier, document.Bundle, error) { log.Debug("Getting kubeconfig context name from cluster map") context, err := e.Options.ClusterMap.ClusterKubeconfigContext(e.Options.ClusterName) if err != nil { @@ -146,7 +147,7 @@ func (e *Executor) prepareApplier(ch chan events.Event) (*Applier, document.Bund e.cleanup = cleanup log.Debugf("Using kubeconfig at '%s' and context '%s'", path, context) factory := utils.FactoryFromKubeConfig(path, context) - return NewApplier(ch, factory), bundle, nil + return k8sapplier.NewApplier(ch, factory), bundle, nil } // Validate document set diff --git a/pkg/k8s/applier/executor_test.go b/pkg/phase/executors/k8s_applier_test.go similarity index 86% rename from pkg/k8s/applier/executor_test.go rename to pkg/phase/executors/k8s_applier_test.go index 1a712df08..0ef0025ac 100644 --- a/pkg/k8s/applier/executor_test.go +++ b/pkg/phase/executors/k8s_applier_test.go @@ -12,7 +12,7 @@ limitations under the License. */ -package applier_test +package executors_test import ( "bytes" @@ -27,10 +27,10 @@ import ( "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/events" "opendev.org/airship/airshipctl/pkg/fs" - "opendev.org/airship/airshipctl/pkg/k8s/applier" "opendev.org/airship/airshipctl/pkg/k8s/kubeconfig" "opendev.org/airship/airshipctl/pkg/k8s/utils" "opendev.org/airship/airshipctl/pkg/phase" + "opendev.org/airship/airshipctl/pkg/phase/executors" "opendev.org/airship/airshipctl/pkg/phase/ifc" testfs "opendev.org/airship/airshipctl/testutil/fs" ) @@ -83,7 +83,7 @@ users: ` ) -func TestNewExecutor(t *testing.T) { +func TestNewKubeApplierExecutor(t *testing.T) { tests := []struct { name string cfgDoc string @@ -96,8 +96,8 @@ func TestNewExecutor(t *testing.T) { name: "valid executor", cfgDoc: ValidExecutorDoc, kubeconf: testKubeconfig(testValidKubeconfig), - helper: makeDefaultHelper(t), - bundleFactory: testBundleFactory("testdata/source_bundle"), + helper: makeKubeApplierDefaultHelper(t), + bundleFactory: testBundleFactory("../../k8s/applier/testdata/source_bundle"), }, { name: "wrong config document", @@ -109,8 +109,8 @@ metadata: labels: cli-utils.sigs.k8s.io/inventory-id: "some id"`, expectedErr: "wrong config document", - helper: makeDefaultHelper(t), - bundleFactory: testBundleFactory("testdata/source_bundle"), + helper: makeKubeApplierDefaultHelper(t), + bundleFactory: testBundleFactory("../../k8s/applier/testdata/source_bundle"), }, { @@ -118,7 +118,7 @@ metadata: cfgDoc: ValidExecutorDoc, expectedErr: "no such file or directory", kubeconf: testKubeconfig(testValidKubeconfig), - helper: makeDefaultHelper(t), + helper: makeKubeApplierDefaultHelper(t), bundleFactory: testBundleFactory("does not exist"), }, } @@ -130,8 +130,8 @@ metadata: require.NoError(t, err) require.NotNil(t, doc) - exec, err := applier.NewExecutor( - applier.ExecutorOptions{ + exec, err := executors.NewKubeApplierExecutor( + executors.ExecutorOptions{ ExecutorDocument: doc, BundleFactory: tt.bundleFactory, Kubeconfig: tt.kubeconf, @@ -152,7 +152,7 @@ metadata: // TODO We need valid test that checks that actual bundle has arrived to applier // for that we need a way to inject fake applier, which is not doable with `black box` test currently // since we tests are in different package from executor -func TestExecutorRun(t *testing.T) { +func TestKubeApplierExecutorRun(t *testing.T) { tests := []struct { name string containsErr string @@ -167,8 +167,8 @@ func TestExecutorRun(t *testing.T) { { name: "cant read kubeconfig error", containsErr: "no such file or directory", - helper: makeDefaultHelper(t), - bundleFactory: testBundleFactory("testdata/source_bundle"), + helper: makeKubeApplierDefaultHelper(t), + bundleFactory: testBundleFactory("../../k8s/applier/testdata/source_bundle"), kubeconf: testKubeconfig(`invalid kubeconfig`), execDoc: toKubernetesApply(t, ValidExecutorDocNamespaced), clusterName: "ephemeral-cluster", @@ -181,8 +181,8 @@ func TestExecutorRun(t *testing.T) { { name: "error cluster not defined", containsErr: "cluster is not defined in in cluster map", - helper: makeDefaultHelper(t), - bundleFactory: testBundleFactory("testdata/source_bundle"), + helper: makeKubeApplierDefaultHelper(t), + bundleFactory: testBundleFactory("../../k8s/applier/testdata/source_bundle"), kubeconf: testKubeconfig(testValidKubeconfig), execDoc: toKubernetesApply(t, ValidExecutorDocNamespaced), clusterMap: clustermap.NewClusterMap(v1alpha1.DefaultClusterMap()), @@ -191,8 +191,8 @@ func TestExecutorRun(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - exec, err := applier.NewExecutor( - applier.ExecutorOptions{ + exec, err := executors.NewKubeApplierExecutor( + executors.ExecutorOptions{ ExecutorDocument: tt.execDoc, Helper: tt.helper, BundleFactory: tt.bundleFactory, @@ -225,8 +225,8 @@ func TestRender(t *testing.T) { execDoc, err := document.NewDocumentFromBytes([]byte(ValidExecutorDoc)) require.NoError(t, err) require.NotNil(t, execDoc) - exec, err := applier.NewExecutor(applier.ExecutorOptions{ - BundleFactory: testBundleFactory("testdata/source_bundle"), + exec, err := executors.NewKubeApplierExecutor(executors.ExecutorOptions{ + BundleFactory: testBundleFactory("../../k8s/applier/testdata/source_bundle"), ExecutorDocument: execDoc, }) require.NoError(t, err) @@ -240,7 +240,7 @@ func TestRender(t *testing.T) { assert.Contains(t, result, "ReplicationController") } -func makeDefaultHelper(t *testing.T) ifc.Helper { +func makeKubeApplierDefaultHelper(t *testing.T) ifc.Helper { t.Helper() conf := &config.Config{ CurrentContext: "default", @@ -252,7 +252,7 @@ func makeDefaultHelper(t *testing.T) ifc.Helper { Manifests: map[string]*config.Manifest{ "default-manifest": { MetadataPath: "metadata.yaml", - TargetPath: "testdata", + TargetPath: "../../k8s/applier/testdata", PhaseRepositoryName: config.DefaultTestPhaseRepo, Repositories: map[string]*config.Repository{ config.DefaultTestPhaseRepo: { @@ -292,9 +292,3 @@ func testKubeconfig(stringData string) kubeconfig.Interface { }, )) } - -func testBundleFactory(path string) document.BundleFactoryFunc { - return func() (document.Bundle, error) { - return document.NewBundleByPath(path) - } -}