-
Notifications
You must be signed in to change notification settings - Fork 120
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
Fix go bindings generation #182
Fix go bindings generation #182
Conversation
Followup to bazelbuild#154, which forgot to update the bazel configuration. This makes hooks/pre-commit work again.
Why doesn’t CI also enforce this? |
This repository doesn't actually have CI. I could add a simple github action with tests though. |
|
||
jobs: | ||
build: | ||
runs-on: ubuntu-latest |
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.
You could add:
container: docker://l.gcr.io/google/bazel:<some version number>
That way the action runs on a container that has Bazel immediately present, meaning that you don't need to install Bazelisk.
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.
Those container images seem to lag behind the released bazel versions more than bazelisk, the latest version there is 3.5.0 while the latest bazel release is 3.7.1
https://console.cloud.google.com/gcr/images/cloud-marketplace-containers/GLOBAL/google/bazel?gcrImageListsize=30
Not that it matters too much either way.
.github/workflows/main.yml
Outdated
run: | | ||
set -e | ||
PATH=${HOME}/bin:$PATH ./hooks/pre-commit | ||
# This might stage some files, if the bindings need to be regenerated. |
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.
You could add a step that runs git diff --exit-code HEAD --
.
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.
If we do this, then the check will fail on all PRs that haven't committed the updated go bindings- I'm not sure that we want to do that (in which case, maybe it would be better to git commit --amend --no-edit
).
trigger check on PRs too
Weren't we going to drop the bindings completely as per #177? Not sure if we want to go the opposite direction with CI? |
This github action doesn't commit the changes- it just runs the hook to confirm that it works. (I think it's worth keeping the go bindings generators in this repository, even if we don't host the generated code. It saves people from having to figure out how to do it themselves.) |
drop unneeded git config (since we aren't going to commit 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.
As per the Dec 8th meeting.
It looks like the github action confirmed that the bindings generation works again: |
* Fix go bindings generation Followup to bazelbuild#154, which forgot to update the bazel configuration. This makes hooks/pre-commit work again. * Add a simple CI check that bindings generation works
Followup to #154, which forgot to update the bazel configuration.
This makes hooks/pre-commit work again, and adds a simple github action to confirm this still works. If we wanted to test the check, we could land the check first, a test PR to verify that the check fails, then update this PR to trigger a check and confirm that it succeeds.