From 01266f036ece49e5165a24b6fa004fe65be37f34 Mon Sep 17 00:00:00 2001 From: Ruslan Aliev Date: Thu, 10 Dec 2020 09:44:27 -0600 Subject: [PATCH] Move container phase executor to a separate package Having an executor within container 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: Ibd07c2ba46d1971604bdbd9e5e360759bb68a659 Signed-off-by: Ruslan Aliev Relates-To: #432 --- pkg/phase/client.go | 3 +- .../executors/container.go} | 38 +++++++--------- .../executors/container_test.go} | 44 ++++++++----------- 3 files changed, 36 insertions(+), 49 deletions(-) rename pkg/{container/executor.go => phase/executors/container.go} (82%) rename pkg/{container/executor_test.go => phase/executors/container_test.go} (84%) diff --git a/pkg/phase/client.go b/pkg/phase/client.go index 819610b2e..78c7ed36a 100644 --- a/pkg/phase/client.go +++ b/pkg/phase/client.go @@ -21,7 +21,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "opendev.org/airship/airshipctl/pkg/api/v1alpha1" - "opendev.org/airship/airshipctl/pkg/container" "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/events" "opendev.org/airship/airshipctl/pkg/k8s/kubeconfig" @@ -47,7 +46,7 @@ func DefaultExecutorRegistry() map[schema.GroupVersionKind]ifc.ExecutorFactory { if err := executors.RegisterIsogenExecutor(execMap); err != nil { log.Fatal(ErrExecutorRegistration{ExecutorName: "isogen", Err: err}) } - if err := container.RegisterExecutor(execMap); err != nil { + if err := executors.RegisterContainerExecutor(execMap); err != nil { log.Fatal(ErrExecutorRegistration{ExecutorName: "generic-container", Err: err}) } return execMap diff --git a/pkg/container/executor.go b/pkg/phase/executors/container.go similarity index 82% rename from pkg/container/executor.go rename to pkg/phase/executors/container.go index 810eb3cad..f5ede4f32 100644 --- a/pkg/container/executor.go +++ b/pkg/phase/executors/container.go @@ -12,7 +12,7 @@ limitations under the License. */ -package container +package executors import ( "bytes" @@ -40,10 +40,10 @@ const ( yamlSeparator = "---\n" ) -var _ ifc.Executor = &Executor{} +var _ ifc.Executor = &ContainerExecutor{} -// Executor contains resources for generic container executor -type Executor struct { +// ContainerExecutor contains resources for generic container executor +type ContainerExecutor struct { ExecutorBundle document.Bundle ExecutorDocument document.Document @@ -52,19 +52,19 @@ type Executor struct { targetPath string } -// RegisterExecutor adds executor to phase executor registry -func RegisterExecutor(registry map[schema.GroupVersionKind]ifc.ExecutorFactory) error { +// RegisterContainerExecutor adds executor to phase executor registry +func RegisterContainerExecutor(registry map[schema.GroupVersionKind]ifc.ExecutorFactory) error { obj := v1alpha1.DefaultGenericContainer() gvks, _, err := v1alpha1.Scheme.ObjectKinds(obj) if err != nil { return err } - registry[gvks[0]] = NewExecutor + registry[gvks[0]] = NewContainerExecutor return nil } -// NewExecutor creates instance of phase executor -func NewExecutor(cfg ifc.ExecutorConfig) (ifc.Executor, error) { +// NewContainerExecutor creates instance of phase executor +func NewContainerExecutor(cfg ifc.ExecutorConfig) (ifc.Executor, error) { bundle, err := cfg.BundleFactory() if err != nil { return nil, err @@ -78,7 +78,7 @@ func NewExecutor(cfg ifc.ExecutorConfig) (ifc.Executor, error) { return nil, err } - return &Executor{ + return &ContainerExecutor{ ExecutorBundle: bundle, ExecutorDocument: cfg.ExecutorDocument, @@ -91,7 +91,7 @@ func NewExecutor(cfg ifc.ExecutorConfig) (ifc.Executor, error) { } // Run generic container as a phase runner -func (c *Executor) Run(evtCh chan events.Event, opts ifc.RunOptions) { +func (c *ContainerExecutor) Run(evtCh chan events.Event, opts ifc.RunOptions) { defer close(evtCh) evtCh <- events.NewEvent().WithGenericContainerEvent(events.GenericContainerEvent{ @@ -129,7 +129,7 @@ func (c *Executor) Run(evtCh chan events.Event, opts ifc.RunOptions) { } // SetInput sets input for function -func (c *Executor) SetInput(evtCh chan events.Event) { +func (c *ContainerExecutor) SetInput(evtCh chan events.Event) { docs, err := c.ExecutorBundle.GetAllDocuments() if err != nil { handleError(evtCh, err) @@ -150,7 +150,7 @@ func (c *Executor) SetInput(evtCh chan events.Event) { } // PrepareFunctions prepares data for function -func (c *Executor) PrepareFunctions(evtCh chan events.Event) { +func (c *ContainerExecutor) PrepareFunctions(evtCh chan events.Event) { rnode, err := kyaml.Parse(c.ContConf.Config) if err != nil { handleError(evtCh, err) @@ -174,7 +174,7 @@ func (c *Executor) PrepareFunctions(evtCh chan events.Event) { } // SetMounts allows to set relative path for storage mounts to prevent security issues -func (c *Executor) SetMounts() { +func (c *ContainerExecutor) SetMounts() { if len(c.ContConf.Spec.Container.StorageMounts) == 0 { return } @@ -186,17 +186,11 @@ func (c *Executor) SetMounts() { } // Validate executor configuration and documents -func (c *Executor) Validate() error { +func (c *ContainerExecutor) Validate() error { return errors.ErrNotImplemented{} } // Render executor documents -func (c *Executor) Render(_ io.Writer, _ ifc.RenderOptions) error { +func (c *ContainerExecutor) Render(_ io.Writer, _ ifc.RenderOptions) error { return errors.ErrNotImplemented{} } - -func handleError(ch chan<- events.Event, err error) { - ch <- events.NewEvent().WithErrorEvent(events.ErrorEvent{ - Error: err, - }) -} diff --git a/pkg/container/executor_test.go b/pkg/phase/executors/container_test.go similarity index 84% rename from pkg/container/executor_test.go rename to pkg/phase/executors/container_test.go index 45bfb15f8..6e0df1aca 100644 --- a/pkg/container/executor_test.go +++ b/pkg/phase/executors/container_test.go @@ -12,7 +12,7 @@ limitations under the License. */ -package container_test +package executors_test import ( "bytes" @@ -27,15 +27,15 @@ import ( "opendev.org/airship/airshipctl/pkg/api/v1alpha1" "opendev.org/airship/airshipctl/pkg/config" - "opendev.org/airship/airshipctl/pkg/container" "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/events" "opendev.org/airship/airshipctl/pkg/phase" + "opendev.org/airship/airshipctl/pkg/phase/executors" "opendev.org/airship/airshipctl/pkg/phase/ifc" ) const ( - executorDoc = ` + containerExecutorDoc = ` apiVersion: airshipit.org/v1alpha1 kind: GenericContainer metadata: @@ -86,7 +86,7 @@ metadata: config.kubernetes.io/function: "container:\n image: quay.io/test/image:v0.0.1\nexec: {}\nstarlark: {}\n" ` - singleExecutorBundlePath = "testdata/single" + singleExecutorBundlePath = "../../container/testdata/single" firstDocInput = `--- apiVersion: v1 kind: Secret @@ -97,7 +97,7 @@ stringData: #!/bin/sh echo WORKS! $var >&2 type: Opaque` - manyExecutorBundlePath = "testdata/many" + manyExecutorBundlePath = "../../container/testdata/many" secondDocInput = `--- apiVersion: v1 kind: Secret @@ -110,27 +110,27 @@ type: Opaque yamlSeparator = "---\n" ) -func TestRegisterExecutor(t *testing.T) { +func TestRegisterContainerExecutor(t *testing.T) { registry := make(map[schema.GroupVersionKind]ifc.ExecutorFactory) expectedGVK := schema.GroupVersionKind{ Group: "airshipit.org", Version: "v1alpha1", Kind: "GenericContainer", } - err := container.RegisterExecutor(registry) + err := executors.RegisterContainerExecutor(registry) require.NoError(t, err) _, found := registry[expectedGVK] assert.True(t, found) } -func TestNewExecutor(t *testing.T) { - execDoc, err := document.NewDocumentFromBytes([]byte(executorDoc)) +func TestNewContainerExecutor(t *testing.T) { + execDoc, err := document.NewDocumentFromBytes([]byte(containerExecutorDoc)) require.NoError(t, err) - _, err = container.NewExecutor(ifc.ExecutorConfig{ + _, err = executors.NewContainerExecutor(ifc.ExecutorConfig{ ExecutorDocument: execDoc, BundleFactory: testBundleFactory(singleExecutorBundlePath), - Helper: makeDefaultHelper(t), + Helper: makeDefaultContainerHelper(t), }) require.NoError(t, err) } @@ -138,9 +138,9 @@ func TestNewExecutor(t *testing.T) { func TestSetInputSingleDocument(t *testing.T) { bundle, err := document.NewBundleByPath(singleExecutorBundlePath) require.NoError(t, err) - execDoc, err := document.NewDocumentFromBytes([]byte(executorDoc)) + execDoc, err := document.NewDocumentFromBytes([]byte(containerExecutorDoc)) require.NoError(t, err) - e := &container.Executor{ + e := &executors.ContainerExecutor{ ExecutorBundle: bundle, ExecutorDocument: execDoc, @@ -169,9 +169,9 @@ func TestSetInputSingleDocument(t *testing.T) { func TestSetInputManyDocuments(t *testing.T) { bundle, err := document.NewBundleByPath(manyExecutorBundlePath) require.NoError(t, err) - execDoc, err := document.NewDocumentFromBytes([]byte(executorDoc)) + execDoc, err := document.NewDocumentFromBytes([]byte(containerExecutorDoc)) require.NoError(t, err) - e := &container.Executor{ + e := &executors.ContainerExecutor{ ExecutorBundle: bundle, ExecutorDocument: execDoc, @@ -207,14 +207,14 @@ func TestSetInputManyDocuments(t *testing.T) { func TestPrepareFunctions(t *testing.T) { bundle, err := document.NewBundleByPath(singleExecutorBundlePath) require.NoError(t, err) - execDoc, err := document.NewDocumentFromBytes([]byte(executorDoc)) + execDoc, err := document.NewDocumentFromBytes([]byte(containerExecutorDoc)) require.NoError(t, err) contConf := &v1alpha1.GenericContainer{ Spec: runtimeutil.FunctionSpec{}, } err = execDoc.ToAPIObject(contConf, v1alpha1.Scheme) require.NoError(t, err) - e := &container.Executor{ + e := &executors.ContainerExecutor{ ExecutorBundle: bundle, ExecutorDocument: execDoc, @@ -233,16 +233,10 @@ func TestPrepareFunctions(t *testing.T) { assert.Equal(t, transformedFunction, strFuncs) } -func testBundleFactory(path string) document.BundleFactoryFunc { - return func() (document.Bundle, error) { - return document.NewBundleByPath(path) - } -} - -func makeDefaultHelper(t *testing.T) ifc.Helper { +func makeDefaultContainerHelper(t *testing.T) ifc.Helper { t.Helper() cfg := config.NewConfig() - cfg.Manifests[config.AirshipDefaultManifest].TargetPath = "./testdata" + cfg.Manifests[config.AirshipDefaultManifest].TargetPath = "../../container/testdata" cfg.Manifests[config.AirshipDefaultManifest].MetadataPath = "metadata.yaml" cfg.Manifests[config.AirshipDefaultManifest].Repositories[config.DefaultTestPhaseRepo].URLString = "" cfg.SetLoadedConfigPath(".")