Remove --mutex network argument for Yarn builds

[scherler]

add profile for yarn 2 which do not support the mutex argunment anymore

Signed-off-by: Thorsten Scherler [email protected]

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[jtnord]

I guess the same applied for yarn3? (the --mutex flag is a yarn 1 thing right?)

[jtnord]

I am wondering rather than replicate the entire profile except the argument should the extra argument just be pulled out as a property and referenced?

Then if you want to use yarn2+ you set the argument to empty in your projects <properties>.

[scherler]

@jtnord yeah what you recommend is much cleaner, how could I do that?

[jtnord]

@jtnord yeah what you recommend is much cleaner, how could I do that?

in the existing profile remove the --mutex blah blah and instead add ${yarn.extraArguments}

then in the top level <properties> add a new property like
<yarn.extraArguments>install --mutex blah blah</yarn.extraArguments> <!-- yarn 1 requires a mutex but this is not supported in yarn2+ yarn2 users can just use install here-->

infact looking at the frontend plugin code... you can remove the setting in both places and just set the property frontend.yarn.arguments to install --mutex with a comment can be set to the empty string or install for yarn2

[basil]

Commit 7556d86 re-indents the whole file. Do not include unrelated formatting changes in this pull request.

[scherler]

@basil fix it, sorry about that. @jtnord like this?

[basil]

frontend-maven-plugin already uses the value of the frontend.yarn.arguments property, if defined, so for consistency I suggest using that same property name rather than yarn.args. That would allow you to delete this <arguments> line (and therefore its parent <configuration>).

[basil]

