Skip to content
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

Backport PR #2691 to release/v1.7 for Buf CLI migrate to v2 #2695

Merged

Conversation

vdaas-ci
Copy link
Collaborator

@vdaas-ci vdaas-ci commented Oct 11, 2024

Description

migrate to buf v2 and regenerate proto files.

Related Issue

Versions

  • Vald Version: v1.7.13
  • Go Version: v1.23.2
  • Rust Version: v1.81.0
  • Docker Version: v27.3.1
  • Kubernetes Version: v1.31.1
  • Helm Version: v3.16.1
  • NGT Version: v2.2.4
  • Faiss Version: v1.8.0

Checklist

Special notes for your reviewer

Summary by CodeRabbit

  • New Features
    • Updated configuration files to version v2, enhancing clarity and structure.
    • Introduced new section for modules in buf.yaml, specifying dependencies.
  • Bug Fixes
    • Removed unnecessary comments across various modules for a cleaner codebase.
    • Removed methods multi_search_object and search_by_id from the API, simplifying the interface.
  • Chores
    • Removed the buf.work.yaml file as part of the configuration cleanup.

* migrate to v2

Signed-off-by: Kosuke Morimoto <[email protected]>

* replace

Signed-off-by: Kosuke Morimoto <[email protected]>

* format

---------

Signed-off-by: Kosuke Morimoto <[email protected]>
Co-authored-by: Yusuke Kato <[email protected]>
Copy link

cloudflare-workers-and-pages bot commented Oct 11, 2024

Deploying vald with  Cloudflare Pages  Cloudflare Pages

Latest commit: 52be243
Status: ✅  Deploy successful!
Preview URL: https://dc743e7e.vald.pages.dev
Branch Preview URL: https://backport-release-v1-7-featur-of8a.vald.pages.dev

View logs

Copy link
Contributor

coderabbitai bot commented Oct 11, 2024

📝 Walkthrough

Walkthrough

The pull request includes significant updates to several configuration files and Rust modules related to the Buf tool and gRPC services. The buf.gen.yaml and buf.yaml files have been updated to version 2, with changes in structure and content, including the removal of the buf.work.yaml file. Additionally, documentation comments have been removed from various Rust modules, simplifying the code but potentially impacting clarity. Notably, several methods related to searching and filtering in the gRPC services have been removed, indicating a shift in API functionality.

Changes

File Path Change Summary
buf.gen.yaml Updated to version 2; removed enabled, added disable and override; changed plugin to remote for several plugins.
buf.work.yaml File removed.
buf.yaml Updated to version 2; added modules section; updated lint and breaking configurations.
rust/libs/proto/src/filter.egress.v1.tonic.rs Removed documentation comments from FilterClient and FilterServer structs.
rust/libs/proto/src/filter.ingress.v1.tonic.rs Removed documentation comments from FilterClient and FilterServer structs.
rust/libs/proto/src/meta.v1.tonic.rs Removed documentation comments from get, set, and delete methods in meta_client and meta_server.
rust/libs/proto/src/vald.v1.tonic.rs Removed multi_search_object and search_by_id methods from FilterClient and SearchClient.

Possibly related PRs

Suggested labels

type/feature, priority/medium, area/internal, 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, actions/backport/release/v1.7

Suggested reviewers

  • hlts2
  • vankichi

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between fbc36f3 and 52be243.

