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

Fixes rendering v1.2.3-alpha.8 style of versions to valid PyPI version #13707

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Aug 10, 2023

Description

Fixes rendering v1.2.3-alpha.8 style of versions to valid PyPI versions.

Fixes #13706

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@t0yv0 t0yv0 requested a review from a team August 10, 2023 17:07
@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2023-08-10)

Bug Fixes

  • [sdkgen/python] Fixes rendering v1.2.3-alpha.8 style of versions to valid PyPI versions when respectSchemaVersions option is set in sdkgen
    #13707

@@ -109,10 +109,21 @@ var pypiDev = regexp.MustCompile("^dev[0-9]+$")
// A valid post tag for pypi
var pypiPost = regexp.MustCompile("^post[0-9]+$")

// Transforms 0.0.1-alpha.18 to 0.0.1-alpha18; our users want to be able to use the previous form but semver.Version
// parses it into two Pre segments ["alpha", "18"] which trips up translation. The same treatment is given beta and rc
Copy link
Member

Choose a reason for hiding this comment

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

That's because it is two segments. Semver defines pre-release as a dot separated identifiers where each individual identifier is either alphanumeric or just numeric. Which means "0.0.1-18.alpha" is a valid semver, but after your squashing here making it "0.0.1-18alpha" is now invalid and will trigger a panic in MustParse.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. 0.0.1-18alpha does not match this regex. You can add a test. There's another bug in here not affected by my change that spits out suspicious version.

		{"0.0.1-18.alpha", "0.0.1+.alpha"},

Copy link
Member

Choose a reason for hiding this comment

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

Ah right ok, missed the regex only checked one way.
This conversion is fraught with issues in general 😆 but hey this covers one more case that's being used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, yes, I started comparing this conversion to the one used in pulumictl. It turns out that neither can pass the union of each other's test suites at the moment. Which his unfortunate. pulumictl is "more used in prod" as that currently decides versions for all pulumi packages including alphas and it does support the v1.2.3-alpha.8 style out of the box..

Copy link
Member

Choose a reason for hiding this comment

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

Yeh I dislike that pulumictl has that logic, it feels like it ought to be kept in just the language code generators but I get the oddity of publish workflows at the moment make that hard :(

@@ -109,10 +109,21 @@ var pypiDev = regexp.MustCompile("^dev[0-9]+$")
// A valid post tag for pypi
var pypiPost = regexp.MustCompile("^post[0-9]+$")

// Transforms 0.0.1-alpha.18 to 0.0.1-alpha18; our users want to be able to use the previous form but semver.Version
// parses it into two Pre segments ["alpha", "18"] which trips up translation. The same treatment is given beta and rc
Copy link
Member

Choose a reason for hiding this comment

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

Ah right ok, missed the regex only checked one way.
This conversion is fraught with issues in general 😆 but hey this covers one more case that's being used.

@t0yv0
Copy link
Member Author

t0yv0 commented Aug 10, 2023

/bors merge

@t0yv0
Copy link
Member Author

t0yv0 commented Aug 10, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 10, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 2849a54 into master Aug 10, 2023
50 checks passed
@bors bors bot deleted the t0yv0/fix-13706 branch August 10, 2023 22:48
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.

Invalid PyPI version emitted in codegen for 0.0.1-alpha.18
3 participants