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.

Sign up for GitHub

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

Jump to bottom

fix(maven-workspace): SNAPSHOT versions should not be included in the manifest #1727

Merged
merged 2 commits into from
Oct 28, 2022
Merged

fix(maven-workspace): SNAPSHOT versions should not be included in the manifest #1727

merged 2 commits into from
Oct 28, 2022

Conversation

chingor13
Copy link
Contributor

@chingor13 chingor13 commented Oct 27, 2022 •
edited
Loading

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Oct 27, 2022
@chingor13 chingor13 changed the title test: failing test for updating non-release versions in manifest fix(maven-workspace): SNAPSHOT versions should not be included in the manifest Oct 27, 2022
@chingor13 chingor13 marked this pull request as ready for review October 27, 2022 23:55
@chingor13 chingor13 requested a review from a team as a code owner October 27, 2022 23:55
@chingor13 chingor13 requested a review from a team October 27, 2022 23:55
@chingor13 chingor13 marked this pull request as draft October 27, 2022 23:55
@chingor13 chingor13 marked this pull request as ready for review October 28, 2022 16:41
@chingor13 chingor13 requested a review from bcoe October 28, 2022 16:41
bcoe
bcoe approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

LGTM, if I'm understanding correctly, prior to this change this file would have been updated to include -SNAPSHOT versions, prior to this fix.

src/plugins/maven-workspace.ts
* @param {Version} version The release version
*/
protected isReleaseVersion(version: Version): boolean {
return !version.preRelease?.includes('SNAPSHOT');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be worth pulling SNAPSHOT into a constant, if we use this value in a variety of places.

@chingor13
Copy link
Contributor Author

cc @suztomo

@chingor13
Copy link
Contributor Author

if I'm understanding correctly, prior to this change this file would have been updated to include -SNAPSHOT versions, prior to this fix

Correct, the core manifest already knows not to update non-release versions into the manifest, but the workspace plugin did not.

@chingor13 chingor13 merged commit d926c84 into main Oct 28, 2022
@chingor13 chingor13 deleted the no-manifest-snapshot branch October 28, 2022 17:17
@release-please release-please bot mentioned this pull request Oct 28, 2022
Merged
@chingor13 chingor13 mentioned this pull request Oct 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoe bcoe bcoe approved these changes

Assignees

@bcoe bcoe

Labels
size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Release notes duplication in new PR design.
2 participants
@chingor13 @bcoe