Version parsing rehaul #228
Conversation
|
Looks good to me! |
|
Nice! 👏🏻 |
|
|
||
| if GitCommit != "" { | ||
| fmt.Fprintf(&versionString, " (%s)", GitCommit) | ||
| func (p *PluginVersion) FormattedVersion() string { |
There was a problem hiding this comment.
Could this be a breaking change?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
As it stands now, plugin developers can only specify a version's core semver-compatible version, and a prerelease. In usage though, some plugins use metadata to add some extra context on the binary that was built, like the git commit, or the delta between the last release and the current HEAD from which a plugin was built. This, coupled with the 1.11.0 changes to plugin loading and version support, means that we cannot support such a workflow with the current code, as we will now start enforcing proper semver for plugins, so we are at risk of having plugins either not load, or lose information when releasing. Therefore as an attempt to address those issues, we are adding official support in the SDK for metadata in versions. That change introduces two new functions: `NewPluginVersion` and `NewRawVersion`. Both are intended as replacements for the now deprecated `InitializePluginVersion`.
Since FormatedVersion was essentially the same thing as `String` with the extra GitCommit if defined, we change its implementation to rely on the code committed for String.
The go-version library we use for parsing versions from the plugin supports 4-segmented versions. This may not be ideal for us, as we want to limit the sprawling nature of plugin installations, which if we start accepting sub-patch version bumps, may become quite strange. The release workflows we offer as template does not take that into account, and I'm not sure our docs do as well. Since there are many unknowns here, 4-segmented version numbers are not semver-valid, and we do not know how tooling will react, we ultimately decide not to allow those in the SDK. If a developer tries to define a 4-segmented version number, the plugin will crash instantly, at least giving users a message quickly that the version number is invalid, and that they need to limit themselves to a 3-segment version number.
lbajolet-hashicorp
force-pushed
the
version_parsing_rehaul
branch
from
47b06d5 to
209160f
Compare
As it stands now, plugin developers can only specify a version's core semver-compatible version, and a prerelease.
In usage though, some plugins use metadata to add some extra context on the binary that was built, like the git commit, or the delta between the last release and the current HEAD from which a plugin was built.
This, coupled with the 1.11.0 changes to plugin loading and version support, means that we cannot support such a workflow with the current code, as we will now start enforcing proper semver for plugins, so we are at risk of having plugins either not load, or lose information when releasing.
Therefore as an attempt to address those issues, we are adding official support in the SDK for metadata in versions. That change introduces two new functions:
NewPluginVersionandNewRawVersion. Both are intended as replacements for the now deprecatedInitializePluginVersion.In addition to this change, we are also limiting the number of segments to 3 in the version number, there are too many unknowns around 4-segmented version numbers, in addition to this not being semver compliant (despite the library supporting them).
cc @NorseGaud @torarnv @chris-rock since we've had recent exchanges regarding plugins recently on your respective repositories, this PR should cement the addition of metadata to what is supported for versions. Please feel free to take a look at this code, and let me know your thoughts, I'd like to know if this kind of interface would suit your development practices.
Thanks in advance!