Remove requirement for maven-release-plugin

[rbellamy]

I've begun the work of refactoring the auto maven plugin to try to be more consistent with the rest of auto. In doing so, I've come to some conclusions.

The current requirement for auto with the maven plugin is that the project use the maven-release-plugin. This has several consequences which make it a poor fit for auto.

The primary functionality of the maven-release-plugin executes the following steps via release:prepare:

1. Check that there are no uncommitted changes in the sources
2. Check that there are no SNAPSHOT dependencies
3. Change the version in the POMs from x-SNAPSHOT to a new version
4. Transform the SCM information in the POM to include the final destination of the tag
5. Run the project tests against the modified POMs to confirm everything is in working order
6. Commit the modified POMs
7. Tag the code in the SCM with a version name
8. Bump the version in the POMs to a new value y-SNAPSHOT
9. Commit the modified POMs

And then a release is done via release:perform:

1. Checkout from an SCM URL with optional tag
2. Run the predefined Maven goals to release the project (by default, deploy site-deploy)

In order to ensure the plugin behaves in a manner consistent with auto, the current implementation uses some git hackery to dispose of the commits made by the release:prepare goal. In other words, the maven-release-plugin is doing the work that auto is designed for, and therefore has to be "worked around" in a less-than-graceful manner.

Of equal importance is the method used to extract the repository and author information - currently the maven plugin uses the <scm/> and <developers/> sections of the pom.xml to derive that information. This deviates from the methodology used by the rest of auto which uses git metadata. The current implementation has several problems:

1. The author is set via the <developers/> section of the pom.xml, which means that the person doing the commit MAY NOT be the same as the selected author. There is no attempt to line up the git commit author with the <developers/> information.
2. The owner and repo information is set via the <scm/> section, which may be different than the actual repository and owner of the current clone.

On doing a deeper dive of the plugin, and in comparing it with the gradle plugin, it appears that the only goal from the maven-release-plugin that would be of benefit is the release:update-versions goal. That goal could easily be replaced with some simple XML handling.

Given the overhead of maintaining the maven-release-plugin and the minimal advantage, my take is that the requirement should be lifted, and the auto maven plugin should be implemented in a more auto-native fashion.

[hipstersmoothie]

I completely agree!

Most of your points with info I get from the pom.xml is probably tied to my misunderstanding of those fields. Any changes here are more than welcome.

I'm all for any breaking changes we have to do. We can probably roll some of the smaller breaking changes detailed in #1156

[rbellamy]

It looks like my two points about <scm/> and <developers/> are incorrect. The npm plugin pulls those from the package.json. Does this mean that they should be located in the pom.xml?

[hipstersmoothie]

If those are the pom equivalents of the npm repository and author fields then yes.

The author is set via the section of the pom.xml, which means that the person doing the commit MAY NOT be the same as the selected author. There is no attempt to line up the git commit author with the information.

