-
Notifications
You must be signed in to change notification settings - Fork 67
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
[Requires repository secret addition] Add Publish to Buf Github Action #508
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.
I know this PR isn't marked "ready for review" yet, but had a couple of comments
buf.yaml
Outdated
@@ -1,4 +1,5 @@ | |||
version: v1 | |||
name: buf.build/temporal/api |
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.
Note, with basically every SDK language, package manager, GH org, Docker hub, etc, etc we have used temporalio
. Do we want to consider that here instead of temporal
(ignoring the fact that there are already registered repositories on temporal
). It seems that temporalio
org was created, do we own it?
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.
Updated the reference to temporalio
and brought it up on Slack 🤞
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.
Would you like me to contact [email protected] to see if we can settle the ownership?
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.
Update: we own it and will have access shortly.
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.
Separate from this PR (so doesn't need to block this one), can we also move the cloud API in there and stop using the temporal
org?
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.
Draft PR for cloud API: https://github.com/temporalio/api-cloud/pull/56/files
.github/workflows/push-to-buf.yml
Outdated
on: | ||
push: | ||
branches: | ||
- master |
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.
Pardon my lack of buf
knowledge, can I get some clarity on how versioning is handled? This isn't doing anything to buf when we make a new tag in this repository, should it?
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 added a new tags
trigger and successfully tagged my personal buf registry:
This way other repositories can depend on a specific release:
https://buf.build/docs/bsr/module/dependency-management/
code: https://github.com/nikki-dag/api
repo: https://buf.build/nikki/api
actions run: https://github.com/nikki-dag/api/actions/runs/12662329218/job/35287043167
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.
Should we test this before merging? I would like to confirm protos under google/
aren't published. You can request internally for the secret to be set (it should be set GH org wide, not just in this repo IMO). You may need to temporarily enable this to push on PR to test.
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.
Sounds good 👍
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.
Very good point.
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.
Tested, LGTM
.github/workflows/push-to-buf.yml
Outdated
tags: | ||
- '**' |
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.
Do we want this for every tag? Should it be only v**
tags?
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.
Updated ✅
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.
Very good point.
READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.
What changed?
Adds a GitHub Actions workflow to automate the process of pushing protocol buffers to the Buf registry on every push to the main branch.
We need to set the
BUF_TOKEN
secret before merging this PR.Why?
We want to be able to depend on server protos from other repositories (e.g. api-cloud) using the buf package manager.
Breaking changes
None.
Server PR
NA