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

Remove maven release plugin requirement #1295

Merged

Conversation

rbellamy
Copy link
Contributor

@rbellamy rbellamy commented Jun 14, 2020

Fixes #1260

Release Notes

This release removes the requirement for the Maven Release Plugin from maven projects. This is a breaking change but that maven plugin was quite experimental. This PR makes it a full featured auto experience.

Remove requirement for "maven-release-plugin" and other improvements

  1. Remove requirement for "maven-release-plugin".
  2. Support recursive changes to all pom.xml files in the project, with the following assumptions:
    a. The project is a multi-module project.
    b. The parent pom.xml file is located in the root directory of the repo.
    c. The parent pom.xml contains the version.
    d. Sub-modules have the same version as the parent pom.xml.
  3. Support plugin options, with environment variable overrides:
    a. MAVEN_COMMAND || mavenCommand - the path to the maven executable to use. Defaults to /usr/bin/mvn.
    b. MAVEN_OPTIONS || mavenOptions - an array of arbitrary maven options, e.g. -DskipTests -P some-profile. No defaults.
    c. MAVEN_RELEASE_GOALS || mavenReleaseGoals - an array of maven goals to run when publishing. Defaults to ["deploy", "site-deploy"].
    d. MAVEN_SETTINGS || mavenSettings - the path to the maven settings file. No defaults.

NOTE: The MAVEN_USERNAME and MAVEN_PASSWORD environment variables are still supported, and have their counterparts as configuration options, but should are deprecated, and will be removed in a later release. This is because MAVEN_SETTINGS or MAVEN_OPTIONS can do the same work, but provide a much more flexible solution.

Support both "versions-maven-plugin" and auto-native DOM/XML

auto will detect if the parent pom.xml file has the versions-maven-plugin configured, and if so, use it to set the version on the parent and all child pom.xml files. If not, then auto will modify the parent and all child pom.xml files using a DOM parser and XML serializer. This has the effect of losing formatting. Therefore it then runs the serialized XML through the prettier "html" pretty-printer.

This means that if the versions-maven-plugin isn't available, the pom.xml files will be pretty-printed using the prettier formatter with the following default settings:

  • printWidth: 120 (configurable - see below)
  • tabWidth: 4 (configurable - see below)
  • parser: "html"

Version

📦 Published PR as canary version: under canary scope @auto-canary@9.40.0-canary.1295.16618.0

✨ Test out this PR locally via:

