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

Added github actions for performing releases #713

Merged
merged 7 commits into from
May 5, 2021

Conversation

UebelAndre
Copy link
Collaborator

This PR adds a workflow for performing releases.

To run the workflow/perform a release, maintainers can manually trigger it
Screen Shot 2021-04-28 at 11 16 33 PM

This then kicks off some builds which produce build artifacts associated with a release candidate pre-release
Screen Shot 2021-04-28 at 11 24 46 PM
Screen Shot 2021-04-28 at 11 31 39 PM

This is done so produced artifacts can be cached and reviewed before the final release is cut.

To finish the release, the pull request that's created just needs to be merged
Screen Shot 2021-04-28 at 11 25 22 PM

This then runs the Release Finalize workflow which fetches artifacts from the latest release candidate and uploads them to the new, official release
Screen Shot 2021-04-28 at 11 26 49 PM

Maintainers should then add the final release notes to the pre-release and promote it to a full release to conclude this process.

@google-cla google-cla bot added the cla: yes label Apr 29, 2021
@UebelAndre
Copy link
Collaborator Author

There's surely many other things we could do improve the release process but I think it'd be in our best interest incrementally improve the process then try and get everything perfect on the first pass.

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

This generally looks like a reasonable shape/workflow to me. We should get all maintainers' views before merging, though :)

A couple of things it would be nice to add soon to the text generated for the release:

  • An actual copy-and-paste-able snippet to put in your WORKSPACE to pull in rules_rust with URLs and sha256s pre-filled.
  • The minimum supported version of Bazel.

And before actually doing a release, we should work out our compatibility policies (in terms of breaking changes, bazel versions supported, and rust versions supported)

URL_PREFIX: https://github.com/${{ github.repository_owner }}/rules_rust/releases/download/${{ github.event.inputs.version }}
- run: |
# Update docs for release
SKIP_COMMIT=1 ${{ github.workspace }}/docs/update_docs.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the effect of this, if we're not committing the output anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This gets included in the PR that's created at the end of the file. I just wanted to run this script without committing so the PR only contained 1 commit. I almost didn't include this change because of #695 which would make this not necessary but I did just to be safe.

