Fix multiple registry related issues (#3341)

* feat: fix registry issues

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

* docs: improve error/help message

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

* test: improve registry tests

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

* feat: github raw url conversion support

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

* test: refactor testing output format

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

* test: fix breaking tests

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

* refactor: add validation for raw url conversion

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

pkg/catalog/catalog.go

@@ -3,6 +3,7 @@ package catalog
 import (
 	"encoding/json"
 	"fmt"
+	"net/url"
 	"sort"
 	"strings"
 	"sync"
@@ -24,12 +25,6 @@ const (
 	apiVersion = "odo.dev/v1alpha1"
 )
 
-// DevfileRegistries contains the links of all devfile registries
-var DevfileRegistries = []string{
-	"https://raw.githubusercontent.com/elsony/devfile-registry/master",
-	"https://che-devfile-registry.openshift.io/",
-}
-
 // GetDevfileRegistries gets devfile registries from preference file,
 // if registry name is specified return the specific registry, otherwise return all registries
 func GetDevfileRegistries(registryName string) (map[string]Registry, error) {
@@ -65,11 +60,49 @@ func GetDevfileRegistries(registryName string) (map[string]Registry, error) {
 	return devfileRegistries, nil
 }
 
+// convertURL converts GitHub regular URL to GitHub raw URL, do nothing if the URL is not GitHub URL
+// For example:
+// GitHub regular URL: https://github.com/elsony/devfile-registry/tree/johnmcollier-crw
+// GitHub raw URL: https://raw.githubusercontent.com/elsony/devfile-registry/johnmcollier-crw
+func convertURL(URL string) (string, error) {
+	url, err := url.Parse(URL)
+	if err != nil {
+		return "", err
+	}
+
+	if strings.Contains(url.Host, "github") && !strings.Contains(url.Host, "raw") {
+		// Convert path part of the URL
+		URLSlice := strings.Split(URL, "/")
+		if len(URLSlice) > 2 && URLSlice[len(URLSlice)-2] == "tree" {
+			// GitHub raw URL doesn't have "tree" structure in the URL, need to remove it
+			URL = strings.Replace(URL, "/tree", "", 1)
+		} else {
+			// Add "master" branch for GitHub raw URL by default if branch is not specified
+			URL = URL + "/master"
+		}
+
+		// Convert host part of the URL
+		if url.Host == "github.com" {
+			URL = strings.Replace(URL, "github.com", "raw.githubusercontent.com", 1)
+		} else {
+			URL = strings.Replace(URL, url.Host, "raw."+url.Host, 1)
+		}
+	}
+
+	return URL, nil
+}
+
 const indexPath = "/devfiles/index.json"
 
 // getDevfileIndexEntries retrieves the devfile entries associated with the specified registry
 func getDevfileIndexEntries(registry Registry) ([]DevfileIndexEntry, error) {
 	var devfileIndex []DevfileIndexEntry
+
+	URL, err := convertURL(registry.URL)
+	if err != nil {
+		return nil, errors.Wrapf(err, "Unable to convert URL %s", registry.URL)
+	}
+	registry.URL = URL
 	indexLink := registry.URL + indexPath
 	jsonBytes, err := util.HTTPGetRequest(indexLink)
 	if err != nil {
@@ -172,10 +205,9 @@ func ListDevfileComponents(registryName string) (DevfileComponentTypeList, error
 		// Load the devfile registry index.json
 		registry := reg // needed to prevent the lambda from capturing the value
 		retrieveRegistryIndices.Add(util.ConcurrentTask{ToRun: func(errChannel chan error) {
-			indexEntries, e := getDevfileIndexEntries(registry)
-			if e != nil {
-				log.Warningf("Registry %s is not set up properly with error: %v", registryName, err)
-				errChannel <- e
+			indexEntries, err := getDevfileIndexEntries(registry)
+			if err != nil {
+				log.Warningf("Registry %s is not set up properly with error: %v", registry.Name, err)
 				return
 			}
 
@@ -204,7 +236,6 @@ func ListDevfileComponents(registryName string) (DevfileComponentTypeList, error
 			devfile, err := GetDevfile(link)
 			if err != nil {
 				log.Warningf("Registry %s is not set up properly with error: %v", devfileIndex.Registry.Name, err)
-				errChannel <- err
 				return
 			}
 

pkg/catalog/catalog_test.go

@@ -187,7 +187,7 @@ OdoSettings:
   - Name: CheDevfileRegistry
     URL: https://che-devfile-registry.openshift.io/
   - Name: DefaultDevfileRegistry
-    URL: https://raw.githubusercontent.com/elsony/devfile-registry/master`,
+    URL: https://github.com/elsony/devfile-registry`,
 	))
 	if err != nil {
 		t.Error(err)
@@ -211,7 +211,7 @@ OdoSettings:
 				},
 				"DefaultDevfileRegistry": {
 					Name: "DefaultDevfileRegistry",
-					URL:  "https://raw.githubusercontent.com/elsony/devfile-registry/master",
+					URL:  "https://github.com/elsony/devfile-registry",
 				},
 			},
 		},
@@ -576,3 +576,40 @@ func MockImageStream() *imagev1.ImageStream {
 
 	return imageStream
 }
+
+func TestConvertURL(t *testing.T) {
+	tests := []struct {
+		name    string
+		URL     string
+		wantURL string
+	}{
+		{
+			name:    "Case 1: GitHub regular URL without specifying branch",
+			URL:     "https://github.com/GeekArthur/registry",
+			wantURL: "https://raw.githubusercontent.com/GeekArthur/registry/master",
+		},
+		{
+			name:    "Case 2: GitHub regular URL with master branch specified",
+			URL:     "https://github.ibm.com/Jingfu-J-Wang/registry/tree/master",
+			wantURL: "https://raw.github.ibm.com/Jingfu-J-Wang/registry/master",
+		},
+		{
+			name:    "Case 3: GitHub regular URL with non-master branch specified",
+			URL:     "https://github.com/elsony/devfile-registry/tree/johnmcollier-crw",
+			wantURL: "https://raw.githubusercontent.com/elsony/devfile-registry/johnmcollier-crw",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotURL, err := convertURL(tt.URL)
+			if err != nil {
+				t.Error(err)
+			}
+
+			if !reflect.DeepEqual(gotURL, tt.wantURL) {
+				t.Errorf("Got url: %s, want URL: %s", gotURL, tt.wantURL)
+			}
+		})
+	}
+}

pkg/odo/cli/component/create.go

@@ -359,9 +359,16 @@ func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string
 
 		// Validate user specify registry
 		if co.devfileMetadata.devfileRegistry.Name != "" {
-			// TODO: We should add more validations here to validate registry existence and correctness
 			if co.devfileMetadata.devfilePath.value != "" {
-				return errors.New("You can specify registry via --registry if you want to use the devfile that is specified via --devfile")
+				return errors.New("You can't specify registry via --registry if you want to use the devfile that is specified via --devfile")
+			}
+
+			registryList, err := catalog.GetDevfileRegistries(co.devfileMetadata.devfileRegistry.Name)
+			if err != nil {
+				return errors.Wrap(err, "Failed to get registry")
+			}
+			if len(registryList) == 0 {
+				return errors.Errorf("Registry %s doesn't exist, please specify a valid registry via --registry", co.devfileMetadata.devfileRegistry.Name)
 			}
 		}
 
@@ -513,16 +520,29 @@ func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string
 			// Since we need to support both devfile and s2i, so we have to check if the component type is
 			// supported by devfile, if it is supported we return and will download the corresponding devfile later,
 			// if it is not supported we still need to run all the codes related with s2i after devfile compatibility check
-			spinner := log.Spinner("Checking devfile compatibility")
+
+			hasComponent := false
 
 			for _, devfileComponent := range catalogDevfileList.Items {
-				if co.devfileMetadata.componentType == devfileComponent.Name && devfileComponent.Support {
-					co.devfileMetadata.devfileSupport = true
-					co.devfileMetadata.devfileLink = devfileComponent.Link
-					co.devfileMetadata.devfileRegistry = devfileComponent.Registry
+				if co.devfileMetadata.componentType == devfileComponent.Name {
+					hasComponent = true
+					if devfileComponent.Support {
+						co.devfileMetadata.devfileSupport = true
+						co.devfileMetadata.devfileLink = devfileComponent.Link
+						co.devfileMetadata.devfileRegistry = devfileComponent.Registry
+						break
+					}
 				}
 			}
 
+			existSpinner := log.Spinner("Checking devfile existence")
+			if hasComponent {
+				existSpinner.End(true)
+			} else {
+				existSpinner.End(false)
+			}
+
+			supportSpinner := log.Spinner("Checking devfile compatibility")
 			if co.devfileMetadata.devfileSupport {
 				registrySpinner := log.Spinnerf("Creating a devfile component from registry: %s", co.devfileMetadata.devfileRegistry.Name)
 
@@ -532,13 +552,19 @@ func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string
 					return err
 				}
 
-				spinner.End(true)
+				supportSpinner.End(true)
 				registrySpinner.End(true)
 				return nil
 			}
+			supportSpinner.End(false)
 
-			spinner.End(false)
-			log.Warning("\nDevfile component type is not supported, please run `odo catalog list components` for a list of supported devfile component types")
+			// Currently only devfile component supports --registry flag, so if user specifies --registry when creating devfile component,
+			// we should error out instead of running s2i componet code and throw warning message
+			if co.devfileMetadata.devfileRegistry.Name != "" {
+				return errors.Errorf("Devfile component type %s is not supported, please run `odo catalog list components` for a list of supported devfile component types", co.devfileMetadata.componentType)
+			} else {
+				log.Warningf("Devfile component type %s is not supported, please run `odo catalog list components` for a list of supported devfile component types", co.devfileMetadata.componentType)
+			}
 		}
 	}
 

pkg/odo/cli/registry/add.go

@@ -25,7 +25,7 @@ var (
 	addExample = ktemplates.Examples(`# Add devfile registry
 	%[1]s CheRegistry https://che-devfile-registry.openshift.io
 
-	%[1]s CheRegistryFromGitHub https://raw.githubusercontent.com/eclipse/che-devfile-registry/master
+	%[1]s RegistryFromGitHub https://github.com/elsony/devfile-registry
 	`)
 )
 

pkg/preference/preference.go

@@ -77,7 +77,7 @@ const (
 	DefaultDevfileRegistryName = "DefaultDevfileRegistry"
 
 	// DefaultDevfileRegistryURL is the URL of default devfile registry
-	DefaultDevfileRegistryURL = "https://raw.githubusercontent.com/elsony/devfile-registry/master"
+	DefaultDevfileRegistryURL = "https://github.com/elsony/devfile-registry"
 )
 
 // TimeoutSettingDescription is human-readable description for the timeout setting

tests/integration/devfile/cmd_devfile_catalog_test.go

@@ -92,4 +92,17 @@ var _ = Describe("odo devfile catalog command tests", func() {
 			helper.MatchAllInOutput(output, wantOutput)
 		})
 	})
+
+	Context("When executing catalog list components with registry that is not set up properly", func() {
+		It("should list components from valid registry", func() {
+			helper.CmdShouldPass("odo", "registry", "add", "fake", "http://fake")
+			output := helper.CmdShouldPass("odo", "catalog", "list", "components")
+			helper.MatchAllInOutput(output, []string{
+				"Odo Devfile Components",
+				"java-spring-boot",
+				"quarkus",
+			})
+			helper.CmdShouldPass("odo", "registry", "delete", "fake", "-f")
+		})
+	})
 })

tests/integration/devfile/cmd_devfile_create_test.go

@@ -121,10 +121,16 @@ var _ = Describe("odo devfile create command tests", func() {
 	})
 
 	Context("When executing odo create with devfile component type argument and --registry flag", func() {
-		It("should successfully create the devfile component", func() {
+		It("should successfully create the devfile component if specified registry is valid", func() {
 			componentRegistry := "DefaultDevfileRegistry"
 			helper.CmdShouldPass("odo", "create", "java-openliberty", "--registry", componentRegistry)
 		})
+
+		It("should fail to create the devfile component if specified registry is invalid", func() {
+			componentRegistry := "fake"
+			output := helper.CmdShouldFail("odo", "create", "java-openliberty", "--registry", componentRegistry)
+			helper.MatchAllInOutput(output, []string{"Registry fake doesn't exist, please specify a valid registry via --registry"})
+		})
 	})
 
 	Context("When executing odo create with devfile component type argument and --context flag", func() {

tests/integration/devfile/docker/cmd_docker_devfile_catalog_test.go

@@ -64,4 +64,17 @@ var _ = Describe("odo docker devfile catalog command tests", func() {
 			helper.MatchAllInOutput(output, wantOutput)
 		})
 	})
+
+	Context("When executing catalog list components with registry that is not set up properly", func() {
+		It("should list components from valid registry", func() {
+			helper.CmdShouldPass("odo", "registry", "add", "fake", "http://fake")
+			output := helper.CmdShouldPass("odo", "catalog", "list", "components")
+			helper.MatchAllInOutput(output, []string{
+				"Odo Devfile Components",
+				"java-spring-boot",
+				"quarkus",
+			})
+			helper.CmdShouldPass("odo", "registry", "delete", "fake", "-f")
+		})
+	})
 })