ci: support releasing proto definitions to BSR #274
Conversation
|
Warning Rate limit exceeded@rach-id has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 55 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a new GitHub Actions workflow file, Changes
Poem
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 (
|
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #274 +/- ##
=======================================
Coverage 68.01% 68.01%
=======================================
Files 6 6
Lines 1138 1138
=======================================
Hits 774 774
Misses 337 337
Partials 27 27 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
.github/workflows/buf-ci.yaml (2)
1-12: Workflow name and trigger events look good, but consider removing the delete event trigger.The workflow name "Buf CI" is clear and descriptive. The trigger events appropriately cover pushes to the main branch, version tags, and pull requests. However, the inclusion of the delete event (line 12) is unusual for a CI workflow and may not be necessary.
Consider removing the delete event trigger:
pull_request: - delete:
22-30: Buf action configuration is well-structured, but comments could be improved.The configuration for the Buf action is appropriate:
- Uses a secret token for secure authentication.
- Clearly specifies the input directory.
- Sets up breaking change checks against the main branch.
- Optimizes the workflow by conditionally running checks only on push events.
Consider cleaning up the comments on lines 26-27 by formatting them as proper YAML comments:
- # Run breaking change, lint, and format checks for Protobuf sources against all branches, - 'pb' # subdirectory, then push to the BSR once validated + # Run breaking change, lint, and format checks for Protobuf sources against all branches, + # then push to the BSR once validated
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/buf-ci.yaml (1 hunks)
- buf.yaml (1 hunks)
Additional comments not posted (4)
buf.yaml (1)
1-4: LGTM! Thebuf.yamlconfiguration looks correct.The configuration is well-structured and aligns with the PR objective of supporting the release of proto definitions to BSR (Binary Serialization Repository). It correctly specifies the Buf version and defines a module with the appropriate path and name.
Consider adding a comment at the top of the file to explain its purpose, for example:
# Configuration for Buf (https://buf.build) to manage and publish Protocol Buffer definitionsThis will improve maintainability and help other developers understand the file's role in the project.
Let's verify if there are any
.protofiles in the repository that this configuration will apply to:Verification successful
Verification Successful:
.protofiles are present in the repository.The
buf.yamlconfiguration correctly specifies the module path and is set up to manage and publish Protocol Buffer definitions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the existence of .proto files in the repository # Test: Search for .proto files fd -e proto # If no .proto files are found, it might indicate a potential issue or missing files if [ $? -ne 0 ]; then echo "No .proto files found in the repository. Please verify if this is intended." fiLength of output: 46
.github/workflows/buf-ci.yaml (3)
13-15: Permissions are well-defined and follow the principle of least privilege.The permissions are appropriately set:
- Read access for contents is sufficient for most CI operations.
- Write access for pull-requests allows the workflow to update PR status and comments.
This configuration ensures the workflow has the necessary permissions without over-privileging.
16-21: Job definition and steps are well-structured and use appropriate actions.The job definition is clear and concise:
- Uses the latest Ubuntu runner for compatibility.
- Employs the latest major version of the checkout action (v4).
- Utilizes a specific major version of the buf-action (v1) for stability.
This setup provides a good balance between using up-to-date tools and maintaining stability.
1-30: Overall, the Buf CI workflow is well-designed and follows best practices.The workflow effectively sets up a CI pipeline for Protocol Buffers using Buf. It covers essential aspects such as:
- Appropriate trigger events
- Least privilege permissions
- Proper job and step definitions
- Secure and optimized Buf action configuration
The minor suggestions provided earlier (removing the delete event trigger and cleaning up comments) would further refine this already solid workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
buf.yaml (3)
1-4: LGTM! Consider adding a brief comment for clarity.The version and module configuration look good. The use of version "v2" is appropriate, and the module naming follows a standard convention.
Consider adding a brief comment above the
modulessection to explain the purpose of this configuration, e.g.:# Define the Protocol Buffer module for the NMT (Namespaced Merkle Tree) project modules: - path: pb name: buf.build/celestia/nmt
5-12: Linting exceptions noted. Consider creating follow-up tasks.The linting configuration is ignoring two rules for
pb/proof.prototo avoid breaking changes. While this is understandable, it's important to address these issues in the future.Consider the following suggestions:
- Create GitHub issues to track these linting exceptions.
- Add TODO comments in the
pb/proof.protofile to highlight these exceptions.- Plan for addressing these in the next major version update when breaking changes are acceptable.
This will ensure that these exceptions are not forgotten and can be addressed at an appropriate time.
1-12: Good addition for Protocol Buffer management. Consider documentation updates.The addition of
buf.yamlis a positive step towards better management of Protocol Buffers in the project. It sets up a clear structure for the module and acknowledges current limitations with a plan to address them in the future.To fully leverage this addition:
- Update the project's README or documentation to mention the use of Buf for Protocol Buffer management.
- If not already in place, consider adding a section in the contributing guidelines about working with Protocol Buffers and using the Buf tool.
- Ensure that the CI/CD pipeline is updated to use this configuration for validating Protocol Buffer changes.
These steps will help team members and contributors understand and effectively use this new configuration.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/buf-ci.yaml (1 hunks)
- buf.yaml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/buf-ci.yaml
it should be tested after the PR is merged |
Co-authored-by: Rootul P <[email protected]>
|
The latest Buf updates on your PR. Results from workflow buf-ci / buf (pull_request).
|
| branches: | ||
| - main | ||
| tags: | ||
| - "v[0-9]+.[0-9]+.[0-9]+" |
There was a problem hiding this comment.
unless you are creating a bunch of tags that start with v that aren't version tags, I think you can simplify this to just v*.
There was a problem hiding this comment.
I just followed the same tags we're using in all other repos. and I think it's better since we know the exact tags formats we target
For GH actions that can't be tested on the PR, because they require being on main to be tested, I recommend either using a fork or having a personal repo like I have a |
|
I tried running this in a personal repo but I keep hitting certain upload permission errors. I will merge this one, test it. if it works, there is no need to waste more time on it. If not, I will fix in a follow up PR |
Adds support for pushing the proto definitions to BSR
Summary by CodeRabbit
New Features
Documentation