- run: |
# Write new releases file
cat << EOF > ${{ github.workspace }}/version.bzl
"""The version of rules_rust."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a note to this generated file that merging any changes to this file will automatically trigger a release, and that it should never be manually edited? That's non-obvious at the moment from the file, and feels important, lest someone accidentally release by reformatting or adding a comment or similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was looking for an automated way to know that a release just occurred so we can create a new release and upload all the built artifacts to it. Otherwise it either becomes a two manual step process or a release is made before the state of the repo is updated to reflect that release.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thank you. It's a smart solution. If it turns out it is too smart for us we can change it in the future :) Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

I'd +1 Daniel's suggestion to add a comment into the file so uninitiated people understand that they should not modify the file by hand.

Copy link
Member

Choose a reason for hiding this comment

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

(Adding the warning text into version.bzl is uncontroversial even in this PR, wdyt?)

Sorry, what text? I think I missed something 😅

I'm happy to still make updates but I don't want to go down a rabbit hole of generating docs and release notes. I'd much rather do that as a separate PR.

Hopefully this link really points where I hope it does: #713 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've moved the template for version.bzl out into it's own file and it now has a note about the impacts of updating it.

.github/workflows/release_finish.yaml Outdated Show resolved Hide resolved
popd &> /dev/null
&& chmod 0644 *.md

if [[ -z "${SKIP_COMMIT:-}" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

FTR, I'd be fine with this script not doing the commit at all. In fact I'd slightly prefer not committing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That should be handled by #695 and not this PR. I just want this to not create a separate commit but the changes here would still be added to a commit when the release PR is made.

.github/workflows/release_finish.yaml Show resolved Hide resolved
@UebelAndre
Copy link
Collaborator Author

This generally looks like a reasonable shape/workflow to me. We should get all maintainers' views before merging, though :)

Yeah, my intention was not to alienate anyone but to open a PR and go to sleep 😄

* An actual copy-and-paste-able snippet to put in your WORKSPACE to pull in `rules_rust` with URLs and sha256s pre-filled.

Agreed, my hope was to do this in a separate PR though. I figured we'd want to discuss what information goes into a release before codifying it. I think we could cut one release with manually gathered info and then work out ways of automating that.

And before actually doing a release, we should work out our compatibility policies (in terms of breaking changes, bazel versions supported, and rust versions supported)

Yeah, this is still an unanswered question on #415 which maybe @hlopko would be able to kick off? 😅 🙏

@illicitonion
Copy link
Collaborator

This generally looks like a reasonable shape/workflow to me. We should get all maintainers' views before merging, though :)

Yeah, my intention was not to alienate anyone but to open a PR and go to sleep 😄

* An actual copy-and-paste-able snippet to put in your WORKSPACE to pull in `rules_rust` with URLs and sha256s pre-filled.

Agreed, my hope was to do this in a separate PR though. I figured we'd want to discuss what information goes into a release before codifying it. I think we could cut one release with manually gathered info and then work out ways of automating that.

And before actually doing a release, we should work out our compatibility policies (in terms of breaking changes, bazel versions supported, and rust versions supported)

Yeah, this is still an unanswered question on #415 which maybe @hlopko would be able to kick off? 😅 🙏

All sounds good to me!

Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

Assuming all comments are addressed this PR looks good to me from the technical point of view. Would you mind either addressing Daniel's suggestion (to generate a WORKSPACE file snippet in the release and also adding the min supported version of Bazel) in this PR, or if you're not going to work on it right away in a followup, creating an issue for that task?

I'm fine with submitting this PR now, and figuring out the policies afterwards. I think we are not ready for 1.0 (and it will require a dedicated effort to become ready, and I don't have capacity for that now), but I'm fine with 0.X releases as needed.

@hlopko
Copy link
Member

hlopko commented Apr 30, 2021

Just noticed there is a WORKSPACE snippet in our docs: https://bazelbuild.github.io/rules_rust/#setup. I'd make it a high priority to make it be correct.

@UebelAndre
Copy link
Collaborator Author

Assuming all comments are addressed this PR looks good to me from the technical point of view. Would you mind either addressing Daniel's suggestion (to generate a WORKSPACE file snippet in the release and also adding the min supported version of Bazel) in this PR, or if you're not going to work on it right away in a followup, creating an issue for that task?

Yeah, I'd rather do anything related to generating docs/release notes in a separate PR. I think it'd be better to go through a release once before codifying docs and release notes.

I'm fine with submitting this PR now, and figuring out the policies afterwards. I think we are not ready for 1.0 (and it will require a dedicated effort to become ready, and I don't have capacity for that now), but I'm fine with 0.X releases as needed.

This PR is written in such a way that we releases can be decided at a later time. We have total freedom in choosing the appropriate release number.

@UebelAndre
Copy link
Collaborator Author

I'll wait for @illicitonion and @dfreese to approve before merging and creating any spin-off issues.

@hlopko
Copy link
Member

hlopko commented May 4, 2021

(Adding the warning text into version.bzl is uncontroversial even in this PR, wdyt?)

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented May 4, 2021

(Adding the warning text into version.bzl is uncontroversial even in this PR, wdyt?)

Sorry, what text? I think I missed something 😅

I'm happy to still make updates but I don't want to go down a rabbit hole of generating docs and release notes. I'd much rather do that as a separate PR.

@UebelAndre UebelAndre requested a review from hlopko May 4, 2021 15:36
@UebelAndre
Copy link
Collaborator Author

@dfreese it would mean the world to me to get your approval on this too 😄

@UebelAndre UebelAndre merged commit 2788357 into bazelbuild:main May 5, 2021
@UebelAndre UebelAndre deleted the releases branch May 13, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants