Separate layers : Preference (redhat-developer#5296)

* Refactor preference package * Unit tests * Apply suggestions from code review Co-authored-by: Parthvi Vala <pvala@redhat.com> * Review * Review from Dharmit * Replace panic with LogErrorAndExit * Remove preference from kclient/oc_server * Remove preference.New from devfile * Remove preference.New from kclient/WaitAndGetPodWithEvents * Get prefClient from CreateOptions * Parthvi review Co-authored-by: Parthvi Vala <pvala@redhat.com>
rnapoles-rh · Jan 24, 2022 · d89e36d · d89e36d
1 parent 9b52733
commit d89e36d
Show file tree

Hide file tree

Showing 56 changed files with 1,458 additions and 1,111 deletions.
diff --git a/cmd/odo/odo.go b/cmd/odo/odo.go
@@ -55,7 +55,7 @@ func main() {
 		return
 	}
 
-	cfg, err := preference.New()
+	cfg, err := preference.NewClient()
 	if err != nil {
 		util.LogErrorAndExit(err, "")
 	}

diff --git a/go.mod b/go.mod
@@ -59,6 +59,7 @@ require (
 	k8s.io/klog v1.0.0
 	k8s.io/klog/v2 v2.10.0
 	k8s.io/kubectl v0.22.1
+	k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a
 	sigs.k8s.io/controller-runtime v0.10.2
 	sigs.k8s.io/yaml v1.3.0
 

diff --git a/pkg/catalog/catalog.go b/pkg/catalog/catalog.go
@@ -28,14 +28,14 @@ import (
 func GetDevfileRegistries(registryName string) ([]Registry, error) {
 	var devfileRegistries []Registry
 
-	cfg, err := preference.New()
+	cfg, err := preference.NewClient()
 	if err != nil {
 		return nil, err
 	}
 
 	hasName := len(registryName) != 0
-	if cfg.OdoSettings.RegistryList != nil {
-		registryList := *cfg.OdoSettings.RegistryList
+	if cfg.RegistryList() != nil {
+		registryList := *cfg.RegistryList()
 		// Loop backwards here to ensure the registry display order is correct (display latest newly added registry firstly)
 		for i := len(registryList) - 1; i >= 0; i-- {
 			registry := registryList[i]
@@ -119,10 +119,14 @@ func getRegistryDevfiles(registry Registry) ([]DevfileComponentType, error) {
 	request := util.HTTPRequestParams{
 		URL: indexLink,
 	}
-	secure, err := registryUtil.IsSecure(registry.Name)
+
+	// TODO(feloy) Get from DI
+	cfg, err := preference.NewClient()
 	if err != nil {
 		return nil, err
 	}
+
+	secure := registryUtil.IsSecure(cfg, registry.Name)
 	if secure {
 		token, e := keyring.Get(fmt.Sprintf("%s%s", util.CredentialPrefix, registry.Name), registryUtil.RegistryUser)
 		if e != nil {
@@ -131,11 +135,6 @@ func getRegistryDevfiles(registry Registry) ([]DevfileComponentType, error) {
 		request.Token = token
 	}
 
-	cfg, err := preference.New()
-	if err != nil {
-		return nil, err
-	}
-
 	jsonBytes, err := util.HTTPGetRequest(request, cfg.GetRegistryCacheTime())
 	if err != nil {
 		return nil, errors.Wrapf(err, "unable to download the devfile index.json from %s", indexLink)

diff --git a/pkg/component/component.go b/pkg/component/component.go
@@ -60,31 +60,26 @@ func GetComponentDir(path string) (string, error) {
 // GetDefaultComponentName generates a unique component name
 // Parameters: desired default component name(w/o prefix) and slice of existing component names
 // Returns: Unique component name and error if any
-func GetDefaultComponentName(componentPath string, componentType string, existingComponentList ComponentList) (string, error) {
+func GetDefaultComponentName(cfg preference.Client, componentPath string, componentType string, existingComponentList ComponentList) (string, error) {
 	var prefix string
+	var err error
 
 	// Get component names from component list
 	var existingComponentNames []string
 	for _, component := range existingComponentList.Items {
 		existingComponentNames = append(existingComponentNames, component.Name)
 	}
 
-	// Fetch config
-	cfg, err := preference.New()
-	if err != nil {
-		return "", errors.Wrap(err, "unable to generate random component name")
-	}
-
 	// If there's no prefix in config file, or its value is empty string use safe default - the current directory along with component type
-	if cfg.OdoSettings.NamePrefix == nil || *cfg.OdoSettings.NamePrefix == "" {
+	if cfg.NamePrefix() == nil || *cfg.NamePrefix() == "" {
 		prefix, err = GetComponentDir(componentPath)
 		if err != nil {
 			return "", errors.Wrap(err, "unable to generate random component name")
 		}
 		prefix = util.TruncateString(prefix, componentRandomNamePartsMaxLen)
 	} else {
 		// Set the required prefix into componentName
-		prefix = *cfg.OdoSettings.NamePrefix
+		prefix = *cfg.NamePrefix()
 	}
 
 	// Generate unique name for the component using prefix and unique random suffix

diff --git a/pkg/component/component_test.go b/pkg/component/component_test.go
@@ -2,7 +2,6 @@ package component
 
 import (
 	"fmt"
-	"os"
 	"reflect"
 	"regexp"
 	"testing"
@@ -17,6 +16,7 @@ import (
 	componentlabels "github.com/redhat-developer/odo/pkg/component/labels"
 	"github.com/redhat-developer/odo/pkg/kclient"
 	"github.com/redhat-developer/odo/pkg/localConfigProvider"
+	"github.com/redhat-developer/odo/pkg/preference"
 	"github.com/redhat-developer/odo/pkg/testingutil"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -253,27 +253,11 @@ func TestGetDefaultComponentName(t *testing.T) {
 	for _, tt := range tests {
 		t.Log("Running test: ", tt.testName)
 		t.Run(tt.testName, func(t *testing.T) {
-			odoConfigFile, kubeConfigFile, err := testingutil.SetUp(
-				testingutil.ConfigDetails{
-					FileName:      "odo-test-config",
-					Config:        testingutil.FakeOdoConfig("odo-test-config", false, ""),
-					ConfigPathEnv: "GLOBALODOCONFIG",
-				}, testingutil.ConfigDetails{
-					FileName:      "kube-test-config",
-					Config:        testingutil.FakeKubeClientConfig(),
-					ConfigPathEnv: "KUBECONFIG",
-				},
-			)
-			defer testingutil.CleanupEnv([]*os.File{odoConfigFile, kubeConfigFile}, t)
-			if err != nil {
-				t.Errorf("failed to setup test env. Error %v", err)
-			}
-
-			name, err := GetDefaultComponentName(tt.componentPath, tt.componentType, tt.existingComponents)
-			if err != nil {
-				t.Errorf("failed to setup mock environment. Error: %v", err)
-			}
+			ctrl := gomock.NewController(t)
+			cfg := preference.NewMockClient(ctrl)
+			cfg.EXPECT().NamePrefix().Return(nil)
 
+			name, err := GetDefaultComponentName(cfg, tt.componentPath, tt.componentType, tt.existingComponents)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("expected err: %v, but err is %v", tt.wantErr, err)
 			}

diff --git a/pkg/devfile/adapters/helper.go b/pkg/devfile/adapters/helper.go
@@ -7,6 +7,7 @@ import (
 	"github.com/redhat-developer/odo/pkg/devfile/adapters/common"
 	"github.com/redhat-developer/odo/pkg/devfile/adapters/kubernetes"
 	"github.com/redhat-developer/odo/pkg/kclient"
+	"github.com/redhat-developer/odo/pkg/preference"
 )
 
 // NewComponentAdapter returns a Devfile adapter for the targeted platform
@@ -32,17 +33,21 @@ func createKubernetesAdapter(adapterContext common.AdapterContext, namespace str
 	if err != nil {
 		return nil, err
 	}
+	prefClient, err := preference.NewClient()
+	if err != nil {
+		return nil, err
+	}
 
 	// If a namespace was passed in
 	if namespace != "" {
 		client.Namespace = namespace
 	}
-	return newKubernetesAdapter(adapterContext, client)
+	return newKubernetesAdapter(adapterContext, client, prefClient)
 }
 
-func newKubernetesAdapter(adapterContext common.AdapterContext, client kclient.ClientInterface) (common.ComponentAdapter, error) {
+func newKubernetesAdapter(adapterContext common.AdapterContext, client kclient.ClientInterface, prefClient preference.Client) (common.ComponentAdapter, error) {
 	// Feed the common metadata to the platform-specific adapter
-	kubernetesAdapter := kubernetes.New(adapterContext, client)
+	kubernetesAdapter := kubernetes.New(adapterContext, client, prefClient)
 
 	return kubernetesAdapter, nil
 }
diff --git a/pkg/devfile/adapters/helper_test.go b/pkg/devfile/adapters/helper_test.go
@@ -49,7 +49,7 @@ func TestNewPlatformAdapter(t *testing.T) {
 				Devfile:       devObj,
 			}
 			fkclient, _ := kclient.FakeNew()
-			adapter, err := newKubernetesAdapter(adapterContext, fkclient)
+			adapter, err := newKubernetesAdapter(adapterContext, fkclient, nil)
 			if err != nil {
 				t.Errorf("unexpected error: '%v'", err)
 			}

diff --git a/pkg/devfile/adapters/kubernetes/adapter.go b/pkg/devfile/adapters/kubernetes/adapter.go
@@ -6,6 +6,7 @@ import (
 	devfilev1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
 	"github.com/redhat-developer/odo/pkg/kclient"
 	"github.com/redhat-developer/odo/pkg/machineoutput"
+	"github.com/redhat-developer/odo/pkg/preference"
 
 	"github.com/pkg/errors"
 	"github.com/redhat-developer/odo/pkg/devfile/adapters/common"
@@ -22,9 +23,9 @@ type KubernetesContext struct {
 }
 
 // New instantiates a kubernetes adapter
-func New(adapterContext common.AdapterContext, client kclient.ClientInterface) Adapter {
+func New(adapterContext common.AdapterContext, client kclient.ClientInterface, prefClient preference.Client) Adapter {
 
-	compAdapter := component.New(adapterContext, client)
+	compAdapter := component.New(adapterContext, client, prefClient)
 
 	return Adapter{
 		componentAdapter: &compAdapter,

diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go
@@ -12,6 +12,7 @@ import (
 	componentlabels "github.com/redhat-developer/odo/pkg/component/labels"
 	"github.com/redhat-developer/odo/pkg/devfile"
 	"github.com/redhat-developer/odo/pkg/envinfo"
+	"github.com/redhat-developer/odo/pkg/preference"
 	"github.com/redhat-developer/odo/pkg/service"
 	"github.com/redhat-developer/odo/pkg/util"
 
@@ -38,9 +39,9 @@ import (
 const supervisorDStatusWaitTimeInterval = 1
 
 // New instantiates a component adapter
-func New(adapterContext common.AdapterContext, client kclient.ClientInterface) Adapter {
+func New(adapterContext common.AdapterContext, client kclient.ClientInterface, prefClient preference.Client) Adapter {
 
-	adapter := Adapter{Client: client}
+	adapter := Adapter{Client: client, prefClient: prefClient}
 	adapter.GenericAdapter = common.NewGenericAdapter(&adapter, adapterContext)
 	adapter.GenericAdapter.InitWith(&adapter)
 	return adapter
@@ -53,7 +54,7 @@ func (a *Adapter) getPod(refresh bool) (*corev1.Pod, error) {
 		podSelector := fmt.Sprintf("component=%s", a.ComponentName)
 
 		// Wait for Pod to be in running state otherwise we can't sync data to it.
-		pod, err := a.Client.WaitAndGetPodWithEvents(podSelector, corev1.PodRunning, "Waiting for component to start")
+		pod, err := a.Client.WaitAndGetPodWithEvents(podSelector, corev1.PodRunning, "Waiting for component to start", time.Duration(a.prefClient.GetPushTimeout())*time.Second)
 		if err != nil {
 			return nil, errors.Wrapf(err, "error while waiting for pod %s", podSelector)
 		}
@@ -91,7 +92,9 @@ func (a *Adapter) SupervisorComponentInfo(command devfilev1.Command) (common.Com
 
 // Adapter is a component adapter implementation for Kubernetes
 type Adapter struct {
-	Client kclient.ClientInterface
+	Client     kclient.ClientInterface
+	prefClient preference.Client
+
 	*common.GenericAdapter
 
 	devfileBuildCmd  string
@@ -196,7 +199,8 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) {
 
 	log.Infof("\nCreating Kubernetes resources for component %s", a.ComponentName)
 
-	err = a.createOrUpdateComponent(componentExists, parameters.EnvSpecificInfo)
+	isMainStorageEphemeral := a.prefClient.GetEphemeralSourceVolume()
+	err = a.createOrUpdateComponent(componentExists, parameters.EnvSpecificInfo, isMainStorageEphemeral)
 	if err != nil {
 		return errors.Wrap(err, "unable to create or update component")
 	}
@@ -421,7 +425,7 @@ func (a Adapter) DoesComponentExist(cmpName string, appName string) (bool, error
 	return utils.ComponentExists(a.Client, cmpName, appName)
 }
 
-func (a *Adapter) createOrUpdateComponent(componentExists bool, ei envinfo.EnvSpecificInfo) (err error) {
+func (a *Adapter) createOrUpdateComponent(componentExists bool, ei envinfo.EnvSpecificInfo, isMainStorageEphemeral bool) (err error) {
 	ei.SetDevfileObj(a.Devfile)
 
 	storageClient := storagepkg.NewClient(storagepkg.ClientOptions{
@@ -430,7 +434,7 @@ func (a *Adapter) createOrUpdateComponent(componentExists bool, ei envinfo.EnvSp
 	})
 
 	// handle the ephemeral storage
-	err = storage.HandleEphemeralStorage(a.Client, storageClient, a.ComponentName)
+	err = storage.HandleEphemeralStorage(a.Client, storageClient, a.ComponentName, isMainStorageEphemeral)
 	if err != nil {
 		return err
 	}

diff --git a/pkg/devfile/adapters/kubernetes/component/adapter_test.go b/pkg/devfile/adapters/kubernetes/component/adapter_test.go
@@ -6,10 +6,12 @@ import (
 	"testing"
 
 	"github.com/devfile/library/pkg/devfile/parser/data"
+	"github.com/golang/mock/gomock"
 
 	"github.com/devfile/library/pkg/devfile/generator"
 	"github.com/pkg/errors"
 	"github.com/redhat-developer/odo/pkg/envinfo"
+	"github.com/redhat-developer/odo/pkg/preference"
 	"github.com/redhat-developer/odo/pkg/util"
 
 	devfilev1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -136,8 +138,8 @@ func TestCreateOrUpdateComponent(t *testing.T) {
 				Name:    testComponentName,
 				AppName: testAppName,
 			})
-			componentAdapter := New(adapterCtx, fkclient)
-			err := componentAdapter.createOrUpdateComponent(tt.running, tt.envInfo)
+			componentAdapter := New(adapterCtx, fkclient, nil)
+			err := componentAdapter.createOrUpdateComponent(tt.running, tt.envInfo, false)
 
 			// Checks for unexpected error cases
 			if !tt.wantErr == (err != nil) {
@@ -350,8 +352,8 @@ func TestDoesComponentExist(t *testing.T) {
 			})
 
 			// DoesComponentExist requires an already started component, so start it.
-			componentAdapter := New(adapterCtx, fkclient)
-			err := componentAdapter.createOrUpdateComponent(false, tt.envInfo)
+			componentAdapter := New(adapterCtx, fkclient, nil)
+			err := componentAdapter.createOrUpdateComponent(false, tt.envInfo, false)
 
 			// Checks for unexpected error cases
 			if err != nil {
@@ -443,7 +445,10 @@ func TestWaitAndGetComponentPod(t *testing.T) {
 				return true, fkWatch, nil
 			})
 
-			componentAdapter := New(adapterCtx, fkclient)
+			ctrl := gomock.NewController(t)
+			prefClient := preference.NewMockClient(ctrl)
+			prefClient.EXPECT().GetPushTimeout().Return(10)
+			componentAdapter := New(adapterCtx, fkclient, prefClient)
 			_, err := componentAdapter.getPod(false)
 
 			// Checks for unexpected error cases
@@ -564,7 +569,7 @@ func TestAdapterDelete(t *testing.T) {
 
 			fkclient, fkclientset := kclient.FakeNew()
 
-			a := New(adapterCtx, fkclient)
+			a := New(adapterCtx, fkclient, nil)
 
 			fkclientset.Kubernetes.PrependReactor("delete-collection", "deployments", func(action ktesting.Action) (bool, runtime.Object, error) {
 				if util.ConvertLabelsToSelector(tt.args.labels) != action.(ktesting.DeleteCollectionAction).GetListRestrictions().Labels.String() {

diff --git a/pkg/devfile/adapters/kubernetes/component/podwatcher_test.go b/pkg/devfile/adapters/kubernetes/component/podwatcher_test.go
@@ -294,7 +294,7 @@ func TestStatusReconciler(t *testing.T) {
 
 			fkclient, _ := kclient.FakeNew()
 
-			adapter := New(adapterCtx, fkclient)
+			adapter := New(adapterCtx, fkclient, nil)
 
 			lfo := logFuncOutput{}
 			adapter.GenericAdapter.SetLogger(machineoutput.NewConsoleMachineEventLoggingClientWithFunction(lfo.logFunc))

diff --git a/pkg/devfile/adapters/kubernetes/component/status_test.go b/pkg/devfile/adapters/kubernetes/component/status_test.go
@@ -249,9 +249,9 @@ func TestGetDeploymentStatus(t *testing.T) {
 				AppName: testAppName,
 			})
 
-			componentAdapter := New(adapterCtx, fkclient)
+			componentAdapter := New(adapterCtx, fkclient, nil)
 			fkclient.Namespace = componentAdapter.Client.GetCurrentNamespace()
-			err := componentAdapter.createOrUpdateComponent(tt.running, tt.envInfo)
+			err := componentAdapter.createOrUpdateComponent(tt.running, tt.envInfo, false)
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)
 			}

diff --git a/pkg/devfile/adapters/kubernetes/storage/utils.go b/pkg/devfile/adapters/kubernetes/storage/utils.go
@@ -10,7 +10,6 @@ import (
 	componentlabels "github.com/redhat-developer/odo/pkg/component/labels"
 	"github.com/redhat-developer/odo/pkg/envinfo"
 	"github.com/redhat-developer/odo/pkg/kclient"
-	"github.com/redhat-developer/odo/pkg/preference"
 	"github.com/redhat-developer/odo/pkg/storage"
 	storagepkg "github.com/redhat-developer/odo/pkg/storage"
 	storagelabels "github.com/redhat-developer/odo/pkg/storage/labels"
@@ -187,20 +186,15 @@ func generateVolumeNameFromPVC(pvc string) (volumeName string, err error) {
 }
 
 // HandleEphemeralStorage creates or deletes the ephemeral volume based on the preference setting
-func HandleEphemeralStorage(client kclient.ClientInterface, storageClient storage.Client, componentName string) error {
-	pref, err := preference.New()
-	if err != nil {
-		return err
-	}
-
+func HandleEphemeralStorage(client kclient.ClientInterface, storageClient storage.Client, componentName string, isEphemeral bool) error {
 	selector := fmt.Sprintf("%v=%s,%s=%s", componentlabels.ComponentLabel, componentName, storagelabels.SourcePVCLabel, storage.OdoSourceVolume)
 
 	pvcs, err := client.ListPVCs(selector)
 	if err != nil && !kerrors.IsNotFound(err) {
 		return err
 	}
 
-	if !pref.GetEphemeralSourceVolume() {
+	if !isEphemeral {
 		if len(pvcs) == 0 {
 			err := storageClient.Create(storage.Storage{
 				ObjectMeta: metav1.ObjectMeta{