diff --git a/pkg/porter/lifecycle.go b/pkg/porter/lifecycle.go index 1ea9927fa..7c38d1733 100644 --- a/pkg/porter/lifecycle.go +++ b/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, tags that do not start with a number and non-semver tags + 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 diff --git a/pkg/porter/lifecycle_test.go b/pkg/porter/lifecycle_test.go index 9d1d2878c..5d37f5019 100644 --- a/pkg/porter/lifecycle_test.go +++ b/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,73 @@ 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 == "" { + assert.Nil(t, opts.BundlePullOptions._ref) + } else { + require.NotNil(t, opts.BundlePullOptions._ref) + assert.Equal(t, tc.wantRefTag, opts.BundlePullOptions._ref.Tag()) + } }) } }