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

Allow setting major version #15

Merged
merged 8 commits into from
Jan 23, 2025
Merged

Allow setting major version #15

merged 8 commits into from
Jan 23, 2025

Conversation

danielrbradley
Copy link
Member

Fixes #13

The inference of the major version can currently lead to some unexpected results. Add an option to pass in the expected major version to this action to ensure the major version is the desired value.

The only exception where the major version is ignored is when pushing tags. Tags are expected to be the exact version to publish, so we don't do any kind of inferrence there.

@danielrbradley danielrbradley requested a review from a team January 23, 2025 14:34
@danielrbradley danielrbradley self-assigned this Jan 23, 2025
Copy link

@thomas11 thomas11 left a comment

Choose a reason for hiding this comment

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

The code looks correct, but the explicit major version silently overriding the three implicit methods seems like a potential source of hard-to-debug issues. For instance, when a new vN+1 branch is started and the user forgets to increment major-version, won't it silently release vN? Would it be clearer for the user to decide on one approach?

@danielrbradley
Copy link
Member Author

The implicit part is still needed to generate reasonable version numbers i.e. if the last release was 2.3.0 then the branch build should be 2.4.0-[hash] and not 2.0.0-[hash].

For real releases this has no impact as tags are taken as-is and there's no implicit behaviour. This only affects automated pre-releases.

@thomas11
Copy link

The implicit part is still needed to generate reasonable version numbers i.e. if the last release was 2.3.0 then the branch build should be 2.4.0-[hash] and not 2.0.0-[hash].

My suggestion would be to run the implicit part to determine the correct version, then check if the major versions match (if major-version is specified), and fail if they don't.

@danielrbradley
Copy link
Member Author

danielrbradley commented Jan 23, 2025

I think we could print a warning, but an error would be very disruptive as the issue is that the inferred major version is often sometimes wrong, so we want to explicitly fix the major version in place and disable the major version inference entirely.

Copy link

@thomas11 thomas11 left a comment

Choose a reason for hiding this comment

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

LGTM pending some logging of how the version was determined, and if there was a discrepancy between explicit and implicit.

- Avoid showing logs during tests.
@danielrbradley danielrbradley merged commit f96d032 into main Jan 23, 2025
3 checks passed
@danielrbradley danielrbradley deleted the set-major-version branch January 23, 2025 17:00
@pulumi-bot
Copy link

This PR has been shipped in release v1.6.0.

danielrbradley added a commit to pulumi/ci-mgmt that referenced this pull request Feb 6, 2025
This ensures the major version always matches the major version generated into the makefile too - based on the .ci-mgmt.yaml config.

This is much more predictable behaviour than inferring from the branch name or PR labels. This utilises the new feature pulumi/provider-version-action#15, released in v1.6.0 of pulumi/provider-version-action
danielrbradley added a commit to pulumi/ci-mgmt that referenced this pull request Feb 6, 2025
This ensures the major version always matches the major version generated into the makefile too - based on the .ci-mgmt.yaml config.

This is much more predictable behaviour than inferring from the branch name or PR labels. This utilises the new feature pulumi/provider-version-action#15, released in v1.6.0 of pulumi/provider-version-action
danielrbradley added a commit to pulumi/ci-mgmt that referenced this pull request Feb 6, 2025
This ensures the major version always matches the major version generated into the makefile too - based on the .ci-mgmt.yaml config.

This is much more predictable behaviour than inferring from the branch name or PR labels. This utilises the new feature pulumi/provider-version-action#15, released in v1.6.0 of pulumi/provider-version-action
github-merge-queue bot pushed a commit to pulumi/ci-mgmt that referenced this pull request Feb 6, 2025
This ensures the major version always matches the major version
generated into the makefile too - based on the .ci-mgmt.yaml config.

This is much more predictable behaviour than inferring from the branch
name or PR labels. This utilises the new feature
pulumi/provider-version-action#15, released in
v1.6.0 of pulumi/provider-version-action

Stacked on #1363
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.

Consider accepting major version as input param
3 participants