This is true for the npm plugin as well. Locally auto will not override your git configuration, but in a CI env it might have to set the author to commit`.

The owner and repo information is set via the section, which may be different than the actual repository and owner of the current clone.

Would this be different? In npm world the repository field usually points to the code for the package. In practice one usually doesn't run auto on a fork and if they were to I would hope they update the repository field to their fork

[rbellamy]

Okay, that makes sense - so the proper behavior is to look for those things (current), and if they're NOT found in the pom.xml, return and assume they've been set in the .autorc (new behavior).

Currently, the plugin will error if it doesn't find the <scm/> and <developers/> sections in the pom.xml.

[hipstersmoothie]

Ah yeah that's definitely a bug. It should not throw errors https://github.com/intuit/auto/blob/master/plugins/npm/src/index.ts#L504

[rbellamy]

Is there a way to get the version NUMBER from auto in a machine-readable fashion? I was surprised that the auto version command produces the TYPE of version bump, rather than the number.

[hipstersmoothie]

You can use --quiet and it will only print the version produced by the command. or pair it with --dry-run and you can see what version will be published next.

I would might be willing to add a flag to make version spit out the actual version. This just gets weird in multi package scenarios

[rbellamy]

I can see that. It's probably better to leave that to the caller to figure out which version it's looking for.

[rbellamy]

Your point about multi-package scenarios is pertinent here - the maven-release-plugin supports that type of scenario, out-of-the-box. This makes me question whether I would want to reproduce that work. I'm finding myself at a bit of an impasse - on the one hand, my argument for NOT using the maven-release-plugin is valid, while on the other, things like submodule version management is a non-trivial use-case that should be addressed, and is covered by the maven-release-plugin.

I've found two methods of using the maven-release-plugin that could be used to prevent the maven plugin from modifying the repository during release:prepare, and will be testing them later today.

[rbellamy]

@hipstersmoothie I have a question about the getAuthor, getRepo, etc. hooks behavior - RE: the comment about #1260 (comment), I'm looking at the tests and they seem to contradict that.

Example:

auto/plugins/maven/__tests__/maven.test.ts

Line 95 in 47d3b54

await expect(hooks.getAuthor.promise()).rejects.toBeInstanceOf(Error);

So, when I return an undefined AuthorInformation no error is thrown.

Any suggestions about what I may be doing wrong, or whether I should re-introduce throw to my hook handling?

[hipstersmoothie]

That's definitely wrong given how all the other plugins work. Returning undefined is the right way. That way core can fall back to the local configured git user if necassary

[rbellamy]

Gotcha - that's what I figured, so I've modified the tests to ensure failed hooks return undefined.

[rbellamy]

Looks like both the gradle and the cocoapods plugins throw errors if they can't find a version. I created a bug for the gradle plugin - do you want another for cocoapods?

[hipstersmoothie]

Just leave a not on the issue. Thanks for the catch!

[rbellamy]

I'm struggling with a good method for testing the initialization of values during hooks.beforeRun. Specifically, I have the following code, that when under debug successfully extracts the author information during the hooks.beforeRun tap, but fails the test:

    test("should get author from pom.xml", async () => {
      mockRead(`
        <project
          xmlns="http://maven.apache.org/POM/4.0.0"
          xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
          xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
          <developers>
            <developer>
              <name>Andrew Lisowski</name>
              <email>test@email.com</email>
            </developer>
          </developers>
        </project>
      `);

      await hooks.beforeRun.promise({} as any);

      expect(await hooks.getAuthor.promise()).toStrictEqual(
        expect.objectContaining({
          email: "test@email.com",
          name: "Andrew Lisowski",
        })
      );
    });

What's happening is that the test is getting to the getAuthor hook BEFORE the beforeRun hook is run. I've looked at other code that tests this (gradle, s3), and am not having any luck.

Basically, I need a way to deterministically execute the beforeRun hook and can't figure out how to do it.

[hipstersmoothie]

push a branch and I can check it out

[rbellamy]

@hipstersmoothie here's the branch: https://github.com/terradatum/auto/tree/remove-maven-release-plugin-requirement

I've commented out the code which would push my changes to the central repo so that I can test the code on a real project without polluting the remote history.

Any help you can give me with this is greatly appreciated - I would love to be able to use the tests to check how well I can modify the pom.xml files, rather than resorting to the old-school method of "run the code against real files, check the real files, run git reset --hard HEAD~1 to get rid of the changes, rinse and repeat."

[rbellamy]

Also, I had to make some changes to the local package.json that were incompatible with the root package.json in order to run the jest tests in IntelliJ - is there any guidance you can give me on that, or should it "just work" out of the box?

Specifically, when in the maven plugin directory and running npm test - ALL the tests end up running because "test": "jest --maxWorkers=2 --config ../../package.json".

And if I try to use the IntelliJ Test Runner from the editor, I get:

  ● Test suite failed to run

    SyntaxError: /home/rbellamy/Development/Terradatum/auto/plugins/maven/__tests__/maven.test.ts: Unexpected token, expected "," (15:24)

      13 | }));
      14 | 
    > 15 | const mockRead = (result: string) =>
         |                         ^
      16 |   jest
      17 |     .spyOn(fs, "readFile")
      18 |     // @ts-ignore

      at Parser._raise (../../node_modules/@babel/parser/src/parser/location.js:241:45)
      at Parser._raise [as raiseWithData] (../../node_modules/@babel/parser/src/parser/location.js:236:17)
      at Parser.raiseWithData [as raise] (../../node_modules/@babel/parser/src/parser/location.js:220:17)
      at Parser.raise [as unexpected] (../../node_modules/@babel/parser/src/parser/util.js:149:16)
      at Parser.unexpected [as expect] (../../node_modules/@babel/parser/src/parser/util.js:129:28)
      at Parser.expect [as parseParenAndDistinguishExpression] (../../node_modules/@babel/parser/src/parser/expression.js:1293:14)
      at Parser.parseParenAndDistinguishExpression [as parseExprAtom] (../../node_modules/@babel/parser/src/parser/expression.js:1029:21)
      at Parser.parseExprAtom [as parseExprSubscripts] (../../node_modules/@babel/parser/src/parser/expression.js:539:23)
      at Parser.parseExprSubscripts [as parseMaybeUnary] (../../node_modules/@babel/parser/src/parser/expression.js:519:21)
      at Parser.parseMaybeUnary [as parseExprOps] (../../node_modules/@babel/parser/src/parser/expression.js:311:23)

[rbellamy]

I can solve my problem with IntelliJ by adding a jest.config.ts file:

module.exports = {
  clearMocks: true,
  moduleFileExtensions: ['js', 'ts'],
  testEnvironment: 'node',
  testMatch: ['**/*.test.ts'],
  testRunner: 'jest-circus/runner',
  transform: {
    '^.+\\.ts$': 'ts-jest'
  },
  verbose: true
}

And then adding jest-circus to either the root or maven package.json files.

[rbellamy]

Just noticed that xml2js isn't going to do the job because it will silently REMOVE all XML comments during the XML => JSON => XML translations. I've been trying to make it work, and it's like pulling teeth. There is no guarantee that the XML will look anything like the original when moving through the xml2js transforms, and that's a deal-breaker.

[hipstersmoothie]

To run tests for just one plugin I run it from the root.

yarn test plugins/maven

[hipstersmoothie]

This then begs the question of whether or not you can easily show all the changes you would be making or the artifacts you would be building without actually making some changes.

That does seem like a pretty big deal breaker. Is there any standard CLI shipped in java environments that allows for editing of that file?

For plugins that don't have a parser for the config file (ex: ruby) auto uses mostly regex to update the file.

[rbellamy]

Did you get a chance to look at the beforeRun issue? I have a workaround, but it feels really hacky...

[rbellamy]

I really dislike the idea of using regex with XML - it's error-prone and brittle. However, it also seems ridiculously complex trying to use a DOMParser or XMLSerializer just to modify the version, which is why I was trying to work with xml2js in the first place.

[hipstersmoothie]

Looking at this now!

[hipstersmoothie]

https://github.com/terradatum/auto/blob/remove-maven-release-plugin-requirement/plugins/maven/src/index.ts#L127

The problem is your are doing a "synchronous" tap and doing async work inside of it

Change this

auto.hooks.beforeRun.tap(this.name, async () => {

to this

auto.hooks.beforeRun.tapPromise(this.name, async () => {

[hipstersmoothie]

Ah the before run hook is sync right now, that's the real problem. It wouldn't change anything if that became an AsyncSeriesHook instead of a SyncHook. I can push a commit to you branch if it's unclear what I'm saying

[rbellamy]

Please do... let me make you a contributor.

[rbellamy]

Go ahead!

[hipstersmoothie]

Just pushed. Ran the tests and only two are failing now. the beforeRun hook is working as expected.

[rbellamy]

@hipstersmoothie I don't see the commit to the terradatum/auto repo?

[hipstersmoothie]

now I have actually pushed

[rbellamy]

Will that cause a breaking change for any plugin that is currently using the synchronous tap?

[hipstersmoothie]

Nope. sync taps should work the same. This just adds the ability to tap a promise

[rbellamy]

I've successfully made this work with brittle/error-prone regex. My concern is that the <version/> element is used EVERYWHERE in a pom.xml file - for the artifact version AND for the dependencies and plugins. In other words, the regex is highly likely to inadvertently change a version it's not supposed to.

In trying to work with XML DOM (via xmldom, xmldom-ts, xml2js, xml2json, etc), I've found myself stuck in a quagmire of epic proportions. I by no means consider myself a typescript or javascript expert, so maybe that's where I'm falling down.

Two things are kicking my ass:

1. I can't figure out how to get IntelliJ to run the maven tests (and ONLY the maven tests) in debug mode. Sometimes it works, sometimes it doesn't.
2. Loading the pom.xml into a Document and then use xpath to select the "/project/version" and the "/project/parent/version" nodes.

Between those two issues I've probably spent 12 to 16 hours total, and I'm getting really frustrated. I had #1 working until this morning, and I would think #2 should be relatively simple, but it seems to be fighting me at every turn.

I'm inclined to go back to using the maven-release-plugin for the ridiculously simple reason that it handles updating the pom.xml files for me.

[hipstersmoothie]

I can't figure out how to get IntelliJ to run the maven tests (and ONLY the maven tests) in debug mode. Sometimes it works, sometimes it doesn't.

I'm sorry I'm not more of a help here. I use VSCode and rarely debug the tests. Can you run from root with yarn test plugins/maven?

Loading the pom.xml into a Document and then use xpath to select the "/project/version" and the "/project/parent/version" nodes.

This does seem like a good method. If that could work that seems good.

I'm inclined to go back to using the maven-release-plugin for the ridiculously simple reason that it handles updating the pom.xml files for me.

Is there only plugin based stuff like https://stackoverflow.com/questions/5726291/updating-version-numbers-of-modules-in-a-multi-module-maven-project available for maven? a low level mvn based thing would be perfect.

---

BTW thanks for spending so much time on this!

[rbellamy]

Thanks for the feedback and support. Sorry for the rant(ish) previous comment.

Great point about the versions-maven-plugin - I've not worked with it before and had no idea it existed. That will certainly make things easier for me... It's a perfect compromise between the maven-release-plugin and trying to manage the pom.xml files via the auto plugin.

I can get yarn test plugins/maven to work. I'm also a big fan of using the debugger to inspect things, rather than the more traditional console.log, so that's probably where I'm causing myself more heartburn than I need...

[rbellamy]

After a long and arduous journey, I almost have what I consider a full solution, that will either modify the pom.xml files directly or use the versions-maven-plugin to set versions.

There are some caveats. The code used to modify the pom.xml is working with a DOM, which has the following consequences:

1. There is a bug in tsconfig.json processing that requires that the "dom" lib is before "es2017" in "compilerOptions":

works:

"compilerOptions": [ "dom", "es2017" ]

doesn't work:

"compilerOptions": [ "es2017", "dom" ]

Without the "fix", the following occurs:

$ tsc -b tsconfig.dev.json
node_modules/@types/jsdom/index.d.ts:28:40 - error TS2304: Cannot find name 'DocumentFragment'.

28         static fragment(html: string): DocumentFragment;
                                          ~~~~~~~~~~~~~~~~

node_modules/@types/jsdom/index.d.ts:45:28 - error TS2304: Cannot find name 'Node'.

45         nodeLocation(node: Node): ElementLocation | null;
                              ~~~~

node_modules/@types/jsdom/index.d.ts:188:19 - error TS2304: Cannot find name 'HTMLScriptElement'.

188         element?: HTMLScriptElement | HTMLLinkElement | HTMLIFrameElement | HTMLImageElement;
                      ~~~~~~~~~~~~~~~~~

node_modules/@types/jsdom/index.d.ts:188:39 - error TS2304: Cannot find name 'HTMLLinkElement'.

188         element?: HTMLScriptElement | HTMLLinkElement | HTMLIFrameElement | HTMLImageElement;
                                          ~~~~~~~~~~~~~~~
<snip - numerous other errors>

2. The jest tests require testEnvironment: 'jsdom' as opposed to 'node'.

Without the "fix", the following occurs:

$ jest --runInBand plugins/maven
 FAIL  plugins/maven/__tests__/maven.test.ts
  maven
    updatePomVersion
      ✕ should replace the previousVersion with the newVersion (39ms)

  ● maven › updatePomVersion › should replace the previousVersion with the newVersion

    ReferenceError: XPathEvaluator is not defined

      51 | ) => {
      52 |   const pomDom = new jsdom.JSDOM(content, {contentType: "text/xml"}).window.document;
    > 53 |   const evaluator = new XPathEvaluator();
         |                     ^
      54 |   const resolver = evaluator.createNSResolver(pomDom.documentElement);
      55 |   const expression = evaluator.createExpression("/project/version", resolver);
      56 |   const versionNode = expression.evaluate(pomDom.documentElement, XPathResult.FIRST_ORDERED_NODE_TYPE);

      at Object.exports.updatePomVersion (plugins/maven/src/index.ts:53:21)
      at Object.test (plugins/maven/__tests__/maven.test.ts:53:26)

---

Neither of the above "fixes" work when set in the plugin's root configurations, e.g. neither plugins/maven/tsconfig.json nor a jest config in plugins/maven/package.json. Making the changes to the root config files did not appear to negatively affect tests or functionality.

[hipstersmoothie]

is this up on your branch? I can probably fix this

[rbellamy]

Not yet... I'm making sure all the tests (and new ones) pass before pushing. I'll ping you when they're ready for you to review.

[rbellamy]

I just rebased master onto my branch, and now I'm getting several build errors.

First the yarn.lock file was borked because of a merge conflict, so I had to delete and rebuild it:

error Incorrect integrity when fetching from the cache for "babel-plugin-jest-hoist". Cache has "sha512-u+/W+WAjMlvoocYGTwthAiQSxDcJAyHpQ6oWlHdFZaaN+Rlk8Q7iiwDPg2lN/FyJtAYnKjFxbn7xus4HCFkg5g== sha1-EpyAulx/x1uvOkW5Pi43LVfKJnc=" and remote has "sha512-+AuoehOrjt9irZL7DOt2+4ZaTM6dlu1s5TTS46JBa0/qem4dy7VNW3tMb96qeEqcIh20LD73TVNtmVEeymTG7w==". Run `yarn cache clean` to fix the problem

Once I did that, now I'm getting @octokit/graphql errors:

$ tsc -b tsconfig.dev.json
packages/core/src/auto.ts:2018:13 - error TS2571: Object is of type 'unknown'.

2018         if (result?.[`hash_${commit}`]) {
                 ~~~~~~

packages/core/src/auto.ts:2019:27 - error TS2571: Object is of type 'unknown'.

2019           const number = (result[`hash_${commit}`] as ISearchResult).edges[0]
                               ~~~~~~

packages/core/src/release.ts:286:52 - error TS2769: No overload matches this call.
  Overload 1 of 2, '(o: ArrayLike<unknown> | { [s: string]: unknown; }): [string, unknown][]', gave the following error.
    Argument of type 'unknown' is not assignable to parameter of type 'ArrayLike<unknown> | { [s: string]: unknown; }'.
      Type 'unknown' is not assignable to type '{ [s: string]: unknown; }'.
  Overload 2 of 2, '(o: {}): [string, any][]', gave the following error.
    Argument of type 'unknown' is not assignable to parameter of type '{}'.

286       (acc, result) => [...acc, ...(Object.entries(result) as QueryEntry[])],
                                                       ~~~~~~


packages/core/src/__tests__/git.test.ts:209:12 - error TS2571: Object is of type 'unknown'.

209     expect(result!.data).not.toBeUndefined();
               ~~~~~~~


Found 4 errors.

error Command failed with exit code 2.

I was super hopeful I would be able to create a PR for this - but I'm concerned it's not ready yet.

[rbellamy]

Okay, I worked out what I needed to do - I just checked out the yarn.lock file from master and then re-ran yarn clean && yarn install && yarn build && yarn test plugins/maven and I think I'm good-to-go.

[hipstersmoothie]

Awesome! I'll review this today. Sorry I didn't respond over the weekend!

[adierkens]

🚀 Issue was released in v9.40.0 🚀