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

feat: ✨ added support for using current npm registry #673

Closed

Conversation

rluvaton
Copy link

@rluvaton rluvaton commented Oct 6, 2021

Description
Use the current NPM registry

Related issue(s)
Fixes #538

@rluvaton
Copy link
Author

rluvaton commented Oct 6, 2021

I would be happy if you could add the hacktoberfest-accepted label to this PR 😄

I would update the tests shortly

@derberg
Copy link
Member

derberg commented Oct 7, 2021

I would be happy if you could add the hacktoberfest-accepted label to this PR

is it needed if after all the whole repo has hacktoberfest topi?

There are 2 challenges I have with proposed solution:

  • libnpmconfig is an archived project and one of its dependencies probably won't work with npm7, and even if it will, maintainers are pretty clear about the library status. This is not a critical package, but if its dependencies already have mixed status than long term, any security updates will cause lots of issues 😞
  • we now use Arborist, work with npm programmatically without using npm installation, and this solution basically expects you have npm installed as your npm config is read. Why do you think it is better than enabling authorization on library API level?

@rluvaton
Copy link
Author

rluvaton commented Oct 7, 2021

I would be happy if you could add the hacktoberfest-accepted label to this PR

is it needed if after all the whole repo has hacktoberfest topi?

I haven't saw that, my bad, it's not needed than.

There are 2 challenges I have with proposed solution:

  • libnpmconfig is an archived project and one of its dependencies probably won't work with npm7, and even if it will, maintainers are pretty clear about the library status. This is not a critical package, but if its dependencies already have mixed status than long term, any security updates will cause lots of issues 😞

Yeah I saw and I thought about it too but the library is not deprecated so instead of running child process with npm config get registry (which I'm trying to avoid) I thought it's better to do this instead, but I'm on the fence there.

There is also npm/config package that NPM use it in their CLI code but it seems like it only getting you the config at specific place

  • we now use Arborist, work with npm programmatically without using npm installation, and this solution basically expects you have npm installed as your npm config is read. Why do you think it is better than enabling authorization on library API level?

I don't really understand what you mean. Anyway, Artborist doesn't use the currently used npm registry.

@rluvaton
Copy link
Author

@derberg just pinging you in case you haven't saw my comment

@derberg
Copy link
Member

derberg commented Oct 13, 2021

@rluvaton ah, you are right, I missed you comment, thanks for pinging me 🙇🏼

My point is, based of course on my understanding of Arborist, is to use Arborist for authentication with npm registries. So you can pass to generator library and cli an environment variable, like NPM_TOKEN and once it is present, we use it in the Arborist API. Look at their docs 👇🏼 we can enable different environment variables for different auth methods and use them in the generator + enable users to pass the custom registry URL through API/CLI

const arb = new Arborist({
  // options object

  // where we're doing stuff.  defaults to cwd.
  path: '/path/to/package/root',

  // url to the default registry.  defaults to npm's default registry
  registry: 'https://registry.npmjs.org',

  // scopes can be mapped to a different registry
  '@foo:registry': 'https://registry.foo.com/',

  // Auth can be provided in a couple of different ways.  If none are
  // provided, then requests are anonymous, and private packages will 404.
  // Arborist doesn't do anything with these, it just passes them down
  // the chain to pacote and npm-registry-fetch.

  // Safest: a bearer token provided by a registry:
  // 1. an npm auth token, used with the default registry
  token: 'deadbeefcafebad',
  // 2. an alias for the same thing:
  _authToken: 'deadbeefcafebad',

  // insecure options:
  // 3. basic auth, username:password, base64 encoded
  auth: 'aXNhYWNzOm5vdCBteSByZWFsIHBhc3N3b3Jk',
  // 4. username and base64 encoded password
  username: 'isaacs',
  password: 'bm90IG15IHJlYWwgcGFzc3dvcmQ=',

  // auth configs can also be scoped to a given registry with this
  // rather unusual pattern:
  '//registry.foo.com:token': 'blahblahblah',
  '//basic.auth.only.foo.com:_auth': 'aXNhYWNzOm5vdCBteSByZWFsIHBhc3N3b3Jk',
  '//registry.foo.com:always-auth': true,
})

does it make sense? it gives you more flexibility imho

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rluvaton
Copy link
Author

IMO I think we should use the currently used registry and maybe let the user pass custom registry, this way we don't need to handle authentication and stuff, if the user wants to use custom registry only for this he can create an .npmrc file.

WDYT?

@rluvaton
Copy link
Author

rluvaton commented Oct 13, 2021

Also, doing this we need to support every change in the NPM capabilities, I suggest that we don't use the Arborist package at all as it doesn't use by default the local machine configuration and if we only use it to download packages we better use better solution


EDIT: I see that npm CLI uses Arborist so I take that back https://github.com/npm/cli/blob/latest/lib/install.js#L169-L170

But I still think we should use the current configuration rather than implement it by ourselfs

@derberg
Copy link
Member

derberg commented Oct 18, 2021

@rluvaton the reason we switched to Arborist as it is the most latest and modern approach to manage NPM modules installation. We had too many issues with reusing NPM clients. Thus my request is for us to use Arborist for private/custom registries. Using the config file could work as a fallback

@rluvaton
Copy link
Author

rluvaton commented Oct 18, 2021

😀 so what we need to do is add those flags: --registry, --username, --password, --auth-token, right?

@derberg
Copy link
Member

derberg commented Oct 18, 2021

@rluvaton yes, and have it in Generator library of course.

Critical to remember is security. By default, we must support passing secret data using environment variables. Flag for password and token should be there to just overwrite env variables.

@zmt-Eason
Copy link

Hi, I was wondering if this issue is handled, for I have met the same question.

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 31, 2022

@rluvaton what do you need to continue here?

@rluvaton
Copy link
Author

I'm sorry, crazy times, if someone wants she/he can continue this PR...

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2022

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

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

Successfully merging this pull request may close these issues.

Generator doesn't pull template from private repo
4 participants