Incorporated latest feedback

Author: Arvinderpal

cmd/clusterctl/pkg/client/repository/repository_local_unix.go

@@ -36,8 +36,8 @@ import (
 // {basepath}/{provider-name}/{version}/{components.yaml}
 //
 // (1): {provider-name} must match the value returned by Provider.Name()
-// (2): {version} must exactly obey the syntax and semantics of
-// the "Semantic Versioning" specification (http://semver.org/).
+// (2): {version} must obey the syntax and semantics of the "Semantic Versioning"
+// specification (http://semver.org/); however, "latest" is also an acceptable value.
 //
 // Concrete example:
 // /home/user/go/src/sigs.k8s.io/aws/v0.4.7/infrastructure-components.yaml
@@ -52,7 +52,6 @@ type localRepository struct {
 	basepath              string
 	providerName          string
 	defaultVersion        string
-	rootPath              string
 	componentsPath        string
 }
 
@@ -65,7 +64,7 @@ func (r *localRepository) DefaultVersion() string {
 
 // RootPath returns rootPath field of localRepository struct
 func (r *localRepository) RootPath() string {
-	return r.rootPath
+	return "" // Not applicable with localRepository
 }
 
 // ComponentsPath returns componentsPath field of localRepository struct
@@ -86,7 +85,7 @@ func (r *localRepository) GetFile(version, fileName string) ([]byte, error) {
 		version = r.defaultVersion
 	}
 
-	absolutePath := filepath.Join(r.basepath, r.providerName, version, r.rootPath, fileName)
+	absolutePath := filepath.Join(r.basepath, r.providerName, version, r.RootPath(), fileName)
 
 	if f, err := os.Stat(absolutePath); err == nil {
 		if f.IsDir() {
@@ -116,40 +115,40 @@ func newLocalRepository(providerConfig config.Provider, configVariablesClient co
 
 	var err error
 
-	urlSplit, err := url.Parse(providerConfig.URL())
+	url, err := url.Parse(providerConfig.URL())
 	if err != nil {
 		return nil, errors.Wrap(err, "invalid url")
 	}
-	absPath := urlSplit.Path
-	if urlSplit.RawPath != "" {
-		absPath = urlSplit.RawPath
+	absPath := url.Path
+	if url.RawPath != "" {
+		absPath = url.RawPath
 	}
 
 	if !filepath.IsAbs(absPath) {
 		return nil, errors.Errorf("invalid path: path %q must be an absolute path", providerConfig.URL())
 	}
 
-	parts := strings.Split(providerConfig.URL(), "/")
+	urlSplit := strings.Split(providerConfig.URL(), "/")
 	// {basepath}/{provider-name}/{version}/{components.yaml}
-	if len(parts) < 3 {
+	if len(urlSplit) < 3 {
 		return nil, errors.Errorf("invalid path: path should be in the form {basepath}/{provider-name}/{version}/{components.yaml}")
 	}
 	// We work our way backwards with {components.yaml} being the last part of the path
-	componentsPath := parts[len(parts)-1]
-	defaultVersion := parts[len(parts)-2]
+	componentsPath := urlSplit[len(urlSplit)-1]
+	defaultVersion := urlSplit[len(urlSplit)-2]
 	if defaultVersion != "latest" {
 		_, err = version.ParseSemantic(defaultVersion)
 		if err != nil {
 			return nil, errors.Errorf("invalid version: %q. Version must obey the syntax and semantics of the \"Semantic Versioning\" specification (http://semver.org/) and path format {basepath}/{provider-name}/{version}/{components.yaml}", defaultVersion)
 		}
 	}
-	providerName := parts[len(parts)-3]
+	providerName := urlSplit[len(urlSplit)-3]
 	if providerName != providerConfig.Name() {
 		return nil, errors.Errorf("invalid path: path %q must contain provider name %q in the format {basepath}/{provider-name}/{version}/{components.yaml}", providerConfig.URL(), providerConfig.Name())
 	}
 	var basePath string
-	if len(parts) > 3 {
-		basePath = filepath.Join(parts[:len(parts)-3]...)
+	if len(urlSplit) > 3 {
+		basePath = filepath.Join(urlSplit[:len(urlSplit)-3]...)
 	}
 	basePath = filepath.Clean("/" + basePath) // ensure basePath starts with "/"
 
@@ -159,7 +158,6 @@ func newLocalRepository(providerConfig config.Provider, configVariablesClient co
 		basepath:              basePath,
 		providerName:          providerName,
 		defaultVersion:        defaultVersion,
-		rootPath:              "", // Not applicable with localRepository
 		componentsPath:        componentsPath,
 	}
 
@@ -176,39 +174,25 @@ func newLocalRepository(providerConfig config.Provider, configVariablesClient co
 // getLatestRelease returns the latest release for the local repository.
 func (r *localRepository) getLatestRelease() (string, error) {
 
-	// get all the sub-directories under {basepath}/{provider-name}/
-	releasesPath := filepath.Join(r.basepath, r.providerName)
-	files, err := ioutil.ReadDir(releasesPath)
+	versions, err := r.getVersions()
 	if err != nil {
-		return "", errors.Wrap(err, "failed to list release directories")
+		return "", errors.Wrapf(err, "failed to get local repository versions")
 	}
-	// search for the latest release according to semantic version
-	// releases with names that are not semantic version number are ignored
 	var latestTag string
 	var latestReleaseVersion *version.Version
-	for _, f := range files {
-		if f.IsDir() {
-			r := f.Name()
-			sv, err := version.ParseSemantic(r)
-			if err != nil {
-				// discard releases with tags that are not a valid semantic versions (the user can point explicitly to such releases)
-				continue
-			}
-			if sv.PreRelease() != "" || sv.BuildMetadata() != "" {
-				// discard pre-releases or build releases (the user can point explicitly to such releases)
-				continue
-			}
-			if latestReleaseVersion == nil || latestReleaseVersion.LessThan(sv) {
-				latestTag = r
-				latestReleaseVersion = sv
-			}
+	for _, v := range versions {
+		sv, err := version.ParseSemantic(v)
+		if err != nil {
+			continue
+		}
+		if latestReleaseVersion == nil || latestReleaseVersion.LessThan(sv) {
+			latestTag = v
+			latestReleaseVersion = sv
 		}
 	}
-
 	if latestTag == "" {
 		return "", errors.New("failed to find releases tagged with a valid semantic version number")
 	}
-
 	return latestTag, nil
 }
 
@@ -220,21 +204,22 @@ func (r *localRepository) getVersions() ([]string, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to list release directories")
 	}
-	var versions []string
+	versions := []string{}
 	for _, f := range files {
-		if f.IsDir() {
-			r := f.Name()
-			sv, err := version.ParseSemantic(r)
-			if err != nil {
-				// discard releases with tags that are not a valid semantic versions (the user can point explicitly to such releases)
-				continue
-			}
-			if sv.PreRelease() != "" || sv.BuildMetadata() != "" {
-				// discard pre-releases or build releases (the user can point explicitly to such releases)
-				continue
-			}
-			versions = append(versions, r)
+		if !f.IsDir() {
+			continue
+		}
+		r := f.Name()
+		sv, err := version.ParseSemantic(r)
+		if err != nil {
+			// discard releases with tags that are not a valid semantic versions (the user can point explicitly to such releases)
+			continue
+		}
+		if sv.PreRelease() != "" || sv.BuildMetadata() != "" {
+			// discard pre-releases or build releases (the user can point explicitly to such releases)
+			continue
 		}
+		versions = append(versions, r)
 	}
 	return versions, nil
 }

cmd/clusterctl/pkg/client/repository/repository_local_unix_test.go

@@ -109,7 +109,7 @@ func Test_localRepository_newLocalRepository(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := newLocalRepository(tt.fields.provider, tt.fields.configVariablesClient)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("error = %v, wantErr %v", err, tt.wantErr)
+				t.Fatalf("error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}