npm install @auto-canary/bot-list@9.40.0-canary.1295.16618.0
npm install @auto-canary/auto@9.40.0-canary.1295.16618.0
npm install @auto-canary/core@9.40.0-canary.1295.16618.0
npm install @auto-canary/all-contributors@9.40.0-canary.1295.16618.0
npm install @auto-canary/brew@9.40.0-canary.1295.16618.0
npm install @auto-canary/chrome@9.40.0-canary.1295.16618.0
npm install @auto-canary/cocoapods@9.40.0-canary.1295.16618.0
npm install @auto-canary/conventional-commits@9.40.0-canary.1295.16618.0
npm install @auto-canary/crates@9.40.0-canary.1295.16618.0
npm install @auto-canary/exec@9.40.0-canary.1295.16618.0
npm install @auto-canary/first-time-contributor@9.40.0-canary.1295.16618.0
npm install @auto-canary/gem@9.40.0-canary.1295.16618.0
npm install @auto-canary/gh-pages@9.40.0-canary.1295.16618.0
npm install @auto-canary/git-tag@9.40.0-canary.1295.16618.0
npm install @auto-canary/gradle@9.40.0-canary.1295.16618.0
npm install @auto-canary/jira@9.40.0-canary.1295.16618.0
npm install @auto-canary/maven@9.40.0-canary.1295.16618.0
npm install @auto-canary/npm@9.40.0-canary.1295.16618.0
npm install @auto-canary/omit-commits@9.40.0-canary.1295.16618.0
npm install @auto-canary/omit-release-notes@9.40.0-canary.1295.16618.0
npm install @auto-canary/released@9.40.0-canary.1295.16618.0
npm install @auto-canary/s3@9.40.0-canary.1295.16618.0
npm install @auto-canary/slack@9.40.0-canary.1295.16618.0
npm install @auto-canary/twitter@9.40.0-canary.1295.16618.0
npm install @auto-canary/upload-assets@9.40.0-canary.1295.16618.0
# or 
yarn add @auto-canary/bot-list@9.40.0-canary.1295.16618.0
yarn add @auto-canary/auto@9.40.0-canary.1295.16618.0
yarn add @auto-canary/core@9.40.0-canary.1295.16618.0
yarn add @auto-canary/all-contributors@9.40.0-canary.1295.16618.0
yarn add @auto-canary/brew@9.40.0-canary.1295.16618.0
yarn add @auto-canary/chrome@9.40.0-canary.1295.16618.0
yarn add @auto-canary/cocoapods@9.40.0-canary.1295.16618.0
yarn add @auto-canary/conventional-commits@9.40.0-canary.1295.16618.0
yarn add @auto-canary/crates@9.40.0-canary.1295.16618.0
yarn add @auto-canary/exec@9.40.0-canary.1295.16618.0
yarn add @auto-canary/first-time-contributor@9.40.0-canary.1295.16618.0
yarn add @auto-canary/gem@9.40.0-canary.1295.16618.0
yarn add @auto-canary/gh-pages@9.40.0-canary.1295.16618.0
yarn add @auto-canary/git-tag@9.40.0-canary.1295.16618.0
yarn add @auto-canary/gradle@9.40.0-canary.1295.16618.0
yarn add @auto-canary/jira@9.40.0-canary.1295.16618.0
yarn add @auto-canary/maven@9.40.0-canary.1295.16618.0
yarn add @auto-canary/npm@9.40.0-canary.1295.16618.0
yarn add @auto-canary/omit-commits@9.40.0-canary.1295.16618.0
yarn add @auto-canary/omit-release-notes@9.40.0-canary.1295.16618.0
yarn add @auto-canary/released@9.40.0-canary.1295.16618.0
yarn add @auto-canary/s3@9.40.0-canary.1295.16618.0
yarn add @auto-canary/slack@9.40.0-canary.1295.16618.0
yarn add @auto-canary/twitter@9.40.0-canary.1295.16618.0
yarn add @auto-canary/upload-assets@9.40.0-canary.1295.16618.0

Copy link
Collaborator

@hipstersmoothie hipstersmoothie left a comment

Choose a reason for hiding this comment

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

This looks great! just a few minor things here and ther

@rbellamy
Copy link
Contributor Author

Gah... I rebased from master and that pulled in a BUNCH of stuff that doesn't belong in this PR. I think I should close this PR and re-submit.

@rbellamy
Copy link
Contributor Author

I'm going to do another rebase and force push the branch to see if that will get the history in proper alignment.

@rbellamy rbellamy force-pushed the remove-maven-release-plugin-requirement branch from a24e5a4 to 7f56e4d Compare June 17, 2020 18:53
@rbellamy
Copy link
Contributor Author

rbellamy commented Jun 17, 2020

That's much better.

However, now we're seeing error Your lockfile needs to be updated, but yarn was run with '--frozen-lockfile'. during the CI build "install dependencies" step. @hipstersmoothie how do you want me to address that?

@rbellamy
Copy link
Contributor Author

rbellamy commented Jun 17, 2020

I did what I thought was the right thing with the yarn.lock file, but it's still showing as conflicting. Not sure how to proceed?

Copy link
Contributor Author

@rbellamy rbellamy left a comment

Choose a reason for hiding this comment

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

Reviewed and implemented each requested change.

@rbellamy
Copy link
Contributor Author

Still getting yarn.lock error in CI.

G. Richard Bellamy and others added 13 commits June 18, 2020 12:04
Remove requirement for "maven-release-plugin" and other improvements
1. Remove requirement for "maven-release-plugin".
2. Support recursive changes to all "pom.xml" files in the project, with the following assumptions:
  a. The project is a multi-module project.
  b. The parent pom.xml file is located in the root directory of the repo.
  c. The parent pom.xml contains the version.
  d. Sub-modules have the same version as the parent pom.xml.
3. Support plugin options, with environment variable overrides:
  a. `MAVEN_COMMAND || mavenCommand` - the path to the maven executable to use. Defaults to `/usr/bin/mvn`.
  b. `MAVEN_OPTIONS || mavenOptions` - an array of arbitrary maven options, e.g. `-DskipTests -P some-profile`. No defaults.
  c. `MAVEN_RELEASE_GOALS || mavenReleaseGoals` - an array of maven goals to run when publishing. Defaults to `["deploy", "site-deploy"]`.
  d. `MAVEN_SETTINGS || mavenSettings` - the path to the maven settings file. No defaults.
