From 71dc1d4703569f6eec75f87c0d1e79d3d987a96c Mon Sep 17 00:00:00 2001
From: Ruslan Aliev <raliev@mirantis.com>
Date: Mon, 7 Dec 2020 17:13:57 -0600
Subject: [PATCH] Move clusterctl phase executor to a separate package

Having an executor within clusterctl.client 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: Ib0a6072a393e68885d9ef911aa2894a0de055668
Signed-off-by: Ruslan Aliev <raliev@mirantis.com>
Relates-To: #374
---
 pkg/clusterctl/client/errors.go               | 10 -------
 pkg/phase/client.go                           |  4 +--
 .../executors/clusterctl.go}                  |  7 +++--
 .../executors/clusterctl_test.go}             | 20 ++++++-------
 pkg/phase/executors/errors.go                 | 29 +++++++++++++++++++
 5 files changed, 45 insertions(+), 25 deletions(-)
 rename pkg/{clusterctl/client/executor.go => phase/executors/clusterctl.go} (96%)
 mode change 100644 => 100755
 rename pkg/{clusterctl/client/executor_test.go => phase/executors/clusterctl_test.go} (93%)
 mode change 100644 => 100755
 create mode 100755 pkg/phase/executors/errors.go

diff --git a/pkg/clusterctl/client/errors.go b/pkg/clusterctl/client/errors.go
index 7eafc0752..ccd6d7431 100644
--- a/pkg/clusterctl/client/errors.go
+++ b/pkg/clusterctl/client/errors.go
@@ -36,13 +36,3 @@ type ErrProviderRepoNotFound struct {
 func (e ErrProviderRepoNotFound) Error() string {
 	return fmt.Sprintf("failed to find repository for provider %s of type %s", e.ProviderName, e.ProviderType)
 }
-
-// ErrUnknownExecutorAction is returned for unknown action parameter
-// in clusterctl configuration document
-type ErrUnknownExecutorAction struct {
-	Action string
-}
-
-func (e ErrUnknownExecutorAction) Error() string {
-	return fmt.Sprintf("unknown action type '%s'", e.Action)
-}
diff --git a/pkg/phase/client.go b/pkg/phase/client.go
index 49d473b5a..df3d13ab6 100644
--- a/pkg/phase/client.go
+++ b/pkg/phase/client.go
@@ -22,7 +22,6 @@ import (
 
 	"opendev.org/airship/airshipctl/pkg/api/v1alpha1"
 	"opendev.org/airship/airshipctl/pkg/bootstrap/isogen"
-	clusterctl "opendev.org/airship/airshipctl/pkg/clusterctl/client"
 	"opendev.org/airship/airshipctl/pkg/container"
 	"opendev.org/airship/airshipctl/pkg/document"
 	"opendev.org/airship/airshipctl/pkg/events"
@@ -30,6 +29,7 @@ import (
 	"opendev.org/airship/airshipctl/pkg/k8s/kubeconfig"
 	"opendev.org/airship/airshipctl/pkg/k8s/utils"
 	"opendev.org/airship/airshipctl/pkg/log"
+	"opendev.org/airship/airshipctl/pkg/phase/executors"
 	"opendev.org/airship/airshipctl/pkg/phase/ifc"
 )
 
@@ -40,7 +40,7 @@ type ExecutorRegistry func() map[schema.GroupVersionKind]ifc.ExecutorFactory
 func DefaultExecutorRegistry() map[schema.GroupVersionKind]ifc.ExecutorFactory {
 	execMap := make(map[schema.GroupVersionKind]ifc.ExecutorFactory)
 
-	if err := clusterctl.RegisterExecutor(execMap); err != nil {
+	if err := executors.RegisterExecutor(execMap); err != nil {
 		log.Fatal(ErrExecutorRegistration{ExecutorName: "clusterctl", Err: err})
 	}
 	if err := applier.RegisterExecutor(execMap); err != nil {
diff --git a/pkg/clusterctl/client/executor.go b/pkg/phase/executors/clusterctl.go
old mode 100644
new mode 100755
similarity index 96%
rename from pkg/clusterctl/client/executor.go
rename to pkg/phase/executors/clusterctl.go
index 80d83d6cc..4227ae566
--- a/pkg/clusterctl/client/executor.go
+++ b/pkg/phase/executors/clusterctl.go
@@ -12,7 +12,7 @@
  limitations under the License.
 */
 
-package client
+package executors
 
 import (
 	"io"
@@ -21,6 +21,7 @@ import (
 
 	airshipv1 "opendev.org/airship/airshipctl/pkg/api/v1alpha1"
 	"opendev.org/airship/airshipctl/pkg/cluster/clustermap"
+	"opendev.org/airship/airshipctl/pkg/clusterctl/client"
 	"opendev.org/airship/airshipctl/pkg/errors"
 	"opendev.org/airship/airshipctl/pkg/events"
 	"opendev.org/airship/airshipctl/pkg/k8s/kubeconfig"
@@ -34,7 +35,7 @@ var _ ifc.Executor = &ClusterctlExecutor{}
 type ClusterctlExecutor struct {
 	clusterName string
 
-	Interface
+	client.Interface
 	clusterMap clustermap.ClusterMap
 	options    *airshipv1.Clusterctl
 	kubecfg    kubeconfig.Interface
@@ -57,7 +58,7 @@ func NewExecutor(cfg ifc.ExecutorConfig) (ifc.Executor, error) {
 	if err := cfg.ExecutorDocument.ToAPIObject(options, airshipv1.Scheme); err != nil {
 		return nil, err
 	}
-	client, err := NewClient(cfg.Helper.TargetPath(), log.DebugEnabled(), options)
+	client, err := client.NewClient(cfg.Helper.TargetPath(), log.DebugEnabled(), options)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/clusterctl/client/executor_test.go b/pkg/phase/executors/clusterctl_test.go
old mode 100644
new mode 100755
similarity index 93%
rename from pkg/clusterctl/client/executor_test.go
rename to pkg/phase/executors/clusterctl_test.go
index a73725570..84a2d0037
--- a/pkg/clusterctl/client/executor_test.go
+++ b/pkg/phase/executors/clusterctl_test.go
@@ -12,7 +12,7 @@
  limitations under the License.
 */
 
-package client_test
+package executors_test
 
 import (
 	"bytes"
@@ -23,12 +23,10 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
 	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"opendev.org/airship/airshipctl/pkg/api/v1alpha1"
 	"opendev.org/airship/airshipctl/pkg/cluster/clustermap"
-	cctlclient "opendev.org/airship/airshipctl/pkg/clusterctl/client"
 	"opendev.org/airship/airshipctl/pkg/config"
 	"opendev.org/airship/airshipctl/pkg/document"
 	airerrors "opendev.org/airship/airshipctl/pkg/errors"
@@ -36,6 +34,7 @@ import (
 	"opendev.org/airship/airshipctl/pkg/fs"
 	"opendev.org/airship/airshipctl/pkg/k8s/kubeconfig"
 	"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"
 )
@@ -67,7 +66,7 @@ func TestRegisterExecutor(t *testing.T) {
 		Version: "v1alpha1",
 		Kind:    "Clusterctl",
 	}
-	err := cctlclient.RegisterExecutor(registry)
+	err := executors.RegisterExecutor(registry)
 	require.NoError(t, err)
 
 	_, found := registry[expectedGVK]
@@ -89,7 +88,7 @@ func TestNewExecutor(t *testing.T) {
 	for _, test := range testCases {
 		tt := test
 		t.Run(tt.name, func(t *testing.T) {
-			_, actualErr := cctlclient.NewExecutor(ifc.ExecutorConfig{
+			_, actualErr := executors.NewExecutor(ifc.ExecutorConfig{
 				ExecutorDocument: sampleCfgDoc,
 				Helper:           tt.helper,
 			})
@@ -114,7 +113,7 @@ func TestExecutorRun(t *testing.T) {
 			cfgDoc:     executorDoc(t, "someAction"),
 			bundlePath: "testdata/executor_init",
 			expectedEvt: []events.Event{
-				wrapError(cctlclient.ErrUnknownExecutorAction{Action: "someAction"}),
+				wrapError(executors.ErrUnknownExecutorAction{Action: "someAction"}),
 			},
 			clusterMap: clustermap.NewClusterMap(v1alpha1.DefaultClusterMap()),
 		},
@@ -168,7 +167,7 @@ func TestExecutorRun(t *testing.T) {
 				kubeconfig.FromByte([]byte("someKubeConfig")),
 				kubeconfig.InjectFileSystem(tt.fs),
 			)
-			executor, err := cctlclient.NewExecutor(
+			executor, err := executors.NewExecutor(
 				ifc.ExecutorConfig{
 					ExecutorDocument: tt.cfgDoc,
 					Helper:           makeDefaultHelper(t),
@@ -199,7 +198,7 @@ func TestExecutorRun(t *testing.T) {
 
 func TestExecutorValidate(t *testing.T) {
 	sampleCfgDoc := executorDoc(t, "init")
-	executor, err := cctlclient.NewExecutor(
+	executor, err := executors.NewExecutor(
 		ifc.ExecutorConfig{
 			ExecutorDocument: sampleCfgDoc,
 			Helper:           makeDefaultHelper(t),
@@ -209,9 +208,10 @@ func TestExecutorValidate(t *testing.T) {
 	actualErr := executor.Validate()
 	assert.Equal(t, expectedErr, actualErr)
 }
+
 func TestExecutorRender(t *testing.T) {
 	sampleCfgDoc := executorDoc(t, "init")
-	executor, err := cctlclient.NewExecutor(
+	executor, err := executors.NewExecutor(
 		ifc.ExecutorConfig{
 			ExecutorDocument: sampleCfgDoc,
 			Helper:           makeDefaultHelper(t),
@@ -226,7 +226,7 @@ func TestExecutorRender(t *testing.T) {
 func makeDefaultHelper(t *testing.T) ifc.Helper {
 	t.Helper()
 	cfg := config.NewConfig()
-	cfg.Manifests[config.AirshipDefaultManifest].TargetPath = "./testdata"
+	cfg.Manifests[config.AirshipDefaultManifest].TargetPath = "../../clusterctl/client/testdata"
 	cfg.Manifests[config.AirshipDefaultManifest].MetadataPath = "metadata.yaml"
 	cfg.Manifests[config.AirshipDefaultManifest].Repositories[config.DefaultTestPhaseRepo].URLString = ""
 	cfg.SetLoadedConfigPath(".")
diff --git a/pkg/phase/executors/errors.go b/pkg/phase/executors/errors.go
new file mode 100755
index 000000000..ca49e5977
--- /dev/null
+++ b/pkg/phase/executors/errors.go
@@ -0,0 +1,29 @@
+/*
+ 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 executors
+
+import (
+	"fmt"
+)
+
+// ErrUnknownExecutorAction is returned for unknown action parameter
+// in clusterctl configuration document
+type ErrUnknownExecutorAction struct {
+	Action string
+}
+
+func (e ErrUnknownExecutorAction) Error() string {
+	return fmt.Sprintf("unknown action type '%s'", e.Action)
+}