Skip to content

Commit

Permalink
fix(lifecycle): only prefix semver tags with 'v'
Browse files Browse the repository at this point in the history 
Change automatic v prefixing to only apply to semver tags.

Signed-off-by: Erik Cederberg <[email protected]>
  • Loading branch information
@erikced
erikced committed Mar 10, 2025
1 parent 28ee4ef commit 61dd0fa
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 142 deletions.
10 changes: 7 additions & 3 deletions pkg/porter/lifecycle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}

Expand All @@ -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
Expand Down
182 changes: 43 additions & 139 deletions pkg/porter/lifecycle_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@ package porter

import (
"context"
"fmt"
"io"
"runtime"
"sort"
"strings"
"testing"

"get.porter.sh/porter/pkg"
Expand Down Expand Up @@ -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())
}
})
}
}
Expand Down

0 comments on commit 61dd0fa

Please sign in to comment.