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

Allow explicit private publish to prevent public publish prompt #497

Merged

Conversation

kylemh
Copy link
Sponsor Contributor

@kylemh kylemh commented Feb 12, 2020

Fixes #496

This will enable private, Org-scoped packages to easily publish new versions of a package as part of a CI/CD process by preventing the prompt of publically publishing a package when publishConfig.access is explicitly set to 'restricted' in package.json.

Also "booleanified" runPublish -> shouldRunPublish and reduced the number of times the same boolean is re-defined.

@kylemh kylemh changed the title Allow explicit private publish Allow explicit private publish to prevent public publish prompt Feb 12, 2020
Copy link
Collaborator

@chinesedfan chinesedfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether setting publishConfig.access will prevent the prompt or not. So will keep silent for it and hope we can handle every prompt friendly in future.

About runPublish,

  • Glad to see replacing options.publish && !pkg.private with runPublish as I commented.
  • But would not like extract it as shouldRunPublish. The name is not consistent with runTests, runCleanup. Considering we have no sufficient tests and the redundant logic is short, I prefer the conservative way.

source/index.js Outdated Show resolved Hide resolved
source/ui.js Outdated Show resolved Hide resolved
source/ui.js Show resolved Hide resolved
source/ui.js Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@itaisteinherz itaisteinherz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻 Thanks for working on this @kylemh!

@itaisteinherz
Copy link
Collaborator

@chinesedfan Have you tested this locally and can verify this works for you? (I haven't tested this out myself, but will hopefully get to it sometime in the next few days.)

@chinesedfan
Copy link
Collaborator

chinesedfan commented Feb 22, 2020

@itaisteinherz I didn't have an end-to-end package to test. Here are how I tested,

  • Add some fields in package.json of np
	"private": true,
	"publishConfig": {
		"access": "restricted"
	},
  • Run, node source/cli.js and select the version
  • I saw it entered prerequisite-tasks stage(i.e. Git -> Check current branch) without any other prompts.

@kylemh
Copy link
Sponsor Contributor Author

kylemh commented Feb 23, 2020

FWIW, I exported my own fork of this PR and am using it in a repository already. I know y'all ensure everything's perfect with PRs, but I needed it immediately 😂

@itaisteinherz
Copy link
Collaborator

Merging as this is ready IMO. Thanks again for working on this @kylemh 🙏🏻

@itaisteinherz itaisteinherz merged commit 08b52a2 into sindresorhus:master Feb 26, 2020
@kylemh kylemh deleted the allow-explicit-private-publish branch February 26, 2020 19:13
@kylemh
Copy link
Sponsor Contributor Author

kylemh commented Feb 26, 2020

Thanks! @itaisteinherz

FWIW, I'm a little afraid of the wording with private just because I went down a rabbit hole of adding private: true which actually prevents publishes entirely

@itaisteinherz
Copy link
Collaborator

FWIW, I'm a little afraid of the wording with private just because I went down a rabbit hole of adding private: true which actually prevents publishes entirely

I agree it's a bit confusing, but it's also the terminology npm uses. In this case, I think adding a note to the readme would help avoid any confusion. If you're up for doing that and creating another PR, that would be great 😉

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

Successfully merging this pull request may close these issues.

Documentation: Clear instruction on CI publishing of private packages
3 participants