Support for explicit registry argument by noherczeg · Pull Request #563 · lerna/lerna

noherczeg · 2017-02-01T03:21:16Z

In response to #515

evocateur

We're headed in the right direction, but I think we need to support the new getOptions() API (and thus lerna.json config) at the very least.

I'm concerned that other npm commands might need the registry config as well (dist-tag, install).

evocateur · 2017-02-01T17:39:02Z

+#### --registry [registry]
+
+```sh
+$ lerna publish --registry https://my-private-registry


I'm not sure publish is the only command that would need this; certainly bootstrap could also potentially need it (for example, dependencies on private modules that aren't co-located in the monorepo).

We should also allow this setting in lerna.json, accessed via the Command##getOptions() API.

I might even suggest that all npm operations provide the registry flag -- I think that's what I would expect if I provided a registry key in lerna.json, for instance.

I'll look into your suggestions, and work on them!

evocateur · 2017-02-01T17:47:44Z

+      }
+    });
+    let command = ("npm publish " + args.join(" ")).trim();
+    ChildProcessUtilities.exec("cd " + escapeArgs(directory) + " && " + command, null, callback);


I'm not a huge fan of this argument whitelisting, though I understand the purpose.

Instead of direct flags, I would rather provide config like this via env vars passed in the options argument.

const command = ("npm publish --tag " + tag).trim() const env = { npm_config_registry: opts.registry }; ChildProcessUtilities.exec("cd " + escapeArgs(directory) + " && " + command, { env }, callback);

evocateur · 2017-02-01T17:53:45Z

+          "tag": "lerna-temp"
+        };
+        if (this.flags.registry) {
+          args.registry = this.flags.registry;


this should use getOptions()

I just noticed that that exists after the PR, will swap it out!

noherczeg · 2017-02-01T23:41:52Z

@evocateur You mentioned dist-tag for this to apply to as well, but I fail to see what does that have to do with registries. Could you please elaborate on this?

evocateur · 2017-02-02T00:37:58Z

What happens when you try to query a dist-tag from a registry where the package doesn't exist?

…

On Feb 1, 2017, at 15:41, Norbert Csaba Herczeg ***@***.***> wrote: @evocateur You mentioned dist-tag for this to apply to as well, but I fail to see what does that have to do with registries. Could you please elaborate on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

- added registry support for bootstrap - modified config handling, now using getOptions() - added tests - updated readme

noherczeg · 2017-02-02T01:56:03Z

@evocateur My bad. I checked the docs before my question, but failed to realize that this is not a git related "tag" (never made tags as aliases before). My last addition still does not include dist tag support, but I'll make it happen next evening.

evocateur

Please constrain this PR to --registry and lerna.json registry only. If there's a great need for the Nexus stuff, we can do that in a separate PR.

Thanks!

evocateur · 2017-02-02T02:44:23Z

    }

-    const opts = {
+    let opts = {


const doesn't prohibit object mutation, only reassignment to the identifier.

evocateur · 2017-02-02T02:47:21Z

+> The `registry` flag can also be set in lerna.json.
+
+There could also be scenarios where your private registry is split up into an upstream
+and a downstream part (like in [Nexus3](https://books.sonatype.com/nexus-book/3.0/reference/npm.html)).


Let's keep this initial implementation as simple as possible. Is there a great need for Nexus3 support from lerna users?

We are using Nexus, that's where the example came from. I understand your point about keeping it simple, therefore I will remove every other part besides registry from my PR. Mostly because nothing prevents us from keeping our upstream publishConfigs in the packages like they are currently :)

I agree with @evocateur that the initial implementation should be as simple as possible, but I'd still like to see nexus support in a subsequent pr -- we're using it at the office.

noherczeg · 2017-02-04T19:00:56Z

Hey guys!

I'm curious if you find my PR okay or is there anything I can make it better?

doug-wade · 2017-02-04T20:07:21Z

+$ lerna bootstrap --registry https://my-private-registry
+```
+
+When run with this flag, the above commands will use the specified registry for your package(s).


not just the above commands anymore, right?

Bad wording, will change it in the evening!

I checked it, and found that the run command does not get decorated yet. Could you guys give me some wisdom on this? Do we need it to be present on run-s as well? I'm not sure about it.

doug-wade · 2017-02-04T20:12:06Z

