Merge cf27963 into 39b8c7e

pkg/binding/add.go

@@ -100,6 +100,7 @@ func (o *BindingClient) AskNamingStrategy(flags map[string]string) (string, erro
 }
 
 func (o *BindingClient) AddBindingToDevfile(
+	componentName string,
 	bindingName string,
 	bindAsFiles bool,
 	serviceNs string,
@@ -112,7 +113,7 @@ func (o *BindingClient) AddBindingToDevfile(
 		return obj, err
 	}
 
-	deploymentName := fmt.Sprintf("%s-app", obj.GetMetadataName())
+	deploymentName := fmt.Sprintf("%s-app", componentName)
 	deploymentGVK, err := o.kubernetesClient.GetDeploymentAPIVersion()
 	if err != nil {
 		return obj, err

pkg/binding/binding_test.go

@@ -395,7 +395,15 @@ func TestBindingClient_AddBindingToDevfile(t *testing.T) {
 			o := &BindingClient{
 				kubernetesClient: tt.fields.kubernetesClient(ctrl),
 			}
-			got, err := o.AddBindingToDevfile(tt.args.bindingName, tt.args.bindAsFiles, tt.args.namespace, tt.args.namingStrategy, tt.args.unstructuredService, tt.args.obj)
+			got, err := o.AddBindingToDevfile(
+				tt.args.obj.GetMetadataName(),
+				tt.args.bindingName,
+				tt.args.bindAsFiles,
+				tt.args.namespace,
+				tt.args.namingStrategy,
+				tt.args.unstructuredService,
+				tt.args.obj,
+			)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("AddBindingToDevfile() error = %v, wantErr %v", err, tt.wantErr)
 				return

pkg/binding/interface.go

@@ -39,6 +39,7 @@ type Client interface {
 	AskNamingStrategy(flags map[string]string) (string, error)
 	// AddBindingToDevfile adds the ServiceBinding manifest to the devfile
 	AddBindingToDevfile(
+		componentName string,
 		bindingName string,
 		bindAsFiles bool,
 		serviceNs string,

pkg/component/component.go

@@ -11,16 +11,17 @@ import (
 	"github.com/devfile/api/v2/pkg/devfile"
 	"github.com/devfile/library/pkg/devfile/parser"
 	"github.com/devfile/library/pkg/devfile/parser/data"
-	dfutil "github.com/devfile/library/pkg/util"
+	"k8s.io/klog"
 
+	"github.com/redhat-developer/odo/pkg/alizer"
 	"github.com/redhat-developer/odo/pkg/api"
 	"github.com/redhat-developer/odo/pkg/kclient"
 	"github.com/redhat-developer/odo/pkg/labels"
 	odolabels "github.com/redhat-developer/odo/pkg/labels"
+	"github.com/redhat-developer/odo/pkg/util"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/klog"
 )
 
 const (
@@ -42,32 +43,36 @@ func GetComponentTypeFromDevfileMetadata(metadata devfile.DevfileMetadata) strin
 	return componentType
 }
 
-// GatherName parses the Devfile and retrieves an appropriate name in two ways.
-// 1. If metadata.name exists, we use it
-// 2. If metadata.name does NOT exist, we use the folder name where the devfile.yaml is located
-func GatherName(devObj parser.DevfileObj, devfilePath string) (string, error) {
-
-	metadata := devObj.Data.GetMetadata()
-
-	klog.V(4).Infof("metadata.Name: %s", metadata.Name)
-
-	// 1. Use metadata.name if it exists
-	if metadata.Name != "" {
-
-		// Remove any suffix's that end with `-`. This is because many Devfile's use the original v1 Devfile pattern of
-		// having names such as "foo-bar-" in order to prepend container names such as "foo-bar-container1"
-		return strings.TrimSuffix(metadata.Name, "-"), nil
+// GatherName computes and returns what should be used as name for the Devfile object specified.
+//
+// If a non-blank name is available in the Devfile metadata (which is optional), it is sanitized and returned.
+//
+// Otherwise, it uses Alizer to detect the name, from the project build tools (pom.xml, package.json, ...),
+// or from the component directory name.
+func GatherName(contextDir string, devfileObj *parser.DevfileObj) (string, error) {
+	var name string
+	if devfileObj != nil {
+		name = devfileObj.GetMetadataName()
+		if name == "" || strings.TrimSpace(name) == "" {
+			// Use Alizer if Devfile has no (optional) metadata.name field.
+			// We need to pass in the Devfile base directory (not the path to the devfile.yaml).
+			// Name returned by alizer.DetectName is expected to be already sanitized.
+			return alizer.DetectName(filepath.Dir(devfileObj.Ctx.GetAbsPath()))
+		}
+	} else {
+		// Fallback to the context dir name
+		baseDir, err := filepath.Abs(contextDir)
+		if err != nil {
+			return "", err
+		}
+		name = filepath.Base(baseDir)
 	}
 
-	// 2. Use the folder name as a last resort if nothing else exists
-	sourcePath, err := dfutil.GetAbsPath(devfilePath)
-	if err != nil {
-		return "", fmt.Errorf("unable to get source path: %w", err)
-	}
-	klog.V(4).Infof("Source path: %s", sourcePath)
-	klog.V(4).Infof("devfile dir: %s", filepath.Dir(sourcePath))
+	//sanitize the name
+	s := util.GetDNS1123Name(name)
+	klog.V(3).Infof("name of component is %q, and sanitized name is %q", name, s)
 
-	return filepath.Base(filepath.Dir(sourcePath)), nil
+	return s, nil
 }
 
 // GetOnePod gets a pod using the component and app name

pkg/component/component_test.go

@@ -2,17 +2,28 @@ package component
 
 import (
 	"errors"
+	"os"
+	"path"
+	"path/filepath"
 	"reflect"
 	"testing"
 
 	devfilepkg "github.com/devfile/api/v2/pkg/devfile"
+	"github.com/devfile/library/pkg/devfile"
+	"github.com/devfile/library/pkg/devfile/parser"
+	devfileCtx "github.com/devfile/library/pkg/devfile/parser/context"
+	"github.com/devfile/library/pkg/devfile/parser/data"
+	"github.com/devfile/library/pkg/testingutil/filesystem"
+	dfutil "github.com/devfile/library/pkg/util"
 	"github.com/golang/mock/gomock"
 	"github.com/kylelemons/godebug/pretty"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+
 	"github.com/redhat-developer/odo/pkg/kclient"
 	"github.com/redhat-developer/odo/pkg/labels"
-
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"github.com/redhat-developer/odo/pkg/testingutil"
+	"github.com/redhat-developer/odo/pkg/util"
 
 	"github.com/redhat-developer/odo/pkg/api"
 )
@@ -345,3 +356,130 @@ func TestGetRunningModes(t *testing.T) {
 		})
 	}
 }
+
+func TestGatherName(t *testing.T) {
+	type devfileProvider func() (*parser.DevfileObj, string, error)
+	fakeDevfileWithNameProvider := func(name string) devfileProvider {
+		return func() (*parser.DevfileObj, string, error) {
+			dData, err := data.NewDevfileData(string(data.APISchemaVersion220))
+			if err != nil {
+				return nil, "", err
+			}
+			dData.SetMetadata(devfilepkg.DevfileMetadata{Name: name})
+			return &parser.DevfileObj{
+				Ctx:  devfileCtx.FakeContext(filesystem.NewFakeFs(), parser.OutputDevfileYamlPath),
+				Data: dData,
+			}, "", nil
+		}
+	}
+
+	fs := filesystem.DefaultFs{}
+	//realDevfileWithNameProvider creates a real temporary directory and writes a devfile with the given name to it.
+	//It is the responsibility of the caller to remove the directory.
+	realDevfileWithNameProvider := func(name string) devfileProvider {
+		return func() (*parser.DevfileObj, string, error) {
+			dir, err := fs.TempDir("", "Component_GatherName_")
+			if err != nil {
+				return nil, dir, err
+			}
+
+			originalDevfile := testingutil.GetTestDevfileObjFromFile("devfile.yaml")
+			originalDevfilePath := originalDevfile.Ctx.GetAbsPath()
+
+			stat, err := os.Stat(originalDevfilePath)
+			if err != nil {
+				return nil, dir, err
+			}
+			dPath := path.Join(dir, "devfile.yaml")
+			err = dfutil.CopyFile(originalDevfilePath, dPath, stat)
+			if err != nil {
+				return nil, dir, err
+			}
+
+			var d parser.DevfileObj
+			d, _, err = devfile.ParseDevfileAndValidate(parser.ParserArgs{Path: dPath})
+			if err != nil {
+				return nil, dir, err
+			}
+
+			err = d.SetMetadataName(name)
+
+			return &d, dir, err
+		}
+	}
+
+	wantDevfileDirectoryName := func(contextDir string, d *parser.DevfileObj) string {
+		return util.GetDNS1123Name(filepath.Base(filepath.Dir(d.Ctx.GetAbsPath())))
+	}
+
+	for _, tt := range []struct {
+		name                string
+		devfileProviderFunc devfileProvider
+		wantErr             bool
+		want                func(contextDir string, d *parser.DevfileObj) string
+	}{
+		{
+			name:                "compliant name",
+			devfileProviderFunc: fakeDevfileWithNameProvider("my-component-name"),
+			want:                func(contextDir string, d *parser.DevfileObj) string { return "my-component-name" },
+		},
+		{
+			name:                "un-sanitized name",
+			devfileProviderFunc: fakeDevfileWithNameProvider("name with spaces"),
+			want:                func(contextDir string, d *parser.DevfileObj) string { return "name-with-spaces" },
+		},
+		{
+			name:                "all numeric name",
+			devfileProviderFunc: fakeDevfileWithNameProvider("123456789"),
+			// "x" prefix added by util.GetDNS1123Name
+			want: func(contextDir string, d *parser.DevfileObj) string { return "x123456789" },
+		},
+		{
+			name:                "no name",
+			devfileProviderFunc: realDevfileWithNameProvider(""),
+			want:                wantDevfileDirectoryName,
+		},
+		{
+			name:                "blank name",
+			devfileProviderFunc: realDevfileWithNameProvider("   "),
+			want:                wantDevfileDirectoryName,
+		},
+		{
+			name: "passing no devfile should use the context directory name",
+			devfileProviderFunc: func() (*parser.DevfileObj, string, error) {
+				dir, err := fs.TempDir("", "Component_GatherName_")
+				if err != nil {
+					return nil, dir, err
+				}
+				return nil, dir, nil
+			},
+			want: func(contextDir string, _ *parser.DevfileObj) string {
+				return util.GetDNS1123Name(filepath.Base(contextDir))
+			},
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			d, dir, dErr := tt.devfileProviderFunc()
+			if dir != "" {
+				defer func(fs filesystem.Filesystem, path string) {
+					if err := fs.RemoveAll(path); err != nil {
+						t.Logf("error while attempting to remove temporary directory %q: %v", path, err)
+					}
+				}(fs, dir)
+			}
+			if dErr != nil {
+				t.Errorf("error when building test Devfile object: %v", dErr)
+				return
+			}
+
+			got, err := GatherName(dir, d)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("error = %v, wantErr %v", err, tt.wantErr)
+			}
+			want := tt.want(dir, d)
+			if !reflect.DeepEqual(got, want) {
+				t.Errorf("GatherName() = %q, want = %q", got, want)
+			}
+		})
+	}
+}

pkg/component/delete/delete.go

@@ -90,12 +90,10 @@ func references(list []unstructured.Unstructured, ownerRef metav1.OwnerReference
 }
 
 // ListResourcesToDeleteFromDevfile parses all the devfile components and returns a list of resources that are present on the cluster and can be deleted
-func (do DeleteComponentClient) ListResourcesToDeleteFromDevfile(devfileObj parser.DevfileObj, appName string, mode string) (isInnerLoopDeployed bool, resources []unstructured.Unstructured, err error) {
+func (do DeleteComponentClient) ListResourcesToDeleteFromDevfile(devfileObj parser.DevfileObj, appName string, componentName string, mode string) (isInnerLoopDeployed bool, resources []unstructured.Unstructured, err error) {
 	var deployment *v1.Deployment
 	if mode == odolabels.ComponentDevMode || mode == odolabels.ComponentAnyMode {
 		// Inner Loop
-		// Fetch the deployment of the devfile component
-		componentName := devfileObj.GetMetadataName()
 		var deploymentName string
 		deploymentName, err = util.NamespaceKubernetesObject(componentName, appName)
 		if err != nil {
@@ -151,11 +149,11 @@ func (do DeleteComponentClient) ListResourcesToDeleteFromDevfile(devfileObj pars
 }
 
 // ExecutePreStopEvents executes preStop events if any, as a precondition to deleting a devfile component deployment
-func (do *DeleteComponentClient) ExecutePreStopEvents(devfileObj parser.DevfileObj, appName string) error {
+func (do *DeleteComponentClient) ExecutePreStopEvents(devfileObj parser.DevfileObj, appName string, componentName string) error {
 	if !libdevfile.HasPreStopEvents(devfileObj) {
 		return nil
 	}
-	componentName := devfileObj.GetMetadataName()
+
 	klog.V(4).Infof("Gathering information for component: %q", componentName)
 
 	klog.V(3).Infof("Checking component status for %q", componentName)

pkg/component/delete/delete_test.go

@@ -545,7 +545,7 @@ func TestDeleteComponentClient_ListResourcesToDeleteFromDevfile(t *testing.T) {
 			do := DeleteComponentClient{
 				kubeClient: tt.fields.kubeClient(ctrl),
 			}
-			gotIsInnerLoopDeployed, gotResources, err := do.ListResourcesToDeleteFromDevfile(tt.args.devfileObj, tt.args.appName, tt.args.mode)
+			gotIsInnerLoopDeployed, gotResources, err := do.ListResourcesToDeleteFromDevfile(tt.args.devfileObj, tt.args.appName, tt.args.devfileObj.GetMetadataName(), tt.args.mode)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("ListResourcesToDeleteFromDevfile() error = %v, wantErr %v", err, tt.wantErr)
 				return
@@ -702,7 +702,7 @@ func TestDeleteComponentClient_ExecutePreStopEvents(t *testing.T) {
 			do := &DeleteComponentClient{
 				kubeClient: tt.fields.kubeClient(ctrl),
 			}
-			if err := do.ExecutePreStopEvents(tt.args.devfileObj, tt.args.appName); (err != nil) != tt.wantErr {
+			if err := do.ExecutePreStopEvents(tt.args.devfileObj, tt.args.appName, tt.args.devfileObj.GetMetadataName()); (err != nil) != tt.wantErr {
 				t.Errorf("DeleteComponent() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

pkg/component/delete/interface.go

@@ -12,9 +12,9 @@ type Client interface {
 	// set wait to true to wait for all the dependencies to be deleted
 	DeleteResources(resources []unstructured.Unstructured, wait bool) []unstructured.Unstructured
 	// ExecutePreStopEvents executes preStop events if any, as a precondition to deleting a devfile component deployment
-	ExecutePreStopEvents(devfileObj parser.DevfileObj, appName string) error
+	ExecutePreStopEvents(devfileObj parser.DevfileObj, appName string, componentName string) error
 	// ListResourcesToDeleteFromDevfile parses all the devfile components and returns a list of resources that are present on the cluster that can be deleted,
 	// and a bool that indicates if the devfile component has been pushed to the innerloop
 	// the mode indicates which component to list, either Dev, Deploy or Any (using constant labels.Component*Mode)
-	ListResourcesToDeleteFromDevfile(devfileObj parser.DevfileObj, appName string, mode string) (bool, []unstructured.Unstructured, error)
+	ListResourcesToDeleteFromDevfile(devfileObj parser.DevfileObj, appName string, componentName string, mode string) (bool, []unstructured.Unstructured, error)
 }