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

fix: support tags with build info #65

Merged
merged 2 commits into from
May 11, 2023

Conversation

rawkode
Copy link
Contributor

@rawkode rawkode commented May 10, 2023

When briding a Terraform provider, it makes
a lot of sense to match the Terraform provider
version in the Pulumi provider.

However, there needs to be a clean way to increment this build while keeping the Terraform version static, without abusing prerelease tags.

This is supported by build metadata:
https://semver.org/spec/v2.0.0.html

When briding a Terraform provider, it makes
a lot of sense to match the Terraform provider
version in the Pulumi provider.

However, there needs to be a clean way to increment
this build while keeping the Terraform version static,
without abusing prerelease tags.

This is supported by build metadata:
https://semver.org/spec/v2.0.0.html
@rawkode
Copy link
Contributor Author

rawkode commented May 10, 2023

@jaxxstorm Any chance you could take a look at this, please?

@iwahbe iwahbe requested review from iwahbe and guineveresaenger and removed request for iwahbe May 11, 2023 16:21
Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

I'd like to see additional tests for longer formats to ensure we're appending everything correctly every time.

To clarify the purpose of adding this enhancement: we want to be able to read and support build tags set on patch versions?

Overall, this is a reasonable enhancement. 👍

pkg/gitversion/gitversion_test.go Show resolved Hide resolved
@@ -130,12 +132,23 @@ func GetLanguageVersionsWithOptions(opts LanguageVersionsOptions) (*LanguageVers
preVersion = fmt.Sprintf("%s%sdirty", preVersion, separator)
}

buildVersion := []byte{}
if len(genericVersion.Build) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the genericVersion is a prerelease? I see the tests pass, but what is keeping this from adding two build tags to a prerelease? we do add a build tag extension to the preVersion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guineveresaenger This is just weird behaviour in this code and I dare not add a test for that or touch it, because pulumictl shouldn't be adding the commit hash to prerelease tags.

@rawkode
Copy link
Contributor Author

rawkode commented May 11, 2023

@guineveresaenger New test case added

require.NoError(t, err)

tagSequence := []string{
"v1.0.0+1abc.345.whoop",
Copy link
Contributor

Choose a reason for hiding this comment

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

😁

Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

Thank you 🙇‍♀️ I'll get this merged once tests pass, and release a new version.

@guineveresaenger guineveresaenger merged commit a2c5d77 into pulumi:master May 11, 2023
@rawkode rawkode deleted the fix/support-build-version branch May 12, 2023 08:32
@rawkode
Copy link
Contributor Author

rawkode commented May 12, 2023

@guineveresaenger Can we sneak in a release? 😄

@rawkode
Copy link
Contributor Author

rawkode commented May 14, 2023

Thanks @guineveresaenger Just seen you cut one. Not sure why it's not shown on the main repo page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants