-
Notifications
You must be signed in to change notification settings - Fork 325
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: Adding Release Automation #1122
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.
explainers
release-pr: | ||
runs-on: ubuntu-latest | ||
needs: [test] | ||
if: github.event_name == 'push' && github.ref == 'refs/heads/main' |
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 only needs to run on push
after the tests
have succeeded. Creates a release-pr based on conventional-commits commit strategy (which we already follow)
This defines what a release PR is: https://github.com/google-github-actions/release-please-action#whats-a-release-pr
release-type: node | ||
package-name: ipfs-companion | ||
changelog-notes-type: github | ||
command: release-pr |
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.
- auto-increments the computed semver
- names the package
- creates changelog
- only creates the release PR
This PR can stay around for as long as it's needed, as soon as that gets merged it pushes the release tags.
.github/workflows/ci.yml
Outdated
- uses: actions/download-artifact@v3 | ||
id: download | ||
with: | ||
name: built-on-ubuntu-latest | ||
path: ~/downloads/ |
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 publish artifacts as part of the release process, we don't need to build those again (I am not sure if the download path is reachable) This from one of their examples https://github.com/actions/download-artifact/tree/main#download-path-output
The artifacts uploaded look like this: https://github.com/ipfs/ipfs-companion/actions/runs/3727290259#artifacts
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.
The uploaded artifacts are developer version, we can't use them for release:
ipfs_companion_dev_build_bf88bee_-2.20.1.1059_brave.zip
ipfs_companion_dev_build_bf88bee_-2.20.1.1059_chromium.zip
ipfs_companion_dev_build_bf88bee_-2.20.1.1059_firefox.zip
To build a release artifacts,
you need to run build in the docker with env variable RELEASE_CHANNEL
set to stable
.
We have convenience script npm run release-build
that does reproducible build in Docker and sets proper version and name in manifest.
I see two ways of fixing this.
You want to special-case build happening on tag (if: github.event_name == 'push' && contains(github.ref, 'refs/tags/')
and then either:
- (A) Keep deterministic release build based on Docker and run
npm run release-build
instead ofnpm run ci:build
- (B) Remove Docker, and set
RELEASE_CHANNEL=stable
so whennpm run ci:build
is called it will produce release artifacts, and not dev ones.
I think we could remove Docker and do (B) only if you confirm that the build produces exactly the same artifact on windows/linux/macos. Otherwise we risk rjection by Mozilla's Add-On Store.
.github/workflows/ci.yml
Outdated
files: ${{ steps.download.outputs.download-path }}/*.zip | ||
fail_on_unmatched_files: true | ||
generate_release_notes: true | ||
draft: true | ||
append_body: true | ||
body: 'Automated Release, please upload artifacts to respective webstores and mark this draft as released.' |
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.
Once we have the assets we can now create a draft release, with release notes + fail if the files are not found.
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 all looks legit to me and looks like a significant improvement
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 a very nice quality of life improvement! Thank you @whizzzkid ❤️
Before we merge, mind addressing two issues below?
.github/workflows/ci.yml
Outdated
- uses: actions/download-artifact@v3 | ||
id: download | ||
with: | ||
name: built-on-ubuntu-latest | ||
path: ~/downloads/ |
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.
The uploaded artifacts are developer version, we can't use them for release:
ipfs_companion_dev_build_bf88bee_-2.20.1.1059_brave.zip
ipfs_companion_dev_build_bf88bee_-2.20.1.1059_chromium.zip
ipfs_companion_dev_build_bf88bee_-2.20.1.1059_firefox.zip
To build a release artifacts,
you need to run build in the docker with env variable RELEASE_CHANNEL
set to stable
.
We have convenience script npm run release-build
that does reproducible build in Docker and sets proper version and name in manifest.
I see two ways of fixing this.
You want to special-case build happening on tag (if: github.event_name == 'push' && contains(github.ref, 'refs/tags/')
and then either:
- (A) Keep deterministic release build based on Docker and run
npm run release-build
instead ofnpm run ci:build
- (B) Remove Docker, and set
RELEASE_CHANNEL=stable
so whennpm run ci:build
is called it will produce release artifacts, and not dev ones.
I think we could remove Docker and do (B) only if you confirm that the build produces exactly the same artifact on windows/linux/macos. Otherwise we risk rjection by Mozilla's Add-On Store.
.github/workflows/ci.yml
Outdated
- name: Release | ||
uses: softprops/action-gh-release@v1 | ||
with: | ||
files: ${{ steps.download.outputs.download-path }}/*.zip | ||
fail_on_unmatched_files: true | ||
generate_release_notes: true | ||
draft: true | ||
append_body: true |
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.
built-on-ubuntu-latest.zip
is nice, but having to unpack it every time to get firefox or chrome specific version and then re-upload it to release is a bit tedious.
Mind adding a step that unzips it, so CI attach three ipfs_companion*.zip
to the release, and person making the release does not need to unzip anything?
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.
Ready for re-review @lidel
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.
approving the updates, but a note that I only saw the fix for the release-build comment lidel had. I didn't catch the three separate zips issue he was talking about so leaving that to @lidel to confirm
.github/workflows/ci.yml
Outdated
@@ -110,18 +110,42 @@ jobs: | |||
needs: [test] | |||
if: github.event_name == 'push' && contains(github.ref, 'refs/tags/') | |||
steps: | |||
- uses: actions/checkout@v2 | |||
- name: Check out Git repository | |||
uses: actions/checkout@v1 |
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.
why downgrading to checkout@v1?
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.
Other actions from the actions
namespace also seem to be outdated. I'd recommend updating them all.
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.
.github/workflows/ci.yml
Outdated
- name: Restore node_modules | ||
id: yarn-cache | ||
uses: actions/cache@v2 | ||
with: | ||
path: node_modules | ||
key: ${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} |
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.
just FYI: aegir actually has a great github action that allows us to cache node-modules. we should be using that most places to ensure we're consistent.
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.
https://github.com/ipfs/aegir/blob/master/actions/cache-node-modules/action.yml#L27-L36
that action seems to cache dist
too. In this case we don't need that, it will create a conflict as we build stable version later, we don't want that to be cached. We just need node_modules
.
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.
@whizzzkid thank you!
Small concern around softprops/action-gh-release
described below, but if you switch to specific revision, it is ok to merge.
.github/workflows/ci.yml
Outdated
run: npm run ci:build:stable | ||
|
||
- name: Release | ||
uses: softprops/action-gh-release@v1 |
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.
google-github-actions/release-please-action
here with command
set to github-release
?
Not a blocker, but softprops/action-gh-release
being a personal account of an individual has a higher risk of being compromised, and that could inject code that would ship to our users.
If we have to use it, please pin it to a specific revision before merging this PR:
uses: softprops/action-gh-release@v1 | |
uses: softprops/action-gh-release@de2c0eb89ae2a093876385947365aca7b0e5f844 |
For future reference, remember to add to the safelist at https://github.com/ipfs/ipfs-companion/settings/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.
Thanks for the callout on adding specific version and whitelisting this.
Not a blocker, but softprops/action-gh-release being a personal account of an individual has a higher risk of being compromised, and that could inject code that would ship to our users.
google-github-actions/release-please-action
doesn't seem to have documentation on creating a draft release. It just pushes the release (that's what i tested on other repo).
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.
Looks like https://github.com/google-github-actions/release-please-action/pull/398/files
seems to suggest there is draft: boolean
but it does not support uploading files, which we want, I'll try creating documentation for them.
Relates to: #778
In this PR (Solves part 1 of #778):
draft
release with:QoL improvements: