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

feat: add PrereleaseVersioningStrategy #1981

Merged
merged 8 commits into from
Jul 11, 2023
Merged

Conversation

chingor13
Copy link
Contributor

This adds a new versioning strategy that can handle prerelease versioning used by google-cloud-dotnet.

For a fix type of release, it will bump the prerelease number and preserve any padded 0's.
Example:

  • 1.2.3-beta01 => 1.2.3-beta02
  • 1.2.3-beta09 => 1.2.3-beta10.

@chingor13 chingor13 requested a review from jskeet June 12, 2023 21:23
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Jun 12, 2023
@chingor13 chingor13 marked this pull request as ready for review June 12, 2023 21:30
@chingor13 chingor13 requested review from a team as code owners June 12, 2023 21:30
@chingor13
Copy link
Contributor Author

@jskeet This should be the last blocker for the google-cloud-dotnet migration. What's unclear to me is what you want the feat and/or breaking change behavior to be for a pre-release version. E.g. 1.2.3-beta01 + feat = ?? (1.3.0-beta00 or 1.2.3-beta02)

@jskeet
Copy link

jskeet commented Jun 13, 2023

"1.2.3-beta01 + feat" should be 1.3.0-beta01, but "1.2.0-beta01 + feat" should be "1.2.0-beta02". My rationale is that 1.2.3-beta01 should still be logically a patch on the 1.2.x series, and adding a feature means that it should be promoted to 1.3.x... whereas 1.2.0-beta01 suggests "we haven't released 1.2.0 yet, so we can keep adding features to it". Hope that makes sense! Will now review the code...

Copy link

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

I suspect the production code is fine - I've suggested a bunch more tests though :)

src/factories/versioning-strategy-factory.ts Show resolved Hide resolved
const strategy = new PrereleaseVersioningStrategy();
const oldVersion = Version.parse('0.1.2');
const newVersion = await strategy.bump(oldVersion, commits);
expect(newVersion.toString()).to.equal('1.0.0');
Copy link

Choose a reason for hiding this comment

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

We don't actually use "less than 0" majors, but I'd expect this to be 0.2.0 personally. (Basically "before 1.0, there are no guarantees.") I've now read that this is the "bumpMinorPreMajor" behavior, but I'd personally expect it to be the default. It feels very weird for "I've taken a breaking change" to be the trigger to go from 0.x to 1.0.

breaking: false,
},
];
it('can bump a major', async () => {
Copy link

Choose a reason for hiding this comment

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

Can you add a test for a breaking change on a various pre-release aspects? For example:

  • 1.0.0-beta01 => 1.0.0-beta02
  • 2.0.0-beta01 => 2.0.0-beta02
  • 1.0.1-beta01 => 2.0.0-beta02
  • 1.1.0-beta01 => 2.0.0-beta01
  • 1.1.1-beta01 => 2.0.0-beta01

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 1.0.1-beta01 => 2.0.0-beta02

This one should probably be 1.0.1-beta01 => 2.0.0-beta01?

Copy link

Choose a reason for hiding this comment

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

Yup, sorry about that. Copy and paste error, I suspect.

Of course, there's actually more subtlety here - if the breaking change is only breaking with respect to a new feature introduce in a beta, then it can just be beta02. But we don't really have that much information, as far as I'm aware. This is why I'm expecting to keep looking at changes fairly carefully, manually, in order to check that release-please is doing the right thing :)

breaking: false,
},
];
it('can bump a minor', async () => {
Copy link

Choose a reason for hiding this comment

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

Again, I'd like to see the impact of features on pre-releases:

  • 1.0.0-beta01 => 1.0.0-beta02
  • 2.0.0-beta01 => 2.0.0-beta02
  • 1.0.1-beta01 => 1.1.0-beta01
  • 1.1.0-beta01 => 1.1.0-beta02
  • 1.1.1-beta01 => 1.2.0-beta01

});
});

describe('with a fix', () => {
Copy link

Choose a reason for hiding this comment

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

In this case:

  • 1.0.0-beta01 => 1.0.0-beta02
  • 2.0.0-beta01 => 2.0.0-beta02
  • 1.0.1-beta01 => 1.0.1-beta02
  • 1.1.0-beta01 => 1.1.0-beta02
  • 1.1.1-beta01 => 1.1.1-beta01

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 1.1.1-beta01 => 1.1.1-beta01

This one should be 1.1.1-beta01 => 1.1.1-beta02?

Copy link

Choose a reason for hiding this comment

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

Yes, sorry.

import {PrereleaseVersioningStrategy} from '../../src/versioning-strategies/prerelease';
import {Version} from '../../src/version';

describe('PrereleaseVersioningStrategyVersioningStrategy', () => {
Copy link

Choose a reason for hiding this comment

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

Also add test for:

  • 1.0.0-beta1 => 1.0.0-beta2
  • 1.0.0-beta01 => 1.0.0-beta02
  • 1.0.0-beta9 => 1.0.0-beta10 (although that would be unfortunate)
  • 1.0.0-beta01 => 1.0.0-beta10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 1.0.0-beta01 => 1.0.0-beta10

I think this one was intended to be 1.0.0-beta09 => 1.0.0-beta10

Copy link

Choose a reason for hiding this comment

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

Yup, absolutely. I was clearly not at my best when commenting. Sorry :(

Copy link

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@chingor13 chingor13 enabled auto-merge (squash) July 11, 2023 22:48
@chingor13 chingor13 merged commit b070888 into main Jul 11, 2023
@chingor13 chingor13 deleted the dotnet-yoshi-versioning branch July 11, 2023 22:50
@bhvngt
Copy link

bhvngt commented Jul 20, 2023

Thanks for this PR. When do you plan to change the github action to point to 15.12.0. Currently it points to ^15.11.0?

Because of above, I am not able to try out prerelase strategy with github action. Though it works fine with cli

@chingor13
Copy link
Contributor Author

When do you plan to change the github action to point to 15.12.0

Sorry, I just released a new action version which should contain this update.

Copy link

@mahmod87 mahmod87 left a comment

Choose a reason for hiding this comment

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

.#

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants