-
Notifications
You must be signed in to change notification settings - Fork 691
Conversation
this is my feature
this is another feature
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
.github/workflows/release.yml
Outdated
name = "io_bazel_rules_docker", | ||
sha256 = "${{ steps.get_sha.outputs.sha }}", | ||
strip_prefix = "rules_docker-${{ env.VERSION }}", | ||
urls = ["https://github.com/bazelbuild/rules_docker/releases/download/${{ steps.get_tag.outputs.TAG }}/rules_docker-${{ steps.get_tag.outputs.TAG }}.tar.gz"], |
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.
urls = ["https://github.com/bazelbuild/rules_docker/releases/download/${{ steps.get_tag.outputs.TAG }}/rules_docker-${{ steps.get_tag.outputs.TAG }}.tar.gz"], | |
urls = ["https://github.com/${{ github.repository_owner }}/${{ github.event.repository.name }}/releases/download/${{ steps.get_tag.outputs.TAG }}/rules_docker-${{ steps.get_tag.outputs.TAG }}.tar.gz"], |
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.
Makes sense, updated to be more dynamic. The generated release information works automatically for forks now as-well.
Thank you for the contribution! Looks good to me, bar one comment. |
.github/workflows/release.yml
Outdated
http_archive( | ||
name = "io_bazel_rules_docker", | ||
sha256 = "${{ steps.get_sha.outputs.sha }}", | ||
strip_prefix = "rules_docker-${{ env.VERSION }}", |
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 we may be able to remove the need for the strip_prefix?
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.
Thank you @uhthomas, I actually think I prefer to leave it in so we don't need to extract/recpompress things as that feels a little unnatural. However, if we want to remove it at a later stage we would just need to:
- After https://github.com/bazelbuild/rules_docker/pull/2106/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R21 is downloaded, extract the contents.
- Move into the top level directory & compress the contents of that instead of the parent directory.
- Use this new tar.gz instead of the other one for generating SHA, uploading etc.
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.
Hmm.
We could use git archive?
- uses: actions/checkout@v3
- run: git archive --output=rules_docker-${{ steps.get_tag.outputs.TAG }}.tar.gz ${{ steps.get_tag.outputs.TAG }}
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.
Even better, we could make tar.zst
archives. Smaller and faster to compress/decompress. Though this would require Bazel 5.2.0. Maybe both? I guess we can do this in a follow up PR. Just wanted to write this down somewhere.
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.
Yeah doing that in a follow up makes sense. I can probably look at it, but won't get a chance this week. Could be good to try what we have here so far with the next release anyway.
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.
Are you happy to try the git archive thing? It would be really nice to remove the requirement for strip_prefix
as most other rulesets don't for major releases. If you don't have time, I'm happy to add a commit?
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 can probably get to this next week but if you'd like to try it in the meantime, feel free to add a commit. Otherwise, I'll try next week.
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.
@uhthomas actually had some time this evening & pushed the change so strip_prefix
isn't needed. Your suggestion worked. Basically, the extracted parent dir just needs to match the name of the tar & then strip_prefix
isn't needed so git archive
works a treat. Thank you!
Whoever is releasing using this, it should just be a case of doing this from master:
|
Damn, didn't mean to rebase and merge. Should have squashed. Thanks again @kriscfoster, will make a release later. |
Here's a demo.
Here's an example release.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Automatic release process with GitHub actions.
What is the current behavior?
Issue Number: #2102
What is the new behavior?
On a tag push, the workflow will be triggered & release will be created.
Does this PR introduce a breaking change?
Other information