Make odo work if optional metadata.name field is missing in Devfile (…

…redhat-developer#6015)

* Move 'starter_project.go' from 'pkg/component' to 'pkg/registry'

Functions in this file are only called from the 'registry' package,
so it makes sense for this file to belong to this package.

Furthermore, this paves the way to calling 'alizer.DetectName' from 'component.go',
thus avoiding a cyclic dependency between packages.

* Introduce central logic for determining component names in 'pkg/component/component.go'

This rewrites the 'component#GatherName' function that was already there,
but not used, to meet the expectations, i.e.:

- use 'metadata.name' field (after sanitizing it) if it is defined in the Devfile
- otherwise, use Alizer to detect the name. Under the hood, this leverages the 'alizer#DetectName' introduced in 83ad3ee, which means that:
-- use Alizer to detect the name automatically
-- otherwise, use the name of the Devfile base directory after sanitizing it

* Compute and store the component name in the CLI context, and pass it as needed

As commented out in [1], the context should ideally be built
and passed down to the business clients structs.

[1] redhat-developer#6015 (comment)

* Enrich relevant integration test cases

For the sake of both performance and readability,
only the tests that break in the absence of a 'metadata.name'
field in their Devfiles have been updated (to test this specific case).

* Add test case for 'odo dev' when a project with no source code is used with no 'metadata.name' in the Devfile

The rationale behind this is to purposely make
the Alizer library unable to detect the project.
Per the requirements, this would force us to use the project
directory name as component name.

This highlights an interesting behavior if the project
directory name is all-numeric (as is the case in our tests);
our sanitization logic automatically prepends an "x" prefix
to the directory name, so it can be used as a valid name
for the component.

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
@@ -195,20 +200,19 @@ func ListAllClusterComponents(client kclient.ClientInterface, namespace string)
 	return components, nil
 }
 
-func ListAllComponents(client kclient.ClientInterface, namespace string, devObj parser.DevfileObj) ([]api.ComponentAbstract, string, error) {
+func ListAllComponents(client kclient.ClientInterface, namespace string, devObj parser.DevfileObj, componentName string) ([]api.ComponentAbstract, string, error) {
 	devfileComponents, err := ListAllClusterComponents(client, namespace)
 	if err != nil {
 		return nil, "", err
 	}
 
-	var localComponent api.ComponentAbstract
+	localComponent := api.ComponentAbstract{
+		Name:      componentName,
+		ManagedBy: "",
+		RunningIn: []api.RunningMode{},
+	}
 	if devObj.Data != nil {
-		localComponent = api.ComponentAbstract{
-			Name:      devObj.Data.GetMetadata().Name,
-			ManagedBy: "",
-			RunningIn: []api.RunningMode{},
-			Type:      GetComponentTypeFromDevfileMetadata(devObj.Data.GetMetadata()),
-		}
+		localComponent.Type = GetComponentTypeFromDevfileMetadata(devObj.Data.GetMetadata())
 	}
 
 	componentInDevfile := ""

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)
 }