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

feat: add release action #123

Merged
merged 4 commits into from Dec 20, 2021
Merged

feat: add release action #123

merged 4 commits into from Dec 20, 2021

Conversation

nuragic
Copy link
Contributor

@nuragic nuragic commented Dec 7, 2021

Close #112.

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

perfect. please also link the issue so that it's closed when we merge this PR

@nuragic
Copy link
Contributor Author

nuragic commented Dec 7, 2021

@simoneb I've linked the issue in the commit description

Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

we can't use this action yet, more details to follow

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

I'm blocking this PR because I just recalled that there's something that the release automation action doesn't do for us and we need to decide how to handle it, or this won't do what we expect.

This action has a build npm script. This script needs to be executed in order to generate the dist folder which is what is actually being executed when the action runs.

The release automation action does npm install (this thing has a problem in itself because it runs on whatever Node.js version the action runs on, NOT what we would like it to run on, but that's another issue), but it runs no other scripts. Without running the build script, the published released will not contain the changes it would need to contain.

I see 2 options here:

  • we enhance the release action to run additional scripts (action-side approach)
  • we configure this repo to run the build script on a npm lifecycle script, e.g. prepare or postinstall or whatever makes sense (user-side approach)

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

Opened an issue about this in nearform-actions/optic-release-automation-action#27

@Eomm
Copy link
Member

Eomm commented Dec 7, 2021

we enhance the release action to run additional scripts (action-side approach)

Just to share that, by default, the GitHub nodejs action template provides you this workflow to check the dist build:

https://github.com/Eomm/why-don-t-you-tweet/blob/main/.github/workflows/check-dist.yml

This is a reminder to build the source code actually.

@nuragic
Copy link
Contributor Author

nuragic commented Dec 7, 2021

Great, thanks for the feedback! Given the above, I think it would make sense to go with:

we enhance the release action to run additional scripts (action-side approach)

Just a question aside, why we compile everything via ncc?

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

Just a question aside, why we compile everything via ncc?

The action needs to be self contained, so either we commit node_modules or we bundle it. This second approach is also what's recommended in https://docs.github.com/en/actions/creating-actions/creating-a-javascript-action#commit-tag-and-push-your-action-to-github.

In fact, we should probably be doing the same for the release action! I'll create an issue.

@nuragic
Copy link
Contributor Author

nuragic commented Dec 7, 2021

Oh thanks for the link!

Checking in your node_modules directory can cause problems. As an alternative, you can use a tool called @vercel/ncc to compile your code and modules into one file used for distribution.

👍

@simoneb
Copy link
Collaborator

simoneb commented Dec 8, 2021

There's one more thing that we have to do before we go ahead with this PR. We first need to release a new version capable of automerging semver-like. So we should keep doing manual releases until #124 is released.

@nuragic
Copy link
Contributor Author

nuragic commented Dec 9, 2021

@simoneb now that #124 is released, should we unblock this PR?

@simoneb
Copy link
Collaborator

simoneb commented Dec 9, 2021

@nuragic no but thanks for bringing it up because the dependency was not explicit. We also depend on nearform-actions/optic-release-automation-action#27

@simoneb
Copy link
Collaborator

simoneb commented Dec 10, 2021

@nuragic @Eomm is there a chance that in fact we're already good here?

See https://github.com/fastify/github-action-merge-dependabot/blob/main/.husky/pre-commit

@Eomm
Copy link
Member

Eomm commented Dec 10, 2021

I don't think we have done here: the local hooks can be skipped and a malicious user may manipulate the dist/ build (who is reading that file)

When we will release this PR nearform-actions/optic-release-automation-action#30 we are ready to go here.
The actual optic-release-automation-action version runs the actions/checkout@v2.4.0 step every time overwriting whatever build the user made before.. so those PR's changes are necessary to let the GHA environment build a secure dist/ payload

.github/workflows/release.yml Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Show resolved Hide resolved
nuragic and others added 3 commits December 16, 2021 11:04
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
@Eomm Eomm requested a review from simoneb December 16, 2021 10:12
@simoneb simoneb merged commit 5dc0db2 into fastify:main Dec 20, 2021
@github-actions github-actions bot mentioned this pull request Dec 27, 2021
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.

Provide major (and minor) tags
3 participants