fix(lifecycle): only prefix semver tags with 'v'

Change automatic v prefixing to only apply to semver tags.

Signed-off-by: Erik Cederberg <erik.cederberg@sectra.com>

Author: erikced

pkg/porter/lifecycle.go

@@ -364,7 +364,7 @@ func (p *Porter) BuildActionArgs(ctx context.Context, installation storage.Insta
 }
 
 // ensureVPrefix adds a "v" prefix to the version tag if it's not already there.
-// Version tag should always be prefixed with a "v", see https://github.com/getporter/porter/issues/2886.
+// Semver version tags tag should always be prefixed with a "v", see https://github.com/getporter/porter/issues/2886.
 // This is safe because "porter publish" adds a "v", see
 // https://github.com/getporter/porter/blob/17bd7816ef6bde856793f6122e32274aa9d01d1b/pkg/storage/installation.go#L350
 func ensureVPrefix(opts *BundleReferenceOptions, out io.Writer) error {
@@ -379,8 +379,8 @@ func ensureVPrefix(opts *BundleReferenceOptions, out io.Writer) error {
 		ociRef = &ref
 	}
 
-	if ociRef.Tag() == "" || ociRef.Tag() == "latest" || strings.HasPrefix(ociRef.Tag(), "v") {
-		// don't do anything if missing tag, if tag is "latest", or if "v" is already there
+	// Do nothing for empty tags, non semver-tags or if tag is not a version
+	if !tagStartsWithNumber(ociRef) || !ociRef.HasVersion() {
 		return nil
 	}
 
@@ -398,6 +398,10 @@ func ensureVPrefix(opts *BundleReferenceOptions, out io.Writer) error {
 	return nil
 }
 
+func tagStartsWithNumber(ociRef *cnab.OCIReference) bool {
+	return ociRef.HasTag() && ociRef.Tag()[0] >= '0' && ociRef.Tag()[0] <= '9'
+}
+
 // prepullBundleByReference handles calling the bundle pull operation and updating
 // the shared options like name and bundle file path. This is used by install, upgrade
 // and uninstall

pkg/porter/lifecycle_test.go

@@ -2,11 +2,9 @@ package porter
 
 import (
 	"context"
-	"fmt"
 	"io"
 	"runtime"
 	"sort"
-	"strings"
 	"testing"
 
 	"get.porter.sh/porter/pkg"
@@ -540,167 +538,71 @@ func TestPorter_applyActionOptionsToInstallation_PreservesExistingParams(t *test
 	}, params, "Incorrect parameter values were persisted on the installationß")
 }
 
-// make sure ensureVPrefix correctly adds a 'v' to the version tag of a reference
 func Test_ensureVPrefix(t *testing.T) {
-	type args struct {
-		opts *BundleReferenceOptions
-	}
-
 	ref, err := cnab.ParseOCIReference("registry/bundle:1.2.3")
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	testCases := []struct {
-		name    string
-		args    args
-		wantErr assert.ErrorAssertionFunc
+		name       string
+		reference  string
+		ref        *cnab.OCIReference
+		want       string
+		wantRefTag string
 	}{
 		{
-			name: "add v to Reference (nil _ref)",
-			args: struct{ opts *BundleReferenceOptions }{opts: &BundleReferenceOptions{
-				installationOptions: installationOptions{},
-				BundlePullOptions: BundlePullOptions{
-					Reference:        "registry/bundle:1.2.3",
-					_ref:             nil,
-					InsecureRegistry: false,
-					Force:            false,
-				},
-				bundleRef: nil,
-			},
-			}, wantErr: assert.NoError,
+			name:      "adds v prefix to semver reference",
+			reference: "registry/bundle:1.2.3",
+			ref:       nil,
+			want:      "registry/bundle:v1.2.3",
 		},
 		{
-			name: "add v to Reference (non-nil _ref)",
-			args: struct{ opts *BundleReferenceOptions }{opts: &BundleReferenceOptions{
-				installationOptions: installationOptions{},
-				BundlePullOptions: BundlePullOptions{
-					Reference:        "registry/bundle:1.2.3",
-					_ref:             &ref,
-					InsecureRegistry: false,
-					Force:            false,
-				},
-				bundleRef: nil,
-			},
-			}, wantErr: assert.NoError,
+			name:       "updates _ref if present",
+			reference:  "registry/bundle:1.2.3",
+			ref:        &ref,
+			want:       "registry/bundle:v1.2.3",
+			wantRefTag: "v1.2.3",
 		},
-	}
-	for _, tt := range testCases {
-		t.Run(tt.name, func(t *testing.T) {
-			// before
-			idx := strings.LastIndex(tt.args.opts.Reference, ":")
-			assert.False(t, tt.args.opts.Reference[idx+1] == 'v')
-			if tt.args.opts._ref != nil {
-				assert.False(t, tt.args.opts._ref.Tag()[0] == 'v')
-			}
-
-			err := ensureVPrefix(tt.args.opts, io.Discard)
-
-			// after
-			tt.wantErr(t, err, fmt.Sprintf("ensureVPrefix(%v)", tt.args.opts))
-
-			idx = strings.LastIndex(tt.args.opts.Reference, ":")
-			assert.True(t, tt.args.opts.Reference[idx+1] == 'v')
-			if tt.args.opts._ref != nil {
-				assert.True(t, tt.args.opts._ref.Tag()[0] == 'v')
-			}
-		})
-	}
-}
-
-// make sure ensureVPrefix doesn't add an extra 'v' to the version tag of a reference if it's already there
-func Test_ensureVPrefix_idempotent(t *testing.T) {
-	type args struct {
-		opts *BundleReferenceOptions
-	}
-
-	vRef, err := cnab.ParseOCIReference("registry/bundle:v1.2.3")
-	assert.NoError(t, err)
-
-	testcasesIdempotent := []struct {
-		name    string
-		args    args
-		wantErr assert.ErrorAssertionFunc
-	}{
 		{
-			name: "don't add v to Reference with existing v (nil _ref)",
-			args: struct{ opts *BundleReferenceOptions }{opts: &BundleReferenceOptions{
-				installationOptions: installationOptions{},
-				BundlePullOptions: BundlePullOptions{
-					Reference:        "registry/bundle:v1.2.3",
-					_ref:             nil,
-					InsecureRegistry: false,
-					Force:            false,
-				},
-				bundleRef: nil,
-			},
-			}, wantErr: assert.NoError,
+			name:      "is idempotent",
+			reference: "registry/bundle:v1.2.3",
+			ref:       nil,
+			want:      "registry/bundle:v1.2.3",
 		},
 		{
-			name: "don't add v to Reference with existing v (non-nil _ref)",
-			args: struct{ opts *BundleReferenceOptions }{opts: &BundleReferenceOptions{
-				installationOptions: installationOptions{},
-				BundlePullOptions: BundlePullOptions{
-					Reference:        "registry/bundle:v1.2.3",
-					_ref:             &vRef,
-					InsecureRegistry: false,
-					Force:            false,
-				},
-				bundleRef: nil,
-			},
-			}, wantErr: assert.NoError,
+			name:      "ignores non-semver references",
+			reference: "registry/bundle:latest",
+			ref:       nil,
+			want:      "registry/bundle:latest",
+		},
+		{
+			name:      "ignores references with no tag",
+			reference: "registry/bundle",
+			ref:       nil,
+			want:      "registry/bundle",
 		},
 	}
-	for _, tt := range testcasesIdempotent {
-		t.Run(tt.name, func(t *testing.T) {
-			// before
-			idx := strings.LastIndex(tt.args.opts.Reference, ":")
-			assert.True(t, tt.args.opts.Reference[idx+1] == 'v')
-			assert.True(t, tt.args.opts.Reference[idx+2] != 'v')
-			if tt.args.opts._ref != nil {
-				assert.True(t, tt.args.opts._ref.Tag()[0] == 'v')
-				assert.True(t, tt.args.opts._ref.Tag()[1] != 'v')
-			}
-
-			err := ensureVPrefix(tt.args.opts, io.Discard)
-
-			// after
-			tt.wantErr(t, err, fmt.Sprintf("ensureVPrefix(%v)", tt.args.opts))
-
-			idx = strings.LastIndex(tt.args.opts.Reference, ":")
-			assert.True(t, tt.args.opts.Reference[idx+1] == 'v')
-			assert.True(t, tt.args.opts.Reference[idx+2] != 'v')
-			if tt.args.opts._ref != nil {
-				assert.True(t, tt.args.opts._ref.Tag()[0] == 'v')
-				assert.True(t, tt.args.opts._ref.Tag()[1] != 'v')
-			}
-		})
-	}
-}
 
-// ensure no v is added if specifying :latest tag or no tag at all
-func Test_ensureVPrefix_latest(t *testing.T) {
-	testcases := map[string]struct {
-		latestRef string
-	}{
-		"latest":     {latestRef: "example/porter-hello:latest"},
-		"not-latest": {latestRef: "example/porter-hello"},
-	}
-	for name, test := range testcases {
-		t.Run(name, func(t *testing.T) {
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
 			opts := BundleReferenceOptions{
 				installationOptions: installationOptions{},
 				BundlePullOptions: BundlePullOptions{
-					Reference:        test.latestRef,
-					_ref:             nil,
+					Reference:        tc.reference,
+					_ref:             tc.ref,
 					InsecureRegistry: false,
 					Force:            false,
 				},
 				bundleRef: nil,
 			}
-			err := ensureVPrefix(&opts, io.Discard)
-			assert.NoError(t, err)
 
-			// shouldn't change
-			assert.Equal(t, test.latestRef, opts.BundlePullOptions.Reference)
+			err = ensureVPrefix(&opts, io.Discard)
+
+			assert.Equal(t, tc.want, opts.BundlePullOptions.Reference)
+			assert.NoError(t, err)
+			if tc.wantRefTag != "" {
+				require.NotNil(t, opts.BundlePullOptions._ref)
+				assert.Equal(t, tc.wantRefTag, opts.BundlePullOptions._ref.Tag())
+			}
 		})
 	}
 }