-
Notifications
You must be signed in to change notification settings - Fork 14
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
Added action to publish to npm on release #175
Conversation
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 think this is a bit too much of a copy from Vanilla, we can clean up and simplify things.
- run: yarn install | ||
- run: yarn build | ||
- run: yarn test | ||
- name: Show size of the build file |
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 Vanilla specific and is not needed here
- run: yarn test | ||
- name: Show size of the build file | ||
run: stat -c '%s' build/css/build.css | ||
- run: cp CURRENT_VERSION build/css |
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 also is likely Vanilla specific (we need this for publishing on assets server)
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.
Definitely, I left it in because it seemed interesting, but it makes the process a bit longer
- uses: actions/upload-artifact@v4 | ||
with: | ||
name: css | ||
path: build/css |
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 Vanilla folder. This would need to be cookie-policy folder (dist?).
But overall - I don't think you need to separate this into build
and publish-npm
jobs.
We do it in Vanilla because we also have publish-assets
job.
I think it would be simpler if you have it all in single job, just build/test first, and then attempt publishing to npm.
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.
Good point, I'll merge the two together
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've merged the file, though the CI action failed because the current version was already published. It shouldn't be a problem though.
Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>
|
||
# Only for testing. Should be on: release: types: [published] | ||
on: | ||
- push |
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.
Not sure if we want that yet, but could make sense to simply publish on every merge to main branch?
This would not create a release tag on GH (unless we can make it automated as well), but may be a nice next step in automating this, instead of relying on manual release.
This is the way we have it set up for our python packages that publish to pip, and in react components I think.
In Vanilla we want more manual control, so we tie it to GH release functionality. But here it probably will make it easier for everyone if it's fully automatic.
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.
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 agree on the automation, and there should a way to create the release tag. In fact, we could
- create the tag
- create the release on GH
- publish to npm
on merge to main.
I think this works for now though, and I can make another PR to set up the merge to main actions.
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.
LGTM.
Just make sure to update it to correct release event.
Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>
Done