Conversation
type: string | ||
steps: | ||
# Allows us to authenticate with the npm registry | ||
- run: echo "//registry.npmjs.org/:_authToken=$NPM_TOKEN" > ~/.npmrc |
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.
We should use scripts for everything that is more than single line
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.
Each of these individual steps is a script. Wrapping those ones up into a bigger script obfuscates how the release process works. I think this usage of CircleCI commands to not duplicate the npm publishing steps is really nice stuff.
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 still not 100% sure about the idea of publishing master versions to npm because it will generate a lot of noise and the module on npm will contain tonnes of spammy looking package versions. However I think the it's done in this PR is pretty good. Going to leave final review to @wtrocki
Thanks for review. With more master versions on npm I agree that this might not be a good idea. Would it make sense to keep only the latest master on npm - with every master release unpublish the previous one? @darahayes @wtrocki |
I think from my side suggestion will be to use A side impact of this change is that I will probably need to do less renovate merges. Aggregate them somehow. Renovate tends to create so many PR's and it can mean 5 releases in an hour where we address them. Obviously, we can wait with addressing those and this is just a workflow change. |
@wtrocki I have changed it so that |
I will ping you on next merge so we can verify that in action |
Description
Motivation: https://issues.jboss.org/browse/AEROGEAR-9334