`auto` will detect if the parent `pom.xml` file has the `versions-maven-plugin` configured, and if so, use it to set the version on the parent and all child pom.xml files. If not, then auto will modify the parent and all child `pom.xml` files using a DOM parser and XML serializer. This has the effect of losing formatting. Therefore it then runs the serialized XML through the `prettier` "html" pretty-printer.

This means that if the versions-maven-plugin isn't available, the pom.xml files will be pretty-printed using prettier formatter with the following default settings:

* `printWidth: 120` (configurable - see below)
* `tabWidth: 4` (configurable - see below)
* `parser: "html"`
I had commented some git commit/push commands so I could test the plugin on some actual maven projects without the possibility of those projects having commits pushed to their repositories. I forgot to uncomment those commands.
Co-authored-by: Andrew Lisowski <lisowski54@gmail.com>
`auto` will detect if the parent `pom.xml` file has the `versions-maven-plugin` configured, and if so, use it to set the version on the parent and all child pom.xml files. If not, then auto will modify the parent and all child `pom.xml` files using a DOM parser and XML serializer. This has the effect of losing formatting. Therefore it then runs the serialized XML through the `prettier` "html" pretty-printer.

This means that if the versions-maven-plugin isn't available, the pom.xml files will be pretty-printed using prettier formatter with the following default settings:

* `printWidth: 120` (configurable - see below)
* `tabWidth: 4` (configurable - see below)
* `parser: "html"`
@hipstersmoothie hipstersmoothie force-pushed the remove-maven-release-plugin-requirement branch from b5d4b45 to dca43c0 Compare June 18, 2020 19:07
@hipstersmoothie
Copy link
Collaborator

@rbellamy odd issue. I tried just running yarn to get the file to update but that didn't work. Seems to work after a rebase though!

@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #1295 into master will decrease coverage by 0.15%.
The diff coverage is 78.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1295      +/-   ##
==========================================
- Coverage   81.18%   81.02%   -0.16%     
==========================================
  Files          56       56              
  Lines        4097     4158      +61     
  Branches      888      864      -24     
==========================================
+ Hits         3326     3369      +43     
- Misses        548      554       +6     
- Partials      223      235      +12     
Impacted Files Coverage Δ
packages/core/src/utils/make-hooks.ts 100.00% <ø> (ø)
plugins/maven/src/index.ts 79.02% <78.03%> (-6.35%) ⬇️
packages/core/src/auto.ts 77.46% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2613d77...799509c. Read the comment docs.

@rbellamy
Copy link
Contributor Author

Yeah, very odd issue - I'm glad the rebase worked for you!

@rbellamy
Copy link
Contributor Author

Looks like testing coverage needs some work.

@rbellamy
Copy link
Contributor Author

@hipstersmoothie was there something else you needed me to do before this gets merged and closed?

@hipstersmoothie
Copy link
Collaborator

The only thing holding me up from merging is that this a breaking change right? I had planned some other things for v10. I don't care too much about those though so I can merge. What label do you think should be attached to this PR?

@rbellamy
Copy link
Contributor Author

It is a breaking change and a significant one.

A likely scenario is that someone has been using this plugin, which previously required the maven-release-plugin, and they run the tool without reading the docs and/or migrating to the versions-maven-plugin. They will get a "surprise reformat" of all their pom.xml files.

So yeah, probably the labels would be "major" and "enhancement".

@rbellamy
Copy link
Contributor Author

I guess the question at hand is - should a plugin's breaking change force a major version upgrade of the entire tool? That's a policy question that only you can answer.

@hipstersmoothie hipstersmoothie added the minor Increment the minor version when merged label Jun 24, 2020
@hipstersmoothie hipstersmoothie merged commit d2632f9 into intuit:master Jun 24, 2020
@adierkens
Copy link
Collaborator

🚀 PR was released in v9.40.0 🚀

@adierkens adierkens added the released This issue/pull request has been released. label Jun 24, 2020
@rbellamy rbellamy deleted the remove-maven-release-plugin-requirement branch April 1, 2021 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove requirement for maven-release-plugin
3 participants