⛔ Files ignored due to path filters (2)
  • apis/proto/buf.lock is excluded by !**/*.lock
  • buf.lock is 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 (8)
buf.yaml (4)

17-21: New modules and deps sections approved.

The addition of modules and deps sections is consistent with Buf v2 configuration. The specified path and dependencies appear appropriate for a project using protocol buffers and gRPC.

Please verify that the listed dependencies are necessary and up-to-date. Run the following script to check the dependencies:

#!/bin/bash
# Description: Verify Buf dependencies.

# Test: List all dependencies
buf mod ls-deps

# Test: Update dependencies to their latest versions
buf mod update

29-34: Breaking section updates approved with caution.

The changes to the breaking section, including the retention of FILE and the addition of specific exceptions, suggest a balanced approach to managing breaking changes.

Please carefully verify that these breaking change rules, especially the exceptions, align with your project's compatibility requirements. Run the following script to check for potential breaking changes:

#!/bin/bash
# Description: Verify Buf breaking change detection.

# Test: Run breaking change detection
buf breaking --against '.git#branch=main'

# Test: Check for any extensions that might be affected by EXTENSION_NO_DELETE
rg --type proto -l 'extend\s+\w+\s*{' apis/proto

# Test: Check for fields with default values that might be affected by FIELD_SAME_DEFAULT
rg --type proto -l '=\s*\[default\s*=\s*' apis/proto

Consider documenting the rationale for the EXTENSION_NO_DELETE and FIELD_SAME_DEFAULT exceptions in a comment or in the project documentation. Ensure that these exceptions do not inadvertently allow breaking changes that could impact API consumers.


16-16: Version update to v2 approved.

The update to Buf v2 aligns with the PR objective. This change may introduce new features and potentially breaking changes.

Please ensure that all project components are compatible with Buf v2. Run the following script to check for any compatibility issues:


22-28: Lint section updates approved with suggestions.

The changes to the lint section align with Buf v2 best practices. The use of STANDARD and the addition of specific exceptions suggest a thoughtful approach to linting.

Please verify that the linting rules, especially the exceptions, are appropriate for your project. Run the following script to check the linting results:

Consider documenting the rationale for the FIELD_NOT_REQUIRED and PACKAGE_NO_IMPORT_CYCLE exceptions in a comment or in the project documentation.

✅ Verification successful

Lint section correctly configured.

The lint section in buf.yaml includes the use: - STANDARD, the exceptions for FIELD_NOT_REQUIRED and PACKAGE_NO_IMPORT_CYCLE, and disallow_comment_ignores is set to true as intended.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Buf linting rules.

# Test: Run linter with new configuration
buf lint

# Test: Check for any files that would be affected by the FIELD_NOT_REQUIRED rule
rg --type proto -l '(optional|repeated)' apis/proto

# Test: Check for any potential import cycles
buf lint --error-format=json | jq '.path as $path | select(.message | contains("PACKAGE_NO_IMPORT_CYCLE")) | $path'

Length of output: 344


Script:

#!/bin/bash
# Description: Verify Buf linting rules without using buf.

# Test: Check if buf.yaml contains the STANDARD lint rule
grep -E 'use:\s*- STANDARD' buf.yaml

# Test: Verify exceptions for FIELD_NOT_REQUIRED and PACKAGE_NO_IMPORT_CYCLE
grep -E 'except:\s*- FIELD_NOT_REQUIRED' buf.yaml
grep -E 'except:\s*- PACKAGE_NO_IMPORT_CYCLE' buf.yaml

# Test: Ensure disallow_comment_ignores is set to true
grep -E 'disallow_comment_ignores:\s*true' buf.yaml

# Test: Corrected rg command to search for 'extend' usage in proto files
rg --type proto -l 'extend\s+\w+\s*\{' apis/proto

# Test: Corrected rg command to search for default values in proto files
rg --type proto -l '=\s*\[default\s*=' apis/proto

Length of output: 398


Script:

#!/bin/bash
# Description: Thoroughly verify the lint section in buf.yaml.

# Display the entire lint section for manual inspection
sed -n '/^lint:/,/^[^ ]/p' buf.yaml

# Alternatively, use awk to print the lint section
awk '/^lint:/{flag=1;next} /^[^ ]/{flag=0} flag' buf.yaml

# Check specifically for 'use: - STANDARD'
grep -E '^\s*use:\s*-$\s*STANDARD' buf.yaml

# Check specifically for exceptions
grep -E '^\s*except:\s*-\s*FIELD_NOT_REQUIRED' buf.yaml
grep -E '^\s*except:\s*-\s*PACKAGE_NO_IMPORT_CYCLE' buf.yaml

# Verify 'disallow_comment_ignores: true'
grep -E '^\s*disallow_comment_ignores:\s*true' buf.yaml

Length of output: 611


Script:

#!/bin/bash
# Description: Refined verification of Buf linting rules with corrected grep patterns.

# Check if 'use: - STANDARD' is correctly set
grep -E '^\s*use:\s*-\s*STANDARD' buf.yaml

# Verify exceptions for 'FIELD_NOT_REQUIRED' and 'PACKAGE_NO_IMPORT_CYCLE'
grep -E '^\s*except:\s*-\s*FIELD_NOT_REQUIRED' buf.yaml
grep -E '^\s*except:\s*-\s*PACKAGE_NO_IMPORT_CYCLE' buf.yaml

# Ensure 'disallow_comment_ignores' is set to true
grep -E '^\s*disallow_comment_ignores:\s*true' buf.yaml

Length of output: 256

buf.gen.yaml (4)

16-47: Summary of changes to buf.gen.yaml

The updates to buf.gen.yaml represent a significant improvement in the project's Protocol Buffer code generation configuration:

  1. Upgrade to Buf configuration version 2.
  2. Enhanced control over package management in the managed section.
  3. Updated plugin declarations and simplified option structures.

These changes should result in more efficient and maintainable code generation processes. However, it's crucial to run the provided verification scripts to ensure all systems are functioning as expected with the new configuration.

Additionally, please ensure that all team members are aware of these changes, as they may impact development workflows.


18-23: Approve changes in the managed section.

The new disable and override fields provide more precise control over code generation:

  1. Disabling go_package for googleapis prevents unintended overwrites.
  2. Setting go_package_prefix ensures consistent package naming for generated Go code.

These changes align with best practices for managing generated code in a large project.

To verify the impact of these changes, please run the following script:

#!/bin/bash
# Description: Verify the impact of managed section changes on generated code

# Test: Generate code and check for changes in package declarations
buf generate

# Test: Check for the correct package prefix in generated Go files
rg -t go 'package github.com/vdaas/vald/apis/grpc' apis/grpc

# Test: Ensure googleapis packages are not affected
rg -t go 'package google.' apis/grpc

25-47: Approve updates to plugin declarations and configurations.

The changes in the plugins section reflect an update in Buf's plugin system and improve configuration readability:

  1. All plugin: declarations have been changed to remote:, aligning with Buf's latest practices for plugin management.
  2. Plugin option structures have been simplified, enhancing maintainability.
  3. All necessary plugins for Go, Rust, and documentation generation are retained.

These updates should streamline the code generation process and potentially improve performance.

To ensure all plugins are functioning correctly with the new configuration, please run the following script:

#!/bin/bash
# Description: Verify functionality of updated plugins

# Test: Generate code using all configured plugins
buf generate

# Test: Check for expected output files
echo "Checking for Go files:"
ls apis/grpc/*.go
echo "Checking for Rust files:"
ls rust/libs/proto/src/*.rs
echo "Checking for documentation files:"
ls apis/docs/v1/*.md
echo "Checking for OpenAPI/Swagger files:"
ls apis/swagger/*.json

# Test: Verify no errors in generated files
echo "Checking for any error messages in generated files:"
rg -i 'error|warning|failed' apis/grpc apis/docs/v1 apis/swagger rust/libs/proto/src

16-16: Approve version update to v2.

The update to version 2 of the Buf configuration format is a significant change. This aligns the project with the latest Buf practices and potentially unlocks new features.

To ensure compatibility, please run the following script to check for any version-related issues:

✅ Verification successful

Confirm version update to v2 implemented correctly.

The update to version 2 of the Buf configuration format has been successfully implemented. This is evidenced by the presence of "version: v2" in all relevant Buf configuration files (buf.yaml, buf.gen.yaml, and buf.lock). The change aligns the project with the latest Buf practices and potentially unlocks new features as mentioned in the original review comment.

No compatibility issues were detected in the configuration files. The buf.gen.yaml file includes appropriate settings for generating Go, Rust, and documentation files, which suggests that the project's build process has been updated to work with the new version.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Buf version compatibility

# Test: Check Buf CLI version
buf --version

# Test: Validate the configuration
buf lint
buf build

# Test: Check for any deprecated options in other Buf-related files
rg -i 'deprecated|removed' -g 'buf*.yaml'

Length of output: 293090


Script:

#!/bin/bash
# Check for Buf-related configuration files and their content
echo "Checking for buf.yaml:"
cat buf.yaml 2>/dev/null || echo "buf.yaml not found"

echo -e "\nChecking for buf.gen.yaml:"
cat buf.gen.yaml 2>/dev/null || echo "buf.gen.yaml not found"

echo -e "\nChecking for buf.lock:"
cat buf.lock 2>/dev/null || echo "buf.lock not found"

echo -e "\nSearching for Buf version information in Rust files:"
grep -r "buf" rust/libs/proto/src/ | grep -i "version"

echo -e "\nChecking for any Buf-related entries in .gitignore:"
grep -i "buf" .gitignore 2>/dev/null || echo "No Buf-related entries found in .gitignore"

Length of output: 3698


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@vdaas-ci
Copy link
Collaborator Author

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@kpango kpango merged commit 1f328a0 into release/v1.7 Oct 11, 2024
160 of 161 checks passed
@kpango kpango deleted the backport/release/v1.7/feature/buf/migrate-to-v2 branch October 11, 2024 01:50
@kpango kpango mentioned this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants