-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat: npm trust command #8899
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
base: latest
Are you sure you want to change the base?
feat: npm trust command #8899
Conversation
owlstronaut
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this. Just a couple of nits, but I think this will be a great architecture to build from.
I skipped over reviewing the tests as I only have so much time on this beautiful planet
| static positionals = null | ||
|
|
||
| // this is a static so that we can read from it without instantiating a command | ||
| // this is a static so that we can read =rom it without instantiating a command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // this is a static so that we can read =rom it without instantiating a command | |
| // this is a static so that we can read from it without instantiating a command |
Mistype?
| #argv = undefined | ||
| #excludeNpmCwd = undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these used?
| // Throw error if unknown flags were found | ||
| if (unknownFlags.length > 0) { | ||
| const flagList = unknownFlags.map(f => `--${f}`).join(', ') | ||
| throw new Error(`Unknown flag${unknownFlags.length > 1 ? 's' : ''}: ${flagList}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would a usageError be nice here?
| await this.confirmOperation(yes) | ||
| const trustConfig = this.constructor.optionsToBody(options.values) | ||
| const response = await this.createConfig(options.values.package, [trustConfig]) | ||
| const body = await response.json(yes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const body = await response.json(yes) | |
| const body = await response.json() |
I think you only wanted this for the this.confirmationOperation above
| } | ||
| await this.confirmOperation(yes) | ||
| const trustConfig = this.constructor.optionsToBody(options.values) | ||
| const response = await this.createConfig(options.values.package, [trustConfig]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the request body be an object trustConfig or an array of single object [trustConfig]?
Introducing
npm trust🎉A new command / set of registry api endpoints that allows a user to add, remove and check the Trusted configurations between npm's registry and Trusted Publishers.
This command will allow maintainers to have a mechanism for managing their configurations outside of the npm website.
A lot of maintainers have mentioned how they'd like to be able to set Trusted Publishing "in bulk" and this solution allows maintainers to setup a simple bash script to loop over and set these configurations for many packages / repositories quickly.
These commands will require two-factor auth, it's not based on package access settings, or your account having 2fa enabled. The two-factor confirmation page will have the 2fa "cooldown" checkbox like
npm publishwhich will allow you to not have to re-enter the 2fa for this endpoint for a set amount of time for a given IP address.