Skip to content
This repository has been archived by the owner on Sep 8, 2021. It is now read-only.

add np to automate release steps #39

Merged
merged 2 commits into from
Jan 8, 2021
Merged

add np to automate release steps #39

merged 2 commits into from
Jan 8, 2021

Conversation

sndrs
Copy link
Member

@sndrs sndrs commented Dec 10, 2020

What does this change?

image

@JamieB-gu
Copy link
Contributor

As far as I understand it, this is meant to be run from your repo directory on an up-to-date version of the main branch? Does that mean it requires pushing directly to main?

@sndrs
Copy link
Member Author

sndrs commented Dec 10, 2020

you run it locally, so it will bump package.json and add a tag too, and push that. so that's a good point, you'd need someone with write permission to main to run it

Copy link
Member

@tjmw tjmw left a comment

Choose a reason for hiding this comment

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

This LGTM 👍

(And thanks for the reminder that I've been meaning to give np a try on another project).

Copy link
Member

@akash1810 akash1810 left a comment

Choose a reason for hiding this comment

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

Nice! We've recently added np to the cdk project and it's excellent. There's a slight caveat when you have branch protection enabled, as described here. We've not properly solved it yet; would be good to get your thoughts.

Update: If you run tsc in a prepublish script, I think you can remove prepare as the tarball will have the compiled code. I've found running npm pack locally to be useful to understand what will get published.

@@ -44,5 +46,8 @@
"dist"
],
"preset": "ts-jest"
},
"np": {
"yarn": false
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you want to be explicit about the branch too to enforce only main being used for publishing?

Suggested change
"yarn": false
"yarn": false,
"branch": "main"

Copy link
Member Author

Choose a reason for hiding this comment

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

main or master are the defaults

@sndrs
Copy link
Member Author

sndrs commented Dec 11, 2020

there no easy solution if your main branch is protected, because making sure main is in sync with the released version requires writing to main as part of the process.

given this repo has no CD setup, what is the value of protecting main? (versus the benefit of the release process being able to write package metadata)

@JamieB-gu
Copy link
Contributor

I think you can remove prepare as the tarball will have the compiled code

Removing prepare would break the GitHub installs I think, because the tarball only applies to the registry? Unless there's a way to associate one with GitHub releases?

given this repo has no CD setup, what is the value of protecting main?

Preventing developers from accidentally pushing to main? Unless there's another way to prevent that?

Copy link
Contributor

@oliverlloyd oliverlloyd left a comment

Choose a reason for hiding this comment

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

Shall we?

@oliverlloyd
Copy link
Contributor

@sndrs any objections to me merging this one? np seems like a great lib and I'm keen to try it out!

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

Successfully merging this pull request may close these issues.

5 participants