-
Notifications
You must be signed in to change notification settings - Fork 10
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
tie xmtpd to specific proto versions #481
Conversation
WalkthroughThe change updates the bash script used to generate protobuf definitions. The script now retrieves the definitions using a fixed commit reference ( Changes
Sequence Diagram(s)sequenceDiagram
participant Script as "gen-protos Script"
participant Repo as "Repository"
participant Generator as "Protobuf Generator"
Script->>Repo: Request definitions using commit ref=08c80536d56e73c6f585d80e7cd2de3db6733c8b
Repo-->>Script: Return protobuf definitions from specific commit
Script->>Generator: Initiate protobuf generation with fetched definitions
Generator-->>Script: Generation process completed
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dev/gen-protos (1)
4-7
: Great improvement for build determinism!Using a specific commit hash instead of
branch=main
ensures reproducible builds, which aligns perfectly with the PR objectives.Let's verify the commit hash and its recency:
Consider externalizing the commit hash to a configuration file or environment variable for easier updates. This would make it simpler to update the proto version when needed. For example:
-if ! buf generate https://github.com/xmtp/proto.git#subdir=proto,ref=08c80536d56e73c6f585d80e7cd2de3db6733c8b; then +PROTO_VERSION="08c80536d56e73c6f585d80e7cd2de3db6733c8b" +if ! buf generate "https://github.com/xmtp/proto.git#subdir=proto,ref=${PROTO_VERSION}"; then✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the commit hash exists and check its age # to ensure we're using an appropriate version # Check if the commit exists and get its details gh api \ -H "Accept: application/vnd.github+json" \ repos/xmtp/proto/commits/08c80536d56e73c6f585d80e7cd2de3db6733c8b \ --jq '.commit.author.date, .commit.message' # Get latest tags for reference gh api \ -H "Accept: application/vnd.github+json" \ repos/xmtp/proto/tags \ --jq '.[0:3] | .[] | {name: .name, date: .commit.commit.author.date}'Length of output: 522
Build determinism looks solid!
- The commit hash
08c80536d56e73c6f585d80e7cd2de3db6733c8b
is valid—it exists with a commit date of2025-02-04T14:04:01Z
and the expected commit message.- Consider externalizing the commit hash (e.g., via a configuration file or environment variable) for easier future updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dev/gen-protos
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (1)
dev/gen-protos (1)
3-3
: LGTM! Comprehensive cleanup of generated files.The cleanup command properly removes all types of generated protobuf files to prevent any stale definitions from persisting.
We don't autogenerate them. It's either a manual step or a nightly automated action. What is your goal with this change? |
Currently we generate the protos based on the main branch.
Instead of doing this, and to help with reproducible deterministic build each xmtpd tag should point to the latest supported tag in xmtp/proto.
Suggestion:
Summary by CodeRabbit