-    ChildProcessUtilities.execSync(`npm dist-tag add ${packageName}@${version} ${tag}`);
+  static addDistTag(packageName, version, tag, registry) {
+    const opts = NpmUtilities.getTagOpts(registry);
+    ChildProcessUtilities.execSync(`npm dist-tag add ${packageName}@${version} ${tag}`, opts);


making this part of the public api like this makes me think you're shelling out to npm every time. I feel like

ChildProcessUtilities.execSync(`npm dist-tag add ${packageName}@${version} ${tag}`, {registry});

would be better, as would making getTagOpts a function

ChildProcessUtilities.execSync(`npm dist-tag add ${packageName}@${version} ${tag}`, getTagOpts(registry));

I put it there because I thought that since NPM is handling the publish process it'd be a good enough fit there. By "making getTagOpts a function" do you mean a global distinct function?

evocateur

Thanks for your patience with this PR, @noherczeg !

We're almost there.

evocateur · 2017-02-10T19:42:42Z

    });

    this.progressBar.init(this.packagesToPublish.length);
+    const registry = this.getOptions().registry;


Let's set this as an instance prop in the initialize method, like this.npmRegistry = this.getOptions().registry;

evocateur · 2017-02-10T19:43:49Z

+          { args: ["cd " + escapeArgs(path.join(testDir, "packages/package-4")) + " && npm publish --tag lerna-temp", {env: {"npm_config_registry":"https://my-private-registry"}}] }
+          // No package-5.  It's private.
+        ], true],
+        [ChildProcessUtilities, "execSync", {}, [


I thought the dist-tag commands passed the env var too?

@evocateur Sorry, but I can not make the tests error out on me. Seems like I misunderstood how to do them properly so I need to evaluate again :(

- added registry support for bootstrap - modified config handling, now using getOptions() - added tests - updated readme

noherczeg · 2017-02-11T17:52:01Z

Webstorm totally messed up my repo during conflict resolution, will create a proper one soon. Sorry for the inconvenience!

evocateur · 2017-02-11T18:35:12Z

Let's stay on this PR to maintain comment history. Feel free to force push into your branch to correct the errors, no need for a "fresh" branch. We'll squash merge anyway, so no worries about noisy commits. :)

…

On Feb 11, 2017, at 09:52, Norbert Csaba Herczeg ***@***.***> wrote: Webstorm totally messed up my repo during conflict resolution, will create a proper one soon. Sorry for the inconvenience! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

noherczeg · 2017-02-11T19:17:25Z

@evocateur I messed it up really bad, so I didn't wanted to risk losing anything, or creating corrupt commits (because I barely managed to keep track what happened where).

lock · 2018-12-27T19:23:56Z

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Support for explicit registry argument

f0050ae

evocateur suggested changes Feb 1, 2017

View reviewed changes

noherczeg added 2 commits February 2, 2017 02:41

Feature update

8d6b1ba

- added registry support for bootstrap - modified config handling, now using getOptions() - added tests - updated readme

fix lint error

452971e

evocateur suggested changes Feb 2, 2017

View reviewed changes

Removed unwanted features, following recommendations

0485845

doug-wade suggested changes Feb 4, 2017

View reviewed changes

evocateur suggested changes Feb 10, 2017

View reviewed changes

noherczeg added 7 commits February 11, 2017 15:19

Following guidelines, test corrections

126d2b3

Support for explicit registry argument

14a368c

Feature update

57e065d

- added registry support for bootstrap - modified config handling, now using getOptions() - added tests - updated readme

fix lint error

a1fb460

Removed unwanted features, following recommendations

fd697e7

Following guidelines, test corrections

cbf1614

merge remote

b23b299

noherczeg closed this Feb 11, 2017

noherczeg mentioned this pull request Feb 11, 2017

Explicit registry flag feature #599

Merged

lock Bot locked as resolved and limited conversation to collaborators Dec 27, 2018

Conversation

noherczeg commented Feb 1, 2017

Uh oh!

evocateur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doug-wade Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noherczeg commented Feb 1, 2017

Uh oh!

evocateur commented Feb 2, 2017 via email

Uh oh!

noherczeg commented Feb 2, 2017

Uh oh!

evocateur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doug-wade Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noherczeg commented Feb 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evocateur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noherczeg commented Feb 11, 2017

Uh oh!

evocateur commented Feb 11, 2017 via email

Uh oh!

noherczeg commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lock Bot commented Dec 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

doug-wade Feb 1, 2017 •

edited

Loading

doug-wade Feb 2, 2017 •

edited

Loading

noherczeg commented Feb 11, 2017 •

edited

Loading