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

Add a configuration option to set --network-concurrency flag #2457

Closed
tobico opened this issue Jan 16, 2017 · 11 comments
Closed

Add a configuration option to set --network-concurrency flag #2457

tobico opened this issue Jan 16, 2017 · 11 comments

Comments

@tobico
Copy link

tobico commented Jan 16, 2017

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
Network concurrency can be specified on the command line thanks to #2129, but if you forget to pass this option it will run with the default concurrency, and in our use-case we get blocked from GitHub SSH access for the next hour as a result.

What is the expected behavior?
We'd like to be able to add an option to our .yarnrc that sets a new default for the network concurrency option.

Please mention your node.js, yarn and operating system version.
node v6.7.0, yarn v0.20.0-20170107.1855, OSX v10.12.2

@bestander
Copy link
Member

Yeah, I think we should expose all of those flags via configuration options.
Any idea to make it generic?

@cliffmeyers
Copy link

cliffmeyers commented Apr 17, 2017

Our team has a similar desire to be able to set the --mutex network option via .yarnrc as well. The issue we're facing appears to be related to #2629 which was possibly fixed in #3090 - although that PR doesn't appear to be available in any official release yet.

Regardless of that fix, supporting more config options in .yarnrc seems desirable. I believe that npm allows for any supported config option to be set in .npmrc as described here:
https://docs.npmjs.com/files/npmrc

I noticed there is a mechanism in cli/index.js to grab any rc args and map them onto the commander's args:
https://github.com/yarnpkg/yarn/blob/master/src/cli/index.js#L139

And the code responsible for parsing out the individual rcargs is here:
https://github.com/yarnpkg/yarn/blob/master/src/rc.js#L40

The intent of miniparse is unclear to me: the end result seems to allow any flag prefixed with double-dash to be passed through to commander, but other args are ignored.

For example, if I hand-edit .yarnrc and add a line such as this:
--mutex network

Then I find that commander has its mutex option set to network.

I expect this usage is unintended, as I can't actually set this option in the yarnrc via a command like this:
yarn config set --mutex network
as those options are getting "stripped" by commander, presumably. I tried wrapping it with various strings with no success but that doesn't feel like the right way to offer this functionality. The double-dash prefix feels like a command line argument convention; in a config file I'd expect just to set the key value pair.

@bestander perhaps you could explain the intent of that filtering? Then we could discuss what the approach would work best to add this functionality?

@bestander
Copy link
Member

bestander commented Apr 17, 2017

This PR added ability to set flags in .yarnrc file #3033.
That should work since 0.23.

Did you try putting --mutex network in the yarnrc manually?
cc @arcanis

@cliffmeyers
Copy link

@bestander @arcanis yes, as I indicated above, adding --mutex network by hand to .yarnrc gives the desired behavior. I feel that might not be the best approach for the following reasons:

  • A yarn user would expect that config options should be able to be set via yarn config set key value and this doesn't work presently (as outlined above)
  • It seems slightly odd for .yarnrc have a mix of "bare" options (such as "email") and "prefixed" options (such as "--mutex")
  • Hand-editing the .yarnrc file is explicitly discouraged in the comment at the top of the file: THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

I certainly appreciate the effort put forth in #3033 and it does work, I just wonder whether it could be improved. I'm willing to open a PR but wanted to discuss the approach before laying down any code.

An alternative I'd considered would be the following:

  • Create an explicit list of config keys which are stored in .yarnrc but which are not valid command line flags (such as "email" or "lastUpdateCheck")
  • Add some logic that ensures any keys in the above list are not passed through to the execution of yarn, but allow all other flags to be passed through.

@bestander
Copy link
Member

bestander commented Apr 17, 2017 via email

@cliffmeyers
Copy link

I discovered recently that there is a totally reasonable "workaround" for setting command line params in .yarnrc:

yarn config set -- --mutex network

The use of the bare double-dash allows the subsequent parameters to be passed down to yarn rather than evaluated as part of the command.

I'm not sure if that resolves @tobico 's issue too. If it does, then perhaps this issue can be closed and no PR would be required.

@choliver
Copy link

@bestander / @cliffmeyers - Setting the mutex globally (i.e. in .yarnrc) leads to problems of its own in some cases. For example, the mapbox-gl postinstall step explicitly invokes Yarn, which leads to deadlock.

@bestander
Copy link
Member

That is why we should pressure package owners not to call package manager from within a package manager installation, in most cases Yarn/npm is used as curl there.

@cliffmeyers
Copy link

@choliver thanks for the heads up.

@BYK
Copy link
Member

BYK commented Oct 17, 2017

Do we still need this or can we close it since yarn config set -- --mutex network works?

@bestander
Copy link
Member

@BYK is right, this config can be done via https://yarnpkg.com/en/docs/yarnrc#toc-cli-arguments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants