[patch] Release v1.7.14 #2699
[patch] Release v1.7.14 #2699
Conversation
Signed-off-by: kpango <[email protected]>
📝 WalkthroughWalkthroughThis pull request introduces multiple changes across various files, including the addition of new contributors in the Changes
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 (
|
|
[FORMAT] Failed to format. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
🛑 Comments failed to post (13)
.github/ISSUE_TEMPLATE/bug_report.md (1)
25-32: 💡 Codebase verification
Issues Identified with Version Numbers and Consistency
The bug report template requires updates to ensure accuracy and consistency:
- Go Version: Listed as
go1.23.2, which exceeds the latest stable release1.21.x.- Docker Version: Listed as
v19.03.14, which is outdated compared to the current releases.- Kubernetes Version: Listed as
v1.31.0, surpassing the latest stable releasev1.28.x.- Rust Version: Unable to verify the version due to a script error; please verify manually.
- Vald Version: Correctly prefixed with 'v' (
v1.7.13), maintaining consistency.Please update the version numbers in the template to reflect the latest stable releases and ensure all entries follow a consistent formatting standard.
🔗 Analysis chain
Verify version numbers and ensure consistency
The updated version information is a valuable addition to the bug report template. However, there are a few points that require attention:
The Vald version (v1.7.13) is missing the 'v' prefix used in other entries. Consider adding it for consistency.
Some version numbers seem unusually high and may not reflect current stable releases:
- Go v1.23.2 (latest stable is 1.21.x as of October 2023)
- Kubernetes v1.31.1 (latest stable is 1.28.x as of October 2023)
The addition of Rust, Helm, and Faiss versions aligns well with the PR objectives.
Please run the following script to verify the latest stable versions of these software components:
Please update the version numbers in the template based on the script output, ensuring they reflect the latest stable versions compatible with your project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify latest stable versions of software components echo "Vald version:" gh release list --repo vdaas/vald --limit 1 echo "Go version:" curl -sSL https://golang.org/VERSION?m=text echo "Rust version:" curl -sSL https://static.rust-lang.org/dist/channel-rust-stable.toml | grep -m 1 "version =" echo "Docker version:" curl -sSL https://api.github.com/repos/docker/docker-ce/releases/latest | jq -r .tag_name echo "Kubernetes version:" curl -sSL https://storage.googleapis.com/kubernetes-release/release/stable.txt echo "Helm version:" curl -sSL https://api.github.com/repos/helm/helm/releases/latest | jq -r .tag_name echo "NGT version:" gh release list --repo yahoojapan/NGT --limit 1 echo "Faiss version:" gh release list --repo facebookresearch/faiss --limit 1Length of output: 1173
.github/workflows/build-protobuf.yaml (2)
42-42: 💡 Codebase verification
Inconsistency Found: 'TAG' Output Variable Missing in
_detect-ci-containerWorkflowThe output variable
TAGis not defined in the_detect-ci-containerworkflow. Please update the_detect-ci-containerworkflow to include theTAGoutput to ensure consistency with thebuild-protobuf.yamlworkflow.🔗 Analysis chain
Verify the consistency of the output variable change.
The output variable referenced in the
buildjob's container image has been updated fromTAG_NAMEtoTAG. This change is crucial for the workflow to function correctly.Run the following script to verify if this change is consistent with the
_detect-ci-containerworkflow:If the script doesn't find the
TAGoutput, it might indicate an inconsistency between this workflow and the_detect-ci-containerworkflow. In that case, you'll need to update either this file or the_detect-ci-containerworkflow to ensure they match.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of the TAG output in the _detect-ci-container workflow # Test: Search for the output definition in the _detect-ci-container workflow output_definition=$(rg --type yaml 'outputs:.*TAG:' .github/workflows/_detect-ci-container.yaml) if [ -n "$output_definition" ]; then echo "Output 'TAG' found in _detect-ci-container workflow:" echo "$output_definition" else echo "Error: Output 'TAG' not found in _detect-ci-container workflow." echo "Please ensure that the output variable is correctly defined as 'TAG' in the _detect-ci-container workflow." fiLength of output: 487
37-37: 💡 Codebase verification
Issue Detected: Missing 'TAG' Output Variable in
_detect-ci-container.yamlThe workflow
.github/workflows/_detect-ci-container.yamldoes not define the requiredTAGoutput variable. This mismatch may cause failures in thebuild-protobuf.yamlworkflow when it referencesTAG.🔗 Analysis chain
Verify the existence of the new workflow file.
The file extension for the
_detect-ci-containerworkflow has been changed from.ymlto.yaml. While this change promotes consistency in file extensions, we need to ensure that the new file actually exists.Run the following script to verify the existence of the new workflow file:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the _detect-ci-container.yaml file # Test: Check if the file exists if [ -f ".github/workflows/_detect-ci-container.yaml" ]; then echo "File exists: .github/workflows/_detect-ci-container.yaml" else echo "Error: File not found: .github/workflows/_detect-ci-container.yaml" echo "Please ensure the file has been renamed or created with the .yaml extension." fiLength of output: 487
.github/workflows/_detect-ci-container.yaml (1)
41-48:
⚠️ Potential issueShell script needs improvement for robustness and safety.
The final step to process the Docker image tag has some areas for improvement:
- Address the shellcheck warning about word splitting:
- TAG=$(echo "$TAGS" | awk '{print $1}' | awk -F '=' '{print $2}') + TAG=$(echo "$TAGS" | awk '{print $1}' | awk -F '=' '{print $2}' | tr -d '"')
- Add error handling for when no tags are found:
- name: Get Docker image tag from detection result id: get_tag_name run: | if [ -z "$TAGS" ]; then echo "Error: No tags found for the image." exit 1 fi TAG=$(echo "$TAGS" | awk '{print $1}' | awk -F '=' '{print $2}' | tr -d '"') if [ -z "$TAG" ]; then echo "Error: Failed to extract tag from '$TAGS'" exit 1 fi echo "TAG=${TAG}" echo "TAG=${TAG}" >> $GITHUB_OUTPUT env: TAGS: ${{ steps.detect_tag_name.outputs.IMAGE_TAGS }}These changes will make the script more robust and handle potential error cases.
🧰 Tools
🪛 actionlint
43-43: shellcheck reported issue in this script: SC2086:info:3:22: Double quote to prevent globbing and word splitting
(shellcheck)
.github/helm/values/values-correction.yaml (2)
34-48: 🛠️ Refactor suggestion
Consider enabling HPA and adjusting replica count for the agent.
The agent component has a fixed number of replicas (10) and HPA disabled. Enabling HPA and adjusting the replica count can provide better scalability and resource utilization.
agent: - minReplicas: 10 - maxReplicas: 10 + minReplicas: 3 + maxReplicas: 20 podManagementPolicy: Parallel hpa: - enabled: false + enabled: true + targetCPUUtilizationPercentage: 80 + targetMemoryUtilizationPercentage: 80 resources: requests: cpu: 100m memory: 50Mi ngt: auto_index_duration_limit: 2m auto_index_check_duration: 30s auto_index_length: 500 dimension: 784📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.agent: minReplicas: 3 maxReplicas: 20 podManagementPolicy: Parallel hpa: enabled: true targetCPUUtilizationPercentage: 80 targetMemoryUtilizationPercentage: 80 resources: requests: cpu: 100m memory: 50Mi ngt: auto_index_duration_limit: 2m auto_index_check_duration: 30s auto_index_length: 500 dimension: 784
22-33: 🛠️ Refactor suggestion
Consider enabling HPA for better scalability.
The gateway load balancer has HPA disabled. Enabling HPA can help the system automatically scale based on resource utilization, improving overall performance and resource efficiency.
gateway: lb: enabled: true minReplicas: 1 hpa: - enabled: false + enabled: true + targetCPUUtilizationPercentage: 80 + targetMemoryUtilizationPercentage: 80 resources: requests: cpu: 100m memory: 50Mi gateway_config: index_replica: 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.gateway: lb: enabled: true minReplicas: 1 hpa: enabled: true targetCPUUtilizationPercentage: 80 targetMemoryUtilizationPercentage: 80 resources: requests: cpu: 100m memory: 50Mi gateway_config: index_replica: 2.deepsource.toml (1)
50-70: 💡 Codebase verification
Discrepancy Detected: Missing Dockerfile Entries in
.deepsource.tomlThe following Dockerfiles are present in the repository but not listed in
.deepsource.toml:
dockers/binfmt/Dockerfiledockers/buildkit/syft/scanner/DockerfilePlease update
.deepsource.tomlto include these Dockerfiles to ensure comprehensive static analysis.🔗 Analysis chain
LGTM! Dockerfile paths updated to reflect project restructuring.
The changes to the
dockerfile_pathssection appear to align with the PR objectives of enhancing and refactoring the project structure. The new paths suggest a more granular and organized Docker setup, which is a positive improvement.To ensure all Dockerfiles are correctly included, please run the following script:
This will help identify any discrepancies between the actual Dockerfiles in the repository and those listed in the configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all Dockerfiles are included in .deepsource.toml # Test: Compare Dockerfiles in the repo with those listed in .deepsource.toml echo "Dockerfiles in the repository:" fd Dockerfile echo "---" echo "Dockerfiles listed in .deepsource.toml:" rg 'dockerfile_paths' .deepsource.toml -A 30 | rg '".+"'Length of output: 2100
.github/workflows/check-conflict.yaml (1)
36-58:
⚠️ Potential issueScript functionality is correct, but could be improved.
The script successfully checks for merge conflicts and handles the GitHub API interaction. However, there are some areas for improvement:
- Use double quotes to prevent globbing and word splitting.
- Replace backticks with
$()for command substitution.- Add error handling for curl commands.
- Consider using GitHub CLI (
gh) for API interactions if available in the runner environment.Here's a revised version of the script addressing these points:
- name: Check conflict run: | if grep -r "<<<< HEAD" . --exclude-dir=.git --exclude=check-conflict.yaml; then PR_COMMENTS=$(curl -s -H "Authorization: token $GITHUB_TOKEN" "$API_URL?per_page=10000") BODY=$(echo -E "$PR_COMMENTS" | jq 'last(.[] | select(.user.login == "vdaas-ci") | select(.body | test("^\\\\*\\\\*\\\\[WARNING:CONFLICT")) | .body)' -r) if [ "$BODY" = "null" ]; then if ! curl --fail \ -H "Accept: application/json" \ -H "Content-Type:application/json" \ -H "Authorization: token $GITHUB_TOKEN" \ --request POST \ --data '{"body": "**[WARNING:CONFLICT]** You may need to resolve conflicts. Please check."}' \ "$API_URL"; then echo "Failed to post comment to PR" exit 1 fi fi echo "Please fix conflict locally." exit 1 else echo "No conflicts found" fi env: GITHUB_TOKEN: ${{ secrets.DISPATCH_TOKEN }} API_URL: ${{ github.event.pull_request.comments_url }}This version addresses the shellcheck warnings and adds basic error handling for the curl command.
🧰 Tools
🪛 actionlint
37-37: shellcheck reported issue in this script: SC2006:style:2:15: Use $(...) notation instead of legacy backticks
...(shellcheck)
37-37: shellcheck reported issue in this script: SC2086:info:2:21: Double quote to prevent globbing and word splitting
(shellcheck)
37-37: shellcheck reported issue in this script: SC2006:style:3:8: Use $(...) notation instead of legacy backticks
...(shellcheck)
37-37: shellcheck reported issue in this script: SC2086:info:12:5: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/dockers-buildbase-image.yaml (1)
20-72: 💡 Codebase verification
Security concern:
secrets: inheritused withpull_request_target.The workflow inherits secrets using
secrets: inherit, which can expose sensitive information when used withpull_request_target.
- File:
.github/workflows/dockers-buildbase-image.yaml- Line:
secrets: inherit🔗 Analysis chain
Workflow triggers are well-defined, but verify security implications.
The triggers for this workflow are comprehensive and well-structured:
- Hourly scheduled builds ensure regular updates.
- Push triggers on main and release branches are appropriate.
- Detailed path filters for pull requests prevent unnecessary builds.
However, the use of
pull_request_targetshould be carefully reviewed:Please verify the security implications of using
pull_request_target. This event can potentially expose secrets to fork-based pull requests. Ensure that proper precautions are in place to prevent unauthorized access to sensitive information.To assist in this verification, you can run the following script to check for any sensitive operations in the workflow:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potentially sensitive operations in the workflow # Test: Look for uses of secrets or other potentially sensitive operations rg --type yaml '(secrets:|env:|\${{ secrets)' .github/workflows/dockers-buildbase-image.yamlLength of output: 366
Script:
#!/bin/bash # Description: Check for potentially sensitive operations in the workflow # Test: Look for uses of secrets or other potentially sensitive operations rg --type yaml '(secrets:|env:|\$\{\{\s*secrets)' .github/workflows/dockers-buildbase-image.yamlLength of output: 119
.github/workflows/dockers-agent-image.yaml (1)
81-86: 💡 Codebase verification
Issue Detected: Duplicate
runs-onEntries in_docker-image.yamlThe
_docker-image.yamlfile contains multipleruns-on: ubuntu-latestentries within thejobssection. Duplicate keys in YAML can lead to unexpected behavior and configuration issues.
.github/workflows/_docker-image.yaml: Duplicateruns-on: ubuntu-latestentries in thejobssection.Additionally, there's a commented-out
username: ${{ github.actor }}line that should be reviewed to ensure proper configuration.🔗 Analysis chain
LGTM: Job definition is concise and follows good practices.
The job definition uses a reusable workflow, which is excellent for maintaining DRY principles. The use of a separate workflow file for Docker image building suggests a modular approach to CI/CD pipelines. Inheriting secrets is crucial for secure access to necessary credentials during the build process.
To ensure the referenced workflow file exists and is properly configured, please run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and basic structure of the _docker-image.yaml file if [ -f .github/workflows/_docker-image.yaml ]; then echo "File _docker-image.yaml exists. Checking its content:" grep -E "name:|on:|jobs:" .github/workflows/_docker-image.yaml else echo "Error: _docker-image.yaml file not found in .github/workflows/" fiLength of output: 962
.github/workflows/fossa.yaml (1)
32-32: 💡 Codebase verification
File extension consistency issue detected.
The workflow file
.github/workflows/codeql-analysis.ymluses the.ymlextension, which is inconsistent with other workflow files using.yaml. It is recommended to standardize all workflow files to use the.yamlextension for consistency.🔗 Analysis chain
LGTM: File extension change is acceptable.
The change from
.ymlto.yamlis valid as both are acceptable extensions for YAML files. However, it's important to maintain consistency across the project.To ensure consistency, let's check if other workflow files use the same extension:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in workflow file extensions # Test: Count .yml and .yaml files in workflows directory echo "Workflow file extension usage:" fd -e yml -e yaml . .github/workflows | sort | uniq -cLength of output: 572
Script:
#!/bin/bash # Description: Check for consistency in workflow file extensions # Test: Count .yml and .yaml files in workflows directory echo "Workflow file extension usage:" fd -e yml -e yaml . .github/workflows | sort | uniq -cLength of output: 3347
.github/workflows/e2e.yaml (1)
223-223: 💡 Codebase verification
Action Required: Standardize
E2E_WAIT_FOR_CREATE_INDEX_DURATIONto 5m across all instances.Multiple occurrences of
E2E_WAIT_FOR_CREATE_INDEX_DURATIONwith differing durations found in.github/workflows/e2e.yaml. Ensure all instances are updated to 5m to maintain consistency and prevent flaky tests.🔗 Analysis chain
LGTM! Monitor impact on workflow duration.
Increasing
E2E_WAIT_FOR_CREATE_INDEX_DURATIONfrom 3m to 5m is a good change to reduce potential flaky tests caused by insufficient wait time for index creation.Monitor the impact of this change on overall workflow duration. Run the following script to compare job durations before and after this change:
If there's a significant increase in job duration, consider optimizing other parts of the workflow to compensate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare e2e-stream-crud-skip-exist-check job durations # Test: Fetch and compare job durations echo "Recent job durations:" gh run list --workflow=e2e.yaml --limit 10 --json databaseId,createdAt --jq '.[] | [.databaseId, .createdAt] | @tsv' | while read -r run_id created_at; do duration=$(gh run view $run_id --json jobs --jq '.jobs[] | select(.name == "E2E test (Stream CRUD: skip strict exist check)") | .durationInMinutes') echo "$created_at: $duration minutes" doneLength of output: 2749
Script:
#!/bin/bash # Description: Verify the updated E2E_WAIT_FOR_CREATE_INDEX_DURATION in e2e.yaml # Search for the E2E_WAIT_FOR_CREATE_INDEX_DURATION variable and print its value grep 'E2E_WAIT_FOR_CREATE_INDEX_DURATION=' .github/workflows/e2e.yamlLength of output: 485
.gitfiles (1)
881-885: 💡 Codebase verification
Exclude Generated Index Files from Version Control
The files
grp,obj,prf, andtrewithininternal/core/algorithm/ngt/assets/index/are currently tracked in the repository. If these files are generated during build or runtime processes, they should be excluded from version control to avoid potential merge conflicts and reduce repository size.
- Update
.gitignoreto exclude the following files:
internal/core/algorithm/ngt/assets/index/grpinternal/core/algorithm/ngt/assets/index/objinternal/core/algorithm/ngt/assets/index/prfinternal/core/algorithm/ngt/assets/index/tre🔗 Analysis chain
Confirm the inclusion of potential generated files
The files within
internal/core/algorithm/ngt/assets/index/:
grpobjprftreappear to be index files that might be generated during runtime or build processes. Including generated files in version control can lead to merge conflicts and increased repository size.
Please run the following script to check if these files are being generated and should be excluded from version control:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find index files that might be generated and should not be committed. # Expected result: List of potentially generated index files included in the repository. fd --type f --regex '^(grp|obj|prf|tre)$' internal/core/algorithm/ngt/assets/index/Length of output: 606
Script:
#!/bin/bash # Description: Find index files that might be generated and should not be committed. # Expected result: List of potentially generated index files included in the repository. fd --type f --regex '^(grp|obj|prf|tre)$' internal/core/algorithm/ngt/assets/index/Length of output: 265
✨ New feature
⚡ Improve performance
♻️ Refactor
🐛 Bugfix
✏️ Document
✅ Testing
💚 CI
📈 Metrics/Tracing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores