-
Notifications
You must be signed in to change notification settings - Fork 1
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
Use semver sort to determine latest upstream release in getExpectedTargetLatest #220
Conversation
0995c94
to
76c1690
Compare
Thanks for fixing the issue here! I'm not sure this is quite the right logic though. It looks to me like this change yields the following logic: We would not upgrade to 1.1 as we should. Should we instead get the latest version by ordering the list from gh releases? |
I mean, this is all trying to make up for inconsistent human behavior around setting release standards. Do we think the However, I think the case you describe is unlikely:
Likely, what happened here is that a maintainer failed to check "Set as pre-release" when they published their beta version. I would be surprised to see a stable release not setting github The reason why I prefer checking things in this way is that there have been several issues in this tool with hand ordering tags, and asking for "what is latest on github" is pretty foolproof from an implementation perspective. I would argue it is reasonable to assume edited to add: |
Alright I spoke with @iwahbe about this. Because (github) However, we also cannot use the first release from We will do a best-attempt semver comparison of the default result of ...I do not like this. But it is the least lossy option we have. :( |
An upgrade issue with the correct upstream version was opened successfully using this code. |
…e output when the release is named anything other than a tag format.
Hm, the lack of json output is extremely unfortunate. Adjusting to split lines by tab instead to ensure we don't wind up with "Terraform" as our release tag when there is whitespace in release names. This should fix this failure.
|
upstreamOrg, GetContext(ctx).UpstreamProviderName) | ||
// Sort the versions. | ||
// Documentation here: https://pkg.go.dev/github.com/Masterminds/semver/v3#readme-sorting-semantic-versions | ||
sort.Sort(semver.Collection(versions)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
See #218 for details.
Uses the full json repo info payload to determine the latest upstream release.
See pulumi/pulumi-minio#251 for the fix applying to pulumi-minio specifically.
Fixes #218.
Fixes pulumi/pulumi-f5bigip#345
Fixes pulumi/pulumi-venafi#244
Reviewer Note: This functionality overrides the changes in #226. Note that we're returning an empty UpgradeTargetVersion in the case of a non-stable release, so that the nil check onPlan Provider Upgrade
reports that we are up to date. Alatest
version with a beta tag should not fail the upgrade process; we should ignore it.