Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version parsing rehaul #228

Merged
merged 4 commits into from
Apr 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions plugin/set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ func TestSet(t *testing.T) {
set.RegisterProvisioner("example-2", new(MockProvisioner))
set.RegisterDatasource("example", new(MockDatasource))
set.RegisterDatasource("example-2", new(MockDatasource))
set.SetVersion(pluginVersion.InitializePluginVersion(
"1.1.1", ""))
set.SetVersion(pluginVersion.NewPluginVersion(
"1.1.1", "", ""))

outputDesc := set.description()

sdkVersion := pluginVersion.InitializePluginVersion(pluginVersion.Version, pluginVersion.VersionPrerelease)
sdkVersion := pluginVersion.NewPluginVersion(pluginVersion.Version, pluginVersion.VersionPrerelease, "")
if diff := cmp.Diff(SetDescription{
Version: "1.1.1",
SDKVersion: sdkVersion.String(),
Expand Down
108 changes: 79 additions & 29 deletions version/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
package version

import (
"bytes"
"fmt"

"github.com/hashicorp/go-version"
Expand All @@ -23,29 +22,71 @@ var Version = "0.5.3"
// such as "dev" (in development), "beta", "rc1", etc.
var VersionPrerelease = "dev"

// The metadata for the version, this is optional information to add around
// a particular release.
//
// This has no impact on the ordering of plugins, and is ignored for non-human eyes.
var VersionMetadata = ""

// SDKVersion is used by the plugin set to allow Packer to recognize
// what version of the sdk the plugin is.
var SDKVersion = InitializePluginVersion(Version, VersionPrerelease)
var SDKVersion = NewPluginVersion(Version, VersionPrerelease, VersionMetadata)

// InitializePluginVersion initializes the SemVer and returns a version var.
// If the provided "version" string is not valid, the call to version.Must
// will panic. Therefore, this function should always be called in a package
// init() function to make sure that plugins are following proper semantic
// versioning and to make sure that plugins which aren't following proper
// semantic versioning crash immediately rather than later.
//
// Deprecated: InitializePluginVersion does not support metadata out of the
// box, and should be replaced by either NewPluginVersion or NewRawVersion.
func InitializePluginVersion(vers, versionPrerelease string) *PluginVersion {
if vers == "" {
return NewPluginVersion(vers, versionPrerelease, "")
}

// NewRawVersion is made for more freeform version strings. It won't accept
// much more than what `NewPluginVersion` already does, but is another
// convenient form to create a version if preferred.
//
// As NewRawVersion, if the version is invalid, it will panic.
func NewRawVersion(rawSemVer string) *PluginVersion {
lbajolet-hashicorp marked this conversation as resolved.
Show resolved Hide resolved
vers := version.Must(version.NewVersion(rawSemVer))

if len(vers.Segments()) != 3 {
panic(fmt.Sprintf("versions should only have 3 segments, %q had %d", rawSemVer, len(vers.Segments())))
}

return &PluginVersion{
version: vers.Core().String(),
versionPrerelease: vers.Prerelease(),
versionMetadata: vers.Metadata(),
semVer: vers,
}
}

// NewPluginVersion initializes the SemVer and returns a PluginVersion from it.
// If the provided "version" string is not valid, the call to version.Must
// will panic.
//
// This function should always be called in a package init() function to make
// sure that plugins are following proper semantic versioning and to make sure
// that plugins which aren't following proper semantic versioning crash
// immediately rather than later.
//
// If the core version number is empty, it will default to 0.0.0.
func NewPluginVersion(vers, versionPrerelease, versionMetadata string) *PluginVersion {
var versionRawString = vers

if versionRawString == "" {
// Defaults to "0.0.0". Useful when binary is created for development purpose.
vers = "0.0.0"
versionRawString = "0.0.0"
}
pv := PluginVersion{
version: vers,
versionPrerelease: versionPrerelease,

if versionPrerelease != "" {
versionRawString = fmt.Sprintf("%s-%s", versionRawString, versionPrerelease)
}

if versionMetadata != "" {
versionRawString = fmt.Sprintf("%s+%s", versionRawString, versionMetadata)
}
// This call initializes the SemVer to make sure that if Packer crashes due
// to an invalid SemVer it's at the very beginning of the Packer run.
pv.semVer = version.Must(version.NewVersion(vers))
return &pv

return NewRawVersion(versionRawString)
}

type PluginVersion struct {
Expand All @@ -55,22 +96,30 @@ type PluginVersion struct {
// then it means that it is a final release. Otherwise, this is a pre-release
// such as "dev" (in development), "beta", "rc1", etc.
versionPrerelease string
// Extra metadata that can be part of the version.
//
// This is legal in semver, and has to be the last part of the version
// string, starting with a `+`.
versionMetadata string
// The Semantic Version of the plugin. Used for version constraint comparisons
semVer *version.Version
}

func (p *PluginVersion) SetMetadata(meta string) {
p.versionMetadata = meta
}

func (p *PluginVersion) FormattedVersion() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I should call that a "breaking" change, but there's some potential extra information here, normally if a plugin creates their version with core + pre-release, the format should not change at all.
If there's a metadata however, this could change (metadata right now could be defined as GitCommit right now already, but it's global)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern was if Packer was using this same method and showing git commit between parenthesis it could be a breaking change for users checking git commits of builds, but Packer doesn't do that. Could it be a problem for packer plugin maintainers? I could see this happening if someone is using the plugin describe command for some validation or using the method to write user-agent to HTTP calls 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good callout, I didn't pay attention that I was changing the format honestly, I thought the commit was added as a metadata to the version if defined. For clarity though, it's not automagic, it needs either to be set by the plugin developers, or as a ldflag. AFAICT we don't set it in the scaffolding, or in any of our plugins.
Also, we use String for printing out the version of the plugin for describe. I'm not certain this is used anywhere, but I cannot certify it.

That said, I'll do a reroll of this commit to avoid changing the commit format, so it remains complete as it was beforehand.
I'll add a test to make sure it doesn't change between the two versions.

var versionString bytes.Buffer
fmt.Fprintf(&versionString, "%s", p.version)
if p.versionPrerelease != "" {
fmt.Fprintf(&versionString, "-%s", p.versionPrerelease)

if GitCommit != "" {
fmt.Fprintf(&versionString, " (%s)", GitCommit)
}
versionString := p.semVer.String()

// Given there could be some metadata already, we add the commit to the
// reported version as part of the metadata, with a `-` spearator if
// the metadata is already there, otherwise we make it the metadata
if GitCommit != "" {
versionString = fmt.Sprintf("%s (%s)", versionString, GitCommit)
}

return versionString.String()
return versionString
}

func (p *PluginVersion) SemVer() *version.Version {
Expand All @@ -91,10 +140,11 @@ func (p *PluginVersion) GetVersionPrerelease() string {
return p.versionPrerelease
}

func (p *PluginVersion) GetMetadata() string {
return p.versionMetadata
}

// String returns the complete version string, including prerelease
func (p *PluginVersion) String() string {
if p.versionPrerelease != "" {
return fmt.Sprintf("%s-%s", p.version, p.versionPrerelease)
}
return p.version
return p.semVer.String()
}
121 changes: 121 additions & 0 deletions version/version_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
package version

import (
"fmt"
"testing"
)

func Test_PluginVersionCreate(t *testing.T) {
tests := []struct {
name string
coreVersion string
preVersion string
metaVersion string
expectError bool
expectVersionString string
}{
{
"Valid semver core only version",
"1.0.0",
"",
"",
false,
"1.0.0",
},
{
"Valid semver, should get canonical version",
"01.001.001",
"",
"",
false,
"1.1.1",
},
{
"Valid semver with prerelease, should get canonical version",
"1.001.010",
"dev",
"",
false,
"1.1.10-dev",
},
{
"Valid semver with metadata, should get canonical version",
"1.001.010",
"",
"123abcdef",
false,
"1.1.10+123abcdef",
},
{
"Valid semver with prerelease and metadata, should get canonical version",
"1.001.010",
"dev",
"123abcdef",
false,
"1.1.10-dev+123abcdef",
},
{
"Invalid version, should fail",
".1.1",
"",
"",
true,
"",
},
{
"4-parts version, should not be accepted",
"1.1.1.1",
"",
"",
true,
"",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
defer func() {
panicMsg := recover()
if !tt.expectError && panicMsg != nil {
t.Errorf("creating version panicked, should not have.")
}

if tt.expectError && panicMsg == nil {
t.Errorf("creating version should have panicked, but did not.")
}

if panicMsg != nil {
t.Logf("panic message was: %v", panicMsg)
}
}()

ver := NewPluginVersion(tt.coreVersion, tt.preVersion, tt.metaVersion)
verStr := ver.String()
if verStr != tt.expectVersionString {
t.Errorf("string format mismatch, version created is %q, expected %q", verStr, tt.expectVersionString)
}
})
}
}

func TestFormattedVersionString(t *testing.T) {
lbajolet-hashicorp marked this conversation as resolved.
Show resolved Hide resolved
GitCommit = "abcdef12345"
defer func() {
GitCommit = ""
}()

expectedVersion := fmt.Sprintf("1.0.0-dev (%s)", GitCommit)

ver := InitializePluginVersion("1.0.0", "dev")
formatted := ver.FormattedVersion()
if formatted != expectedVersion {
t.Errorf("Expected formatted version %q; got %q", expectedVersion, formatted)
}

expectedVersion = fmt.Sprintf("1.0.0-dev+meta (%s)", GitCommit)
ver = NewPluginVersion("1.0.0", "dev", "meta")
formatted = ver.FormattedVersion()
if formatted != expectedVersion {
t.Errorf("Expected formatted version %q; got %q", expectedVersion, formatted)
}
}
Loading