-
Notifications
You must be signed in to change notification settings - Fork 83
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
Binary release GitHub action #133
Conversation
This looks good to me but it would be nice to have Bret's +1 before merging since he has more context on this particular issue (I think he manually deployed these binaries before? not sure) |
Yup, you're correct @joao-r-reis ; I've been doing that upload manually so far. Review on it's way! |
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.
A few suggestions to make this a bit cleaner and remove the code duplication
.github/workflows/release-binary.yml
Outdated
- name: Build Linux/amd64 binary | ||
run: | | ||
export GO111MODULE=on | ||
export CGO_ENABLED=0 |
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.
Unless I'm missing something GO111MODULE and CGO_ENABLED are both constant for all these options. You can define these env vars at the top level using an "env" block. We do something like that in the python-driver-wheel actions here.
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.
Fixed.
.github/workflows/release-binary.yml
Outdated
export GOOS=linux | ||
export GOARCH=arm64 | ||
go build -o cql-proxy | ||
tar cvfz cql-proxy-linux-arm64-${{ github.ref_name }}.tgz cql-proxy LICENSE |
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's a lot of repetition of "go build" and "tar" commands here. You should be able to define these operations once by using a strategy to specify which platforms/processors you wish to build for; the blocks in that strategy will then be applied for each operation. Docs on strategies can be found here; there's also a decent example in the python-driver-wheels 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.
Fixed.
.github/workflows/release-binary.yml
Outdated
sha256sum cql-proxy-windows-amd64-${{ github.ref_name }}.zip | cut -d ' ' -f 1 > cql-proxy-windows-amd64-${{ github.ref_name }}-sha256.txt | ||
sha256sum cql-proxy-darwin-amd64-${{ github.ref_name }}.tgz | cut -d ' ' -f 1 > cql-proxy-darwin-amd64-${{ github.ref_name }}-sha256.txt | ||
sha256sum cql-proxy-darwin-arm64-${{ github.ref_name }}.tgz | cut -d ' ' -f 1 > cql-proxy-darwin-arm64-${{ github.ref_name }}-sha256.txt | ||
sha256sum cql-proxy-linux-arm64-${{ github.ref_name }}.tgz | cut -d ' ' -f 1 > cql-proxy-linux-arm64-${{ github.ref_name }}-sha256.txt |
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.
Seems like this should be done after creating the tarballs in the individual steps above. Doing so also mixes nicely with moving to a strategy since it's another thing that you would only need to define once (in the strategy) rather than for each platform.
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.
Fixed.
.github/workflows/release-binary.yml
Outdated
cql-proxy-darwin-arm64-${{ github.ref_name }}.tgz | ||
cql-proxy-darwin-arm64-${{ github.ref_name }}-sha256.txt | ||
cql-proxy-linux-arm64-${{ github.ref_name }}.tgz | ||
cql-proxy-linux-arm64-${{ github.ref_name }}-sha256.txt |
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.
Yuck... it's kinda unpleasant to have to list every option explicitly here. If you create a subdirectory and then create all your tarballs and checksum files in that directory you should just be able to use a single glob here ("with.files: subdir/*" rather than having to list everything out like this. The docs for action-gh-release indicates that globs are supported but obviously you should test that out.
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 I have fixed this part but it is hard to test locally. AFAIK the only way would be to merge the PR and create old tag (e.g. 0.0.1) to see if upload works. We can remove the test release afterwards.
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.
Fixed, tested on my fork and it works.
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.
Yup, with these changes this looks good to me. Nice work @lukasz-antoniak!
Fix #126.