(see https://github.com/eirslett/frontend-maven-plugin/blob/ec9f8050bc4c4014bbeb1ea84ae92e11682d9764/frontend-maven-plugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/YarnMojo.java#L26)

[scherler]

Ok @basil , the only thing is that we do

 <properties>
    <yarn.version>1.22.19</yarn.version>
  </properties>

So you are suggesting that would now become

 <properties>
    <yarn.version>1.22.19</yarn.version>
    <frontend.yarn.arguments>1.22.19</frontend.yarn.arguments>
  </properties>

I am fine with it, but personally would find it more readable, if we would drop the frontend prefix since it just forces us to remember it. Better like:

 <properties>
    <yarn.version>1.22.19</yarn.version>
    <yarn.arguments>1.22.19</yarn.arguments>
  </properties>

wdyt?

[jtnord]

should be clearer now my previously unpublished comments are published, the question remains is what (if anything) should the default be (keep the default as yarn1 or remove it and make users specify it if they use yarn1)

[jtnord]

forgot to sumbit my review

[jtnord]

given we now require you to specify the yarn version (the previous release) I think we should just remove the property and the use of it on line 1333.

Then we document that users of yarn1 will need to add a property frontend.yarn.arguments to ---mutex network

@basil removed the node/yarn properties so I think this may well be better suited.
if not and we want to keep the default the property needs to be changed to <frontend.yarn.arguments>---mutex network</frontend.yarn.arguments> <!-- extra arguments needed for yarn --> to match the property of the plugin to make the understanding and override of it easier.

Suggested change

	<yarn.args>---mutex network</yarn.args>
	<frontend.yarn.arguments>---mutex network</frontend.yarn.arguments> <!-- extra arguments needed for yarn, --mutex network is needed for yarn1, yarn2+ do not need this flag -->

[scherler]

yarnpkg/yarn#2146 (comment) you can as well create a .yarnrc and add --install.mutex network in case you want it.

[jtnord]

should be clearer now my previously unpublished comments are published, the question remains is what (if anything) should the default be (keep the default as yarn1 or remove it and make users specify it if they use yarn1)

[scherler]

I personally think that the default behavior should be not adding anything. The mutex option is really a safeguard and I wonder which percent of cases really need these defaults. Further most likely only on the CI build, so you could create a .yarnrc in a pipeline step like echo '--install.mutex network' >> .yarnrc so you really only use it if needed.

[scherler]

@basil @jtnord as soon we have reached a decision on whether we want to keep the old default config or we drop it, I can apply the code suggestion from @jtnord

[jtnord]

I personally think that the default behavior should be not adding anything.

I would normally be in agreement - however there would appear to be no users of yarn2+ as you are the first to hit the issue, so that would mean everyone using yarn would need to add it. That said that was the approach in setting the yarn versions.

Further most likely only on the CI build, so you could create a .yarnrc in a pipeline step like echo '--install.mutex network' >> .yarnrc so you really only use it if needed.

That would mean that the standard pipeline would need to know you are going to be running yarn1 vs yarn2 or 3 in order to only set it for the legacy case (setting it for yarn2+ should IIUC cause yarn to fail as it does not understand that switch)?

[timja]

I personally think that the default behavior should be not adding anything. The mutex option is really a safeguard and I wonder which percent of cases really need these defaults. Further most likely only on the CI build, so you could create a .yarnrc in a pipeline step like echo '--install.mutex network' >> .yarnrc so you really only use it if needed.

Not needed on ci.jenkins.io which only runs one build at a time on an executor, only needed if someones running internally with multiple executors

[basil]

+1 from me for removing any custom arguments and requiring consumers to specify any such arguments via the <frontend.yarn.arguments> property (if desired), especially if the existing custom argument is only relevant for Yarn 1.x and not needed on ci.jenkins.io anyway.

[scherler]

To summarize, I will prepare the PR removing the old defaults but allowing passing arguments. Thanks all!

[scherler]

@jtnord @basil @timja if you could validate the last commit, whether it is as we decided?

[jtnord]

this is the default - you can just remove the entire configuration section (lines 1331 - 1333)

[basil]

As James mentioned a few hours ago, and as I previously wrote in #625 (comment), there is no need to declare an empty property and pass it through to frontend-maven-plugin when frontend-maven-plugin is already defaulting to the same thing, so the property and <configuration> section can be deleted. See also #628, including the description of the Testing done in the last sentence.

[scherler]

Thank you for the clarification and sorry for being slow, but I think I understand now.

[basil]

The whole <configuration> section can be deleted, since it no longer contains any customizations from the upstream default. Did you have a draft of what should be published in the release notes explaining how Yarn 1.x users should adapt to this change?

[basil]

Do we have a proposed entry for the release notes that describes the impact this change will have on plugins?

[scherler]

@basil for the release note:

Breaking change in yarn. In case you rely on the extra argument --mutex network that has been injected until now, you have to now define a property like <frontend.yarn.arguments>--mutex network</frontend.yarn.arguments>

WDYT?

[basil]

Sounds fine. How has this change been tested? I would expect this change to be tested under two use cases:

• Ensure that a plugin that relied on --mutex network still gets that setting plumbed through to Yarn when upgrading to the POM from this PR and defining the <frontend.yarn.arguments>--mutex network</frontend.yarn.arguments> property (i.e. no regression has occurred for existing use cases)
• That a plugin that wishes to use Yarn 2 can use it when upgrading to the POM from this PR and defining yarn.version to a 2.x version (i.e., the new use case is supported)

[scherler]

How can I provide these test cases?

[basil]

I would run mvn clean install -Dinvoker.skip, then find a plugin that uses Yarn 1.x and relies on --mutex network, update its plugin parent POM to 4.50-SNAPSHOT, add <frontend.yarn.arguments>--mutex network</frontend.yarn.arguments>, then verify that the argument is still getting plumbed through to Yarn. Then I would find a plugin where Yarn 2.x is desired, update its plugin parent POM to 4.50-SNAPSHOT, update yarn.version to a 2.x version, and verify that Yarn 2.x is successfully invoked when running mvn clean verify.

[scherler]

jenkinsci/design-library-plugin#158 is passing with your instructions now on my local system and I have pushed but I would imagine that the build will fail. Before the build failed locally due to the --mutex problem. Adding the arguments make it fails again

[scherler]

I first used yarn.arguments and that passes since it is not frontend.yarn.arguments. IMHO the first makes more sense on an user perspective. ...or you should use frontend.yarn.version as well.

[basil]

Thanks for the PR! I will try to do a release of this tomorrow once the this week's weekly release is out.

[scherler]

@basil thanks for your big help