Buf CLI migrate to v2 #2691
Buf CLI migrate to v2 #2691
Conversation
Signed-off-by: Kosuke Morimoto <ksk@vdaas.org>
📝 WalkthroughWalkthroughThe pull request includes significant updates to several configuration and Rust source files. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
area/discoverer
area/manager/index
area/filter/ingress
area/filter/egress
area/agent/sidecar
area/tools/cli/loadtest
area/gateway/lb
area/gateway/filter
area/agent/core/faiss
area/agent/core/ngt
area/gateway/mirror
area/index/job/correction
area/index/job/creation
area/index/job/save
area/index/job/readreplica
labels
Deploying vald with
|
| Latest commit: |
aef781d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1d6b93ea.vald.pages.dev |
| Branch Preview URL: | https://feature-buf-migrate-to-v2.vald.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
buf.yaml (2)
16-21: LGTM! Version update and module configuration look good.The update to Buf v2 and the addition of the 'modules' section align well with the PR objectives. The dependencies on
protovalidateandgoogleapisare appropriate for most Protocol Buffer projects.Consider adding a brief comment above the
depssection to explain why these specific dependencies are needed for your project. This can help future maintainers understand the purpose of these dependencies.
16-34: Overall, the migration to Buf v2 looks good, but some clarifications are needed.The update to Buf v2 and the new configurations align well with best practices for Protocol Buffer projects. The use of standard lint rules and stricter breaking change detection will likely improve code quality and maintain better backward compatibility.
However, the introduced exceptions in both the lint and breaking change sections need further explanation. While they may be necessary for your specific use case, it's important to ensure they don't inadvertently introduce risks or reduce the effectiveness of the Buf tool.
Consider documenting the reasons for these exceptions in a comment within the
buf.yamlfile or in a separate documentation file. This will help future maintainers understand the rationale behind these configuration choices and make it easier to reassess them as the project evolves.buf.gen.yaml (1)
16-47: Summary: Successful migration to Buf v2 with expanded functionalityThe changes in this file successfully migrate the Buf configuration to version 2, aligning perfectly with the PR objective. Key improvements include:
- Updated version declaration
- Refined package management configuration
- Modernized plugin declarations for Go, documentation, and Rust
- Addition of Rust support via Prost and Tonic plugins
These changes should lead to improved maintainability and expanded functionality. The migration appears to be comprehensive and well-executed.
As the project now supports multiple languages (Go and Rust), consider implementing a CI step to ensure that generated code for all supported languages remains in sync with the proto definitions. This could help catch any cross-language inconsistencies early in the development process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (20)
apis/grpc/v1/agent/core/agent.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/agent/sidecar/sidecar.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/discoverer/discoverer.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/filter/egress/egress_filter.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/filter/ingress/ingress_filter.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/meta/meta.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/mirror/mirror.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/rpc/errdetails/error_details.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/filter.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/flush.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/index.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/insert.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/object.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/remove.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/search.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/update.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/upsert.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/proto/buf.lockis excluded by!**/*.lockbuf.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
- buf.gen.yaml (1 hunks)
- buf.work.yaml (0 hunks)
- buf.yaml (1 hunks)
- rust/libs/proto/src/filter.egress.v1.tonic.rs (0 hunks)
- rust/libs/proto/src/filter.ingress.v1.tonic.rs (0 hunks)
- rust/libs/proto/src/meta.v1.tonic.rs (0 hunks)
- rust/libs/proto/src/vald.v1.tonic.rs (0 hunks)
💤 Files with no reviewable changes (5)
- buf.work.yaml
- rust/libs/proto/src/filter.egress.v1.tonic.rs
- rust/libs/proto/src/filter.ingress.v1.tonic.rs
- rust/libs/proto/src/meta.v1.tonic.rs
- rust/libs/proto/src/vald.v1.tonic.rs
🧰 Additional context used
🔇 Additional comments (5)
buf.yaml (2)
29-34: Breaking change configuration looks reasonable, but please clarify the exceptions.The continued use of 'FILE' for breaking change detection is a good practice. However, I have a question about the exceptions:
Could you please explain why the exceptions for 'EXTENSION_NO_DELETE' and 'FIELD_SAME_DEFAULT' are necessary? These rules often help maintain backward compatibility, so it would be helpful to understand the reasoning behind disabling them.
To verify the impact of these exceptions, let's run the following script:
#!/bin/bash # Description: Check for potential issues related to the disabled breaking change rules # Check for extensions in proto files echo "Checking for extensions in proto files:" rg --type proto "extend" apis/proto # Check for fields with default values echo "Checking for fields with default values:" rg --type proto "default =" apis/protoThis script will help us understand the usage of extensions and default field values in the proto files, which might justify the exceptions.
22-28: Lint configuration looks good, but please clarify the exceptions.The update to 'STANDARD' lint rules and disallowing comment ignores are excellent practices for maintaining code quality. However, I have a question about the exceptions:
Could you please explain why the exceptions for 'FIELD_NOT_REQUIRED' and 'PACKAGE_NO_IMPORT_CYCLE' are necessary? These rules often catch important issues, so it would be helpful to understand the reasoning behind disabling them.
To verify the impact of these exceptions, let's run the following script:
This script will help us understand the prevalence of optional fields and import statements in the proto files, which might justify the exceptions.
buf.gen.yaml (3)
18-23: Approved: Refined package management configurationThe restructuring of the managed section provides more precise control over package management. The disable option for 'go_package' and the override for 'go_package_prefix' should lead to more consistent and controllable package naming across the project.
To ensure these changes don't negatively impact existing imports, please run:
#!/bin/bash # Check for any import issues after package management changes go mod tidy go build ./...
36-47: Approved: Updated configurations for documentation and Rust pluginsThe plugin configurations for documentation (Markdown and OpenAPI) and Rust (Prost and Tonic) have been successfully updated to the new format. The addition of Rust plugins is a significant enhancement, expanding the project's language support.
To ensure the Rust-related changes are working as expected, please run:
#!/bin/bash # Verify Rust code generation buf generate find rust/libs/proto/src -type f -name "*.rs" | sort
16-16: Approved: Version update to v2The update to version 2 of the Buf configuration is in line with the PR objective. This change is crucial for leveraging the latest features and improvements in Buf.
To ensure all parts of the project are compatible with Buf v2, please run the following verification:
kpango
requested review from
a team,
kpango and
hlts2
and removed request for
a team
* migrate to v2 Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> * replace Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> * format --------- Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> Co-authored-by: Yusuke Kato <kpango@vdaas.org>
* migrate to v2 Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> * replace Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> * format --------- Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> Co-authored-by: Yusuke Kato <kpango@vdaas.org>
* migrate to v2 Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> * replace Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> * format --------- Signed-off-by: Kosuke Morimoto <ksk@vdaas.org> Co-authored-by: Yusuke Kato <kpango@vdaas.org>
Description
migrate to buf v2 and regenerate proto files.
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
v2, enhancing clarity and structure.search_by_id,stream_search_by_id, andmulti_search_by_id.buf.work.yamlfile as part of the configuration cleanup.