refactor merge docker and github actions workflow gen logic #2769
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request involves the deletion of several files related to code generation and Kubernetes job management, indicating a restructuring of functionalities within the project. Additionally, multiple GitHub Actions workflow files have been updated to refine their triggering conditions and improve clarity. The version numbers for various software components have also been updated across several files, ensuring that dependencies are current. The overall changes reflect a significant update to the project's structure and dependencies without introducing new files or modifying existing content. 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
type/ci
area/helm
area/makefile
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
size/XXL
type/refactoring
priority/low
labels
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2769 +/- ##
==========================================
+ Coverage 23.87% 23.92% +0.05%
==========================================
Files 547 546 -1
Lines 54660 54544 -116
==========================================
+ Hits 13051 13052 +1
+ Misses 40823 40705 -118
- Partials 786 787 +1 ☔ 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: 8
🧹 Outside diff range and nitpick comments (18)
.github/workflows/dockers-index-save-image.yaml (1)
53-74: LGTM: Secure handling of fork PRsThe addition of
pull_request_targetwith mirrored path configurations enables secure handling of pull requests from forks while maintaining consistent trigger conditions.Note: The
pull_request_targetevent runs with repository secrets available, so ensure that any scripts or actions in the workflow properly validate the PR content before using sensitive resources.k8s/operator/helm/crds/valdhelmoperatorrelease.yaml (3)
124-136: Consider expanding logging level options for more granular control.While the current log levels (debug, info, error) cover basic needs, consider adding more standard levels like 'warn' and 'trace' for better granularity in logging.
level: enum: - debug + - trace - info + - warn - error type: string
222-225: Consider defining specific toleration fields for better validation.While using
x-kubernetes-preserve-unknown-fieldsprovides flexibility, consider explicitly defining the standard Kubernetes toleration fields for better schema validation and documentation.items: - type: object - x-kubernetes-preserve-unknown-fields: true + type: object + properties: + key: + type: string + operator: + type: string + enum: + - Exists + - Equal + value: + type: string + effect: + type: string + enum: + - NoSchedule + - PreferNoSchedule + - NoExecute + tolerationSeconds: + type: integer
107-107: Add enum validation for probe scheme field.The
schemefield in both liveness and readiness probes could benefit from explicit enum validation to ensure only valid values (HTTP/HTTPS) are used.scheme: + enum: + - HTTP + - HTTPS type: stringAlso applies to: 175-175
.github/workflows/dockers-helm-operator-image.yaml (1)
35-48: Consider refactoring duplicated paths configurationThe paths configuration is duplicated between
pull_requestandpull_request_targettriggers. Consider extracting the paths into a reusable anchor using YAML aliases to improve maintainability.Example refactor:
name: "Build docker image: helm-operator" on: + _shared_paths: &shared_paths + - .github/actions/docker-build/action.yaml + - .github/workflows/_docker-image.yaml + - .github/workflows/dockers-helm-operator-image.yaml + # ... rest of the paths ... pull_request: - paths: - - .github/actions/docker-build/action.yaml - # ... current paths ... + paths: *shared_paths pull_request_target: - paths: - - .github/actions/docker-build/action.yaml - # ... current paths ... + paths: *shared_pathscharts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml (1)
97-116: Consider adding descriptions for schema propertiesWhile the type definitions are good, consider adding descriptions for each property to improve the CRD documentation.
Example addition:
type: object +description: "Configuration for the benchmark dataset" properties: group: type: string + description: "Group identifier for the dataset" indexes: type: integer + description: "Number of indexes to create"charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (1)
Line range hint
446-466: Consider reducing socket options duplicationThe socket configuration options are duplicated across multiple components (server, gRPC, REST). Consider extracting these into a shared schema definition to improve maintainability.
Example approach:
definitions: socketOptions: type: object properties: ip_recover_destination_addr: type: boolean ip_transparent: type: boolean # ... other options ...Then reference this in each component using
$ref: "#/definitions/socketOptions".Also applies to: 537-560, 753-776
go.mod (1)
Line range hint
352-384: Review placeholder dependency versionsSeveral dependencies are using placeholder versions (00010101000000-000000000000). This could lead to build reproducibility issues:
- github.com/akrylysov/pogreb
- github.com/go-redis/redis/v8
- github.com/gocraft/dbr/v2
- github.com/gorilla/mux
Please specify exact versions for these dependencies to ensure build reproducibility.
.github/workflows/dockers-index-creation-image.yaml (2)
17-17: Architectural ImprovementsThe changes demonstrate good architectural improvements:
- Consolidated workflow generation under
hack/docker- Consistent security improvements across all workflows
- Better build trigger specificity
- Removal of unused functionality (index deletion)
However, please consider:
- Adding documentation about the removal of index deletion functionality
- Updating any deployment guides that might reference the removed features
- Adding migration guides if this is a breaking change
Removal of index deletion functionality requires broader cleanup
The verification reveals that while
.github/workflows/dockers-index-deletion.yamlis being deleted, there are still numerous references to index deletion functionality across the codebase that need attention:
- Source code:
pkg/index/job/deletion/andcmd/index/job/deletion/- Docker artifacts:
dockers/index/job/deletion/- Build configurations in Makefiles: Multiple targets for index-deletion
- Configuration and sample files still referencing
vald-index-deletionThis appears to be an incomplete removal of the index deletion functionality. Either:
- The index deletion feature should be retained, or
- A complete cleanup of all related code and configurations should be performed
🔗 Analysis chain
Line range hint
17-74: Verify impact of index deletion removalThe AI summary indicates that this change is part of removing the index deletion functionality (removal of
dockers-index-deletion.yamland related k8s configs). Please ensure this is intentional and won't break existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to index deletion rg "index.*deletion" --type yaml --type go # Check for any migration guides or documentation updates fd -e md -e txt | xargs rg -l "index.*deletion"Length of output: 1005
Script:
#!/bin/bash # Check for deleted files related to index deletion in recent commits git diff --name-status origin/main | grep -i "deletion" # Check for any k8s manifests or configs mentioning index deletion fd . -e yaml -e yml | xargs rg "index.*deletion" # Check for any remaining Docker-related files for index deletion fd Dockerfile | xargs rg "index.*deletion" fd . -e yaml --full-path ".*docker.*" | xargs rg "index.*deletion" # Look for any remaining service references rg "vald-index-deletion" --type yaml --type goLength of output: 3358
.github/workflows/dockers-index-correction-image.yaml (1)
Line range hint
1-74: Excellent workflow refactoring approachThe refactoring of these workflow files demonstrates good architectural decisions:
- Consistent structure across components
- Clear separation of concerns (Docker vs. Actions)
- Improved maintainability through generated files
- Proper security considerations with pull_request_target
Consider documenting the workflow generation process in the repository's documentation to help future contributors understand how to maintain or add new workflow files.
.github/workflows/dockers-dev-container-image.yaml (1)
32-45: Optimize path triggers to reduce duplicationThe same paths are duplicated between
pull_requestandpull_request_target. Consider using a reusable YAML anchor to reduce maintenance overhead and potential inconsistencies.Example refactor using YAML anchors:
on: # ... other triggers ... paths: &trigger_paths - .github/actions/docker-build/action.yaml # ... common paths ... pull_request: paths: *trigger_paths pull_request_target: paths: *trigger_pathsAlso applies to: 47-60
.github/workflows/dockers-binfmt-image.yaml (1)
Consider consolidating Docker image build schedules to reduce Actions usage
The verification reveals multiple Docker-related workflows running hourly, which significantly impacts GitHub Actions minutes:
dockers-buildkit-image.yamldockers-buildbase-image.yamldockers-binfmt-image.yamldockers-buildkit-syft-scanner-image.yamlWhile other workflows use daily schedules, these Docker builds could be coordinated to run less frequently or in a staggered manner to optimize resource usage.
🔗 Analysis chain
Line range hint
22-23: Review schedule frequency necessityThe workflow runs hourly which could consume significant GitHub Actions minutes. Consider if this frequency is necessary or if it could be reduced.
Consider:
- Reducing frequency if possible (e.g., daily instead of hourly)
- Coordinating with other scheduled workflows to distribute load
- Adding conditions to skip builds if no relevant changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other scheduled workflows that might overlap rg "cron:" .github/workflows/Length of output: 561
.github/workflows/dockers-buildkit-image.yaml (1)
29-32: Consider consolidating tag patternsThe tag patterns appear redundant:
tags: - "*.*.*" - "*.*.*-*" - v*.*.* - v*.*.*-*"Consider simplifying to:
tags: - "*.*.*" - "*.*.*-*"Since these patterns would already match tags with 'v' prefix.
.github/workflows/dockers-gateway-mirror-image.yaml (1)
50-51: Consider path organization optimizationThe paths
pkg/gateway/internal/**andpkg/gateway/mirror/**could potentially be consolidated.Consider using:
- pkg/gateway/{internal,mirror}/**This would maintain the same functionality with improved readability.
.github/workflows/dockers-loadtest-image.yaml (1)
Line range hint
17-74: Maintain consistency across workflow filesThe workflow structure is consistent with other files, which is good. However, consider extracting common path patterns into reusable workflow templates to reduce duplication and maintenance overhead.
Consider:
- Creating a reusable workflow template for common path patterns
- Using workflow composition to inherit common configurations
- Implementing a central configuration for shared patterns
.github/workflows/dockers-gateway-filter-image.yaml (1)
28-29: Consider consolidating version tag patternsThe tag patterns could be simplified as they overlap:
*.*.*andv*.*.*are redundant with*.*.*-*andv*.*.*-*respectively, as the latter patterns would also match the former.tags: - "*.*.*-*" - - "*.*.*" - v*.*.*-* - - v*.*.*".github/workflows/dockers-agent-image.yaml (1)
17-17: Consider implementing path deduplication in the workflow generatorAll three workflow files show similar patterns and share common issues:
- Duplicate version file references
- Potentially redundant tag patterns
- Generated nature of files makes manual fixes temporary
Consider enhancing the workflow generator to:
- Deduplicate paths automatically
- Validate path patterns
- Implement a more maintainable path organization structure
Would you like me to help create an issue to track these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (24)
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.goexample/client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumhack/actions/gen/main.gois excluded by!**/gen/**hack/actions/gen/main_test.gois excluded by!**/gen/**hack/docker/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (82)
.gitfiles(0 hunks).github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/ISSUE_TEMPLATE/security_issue_report.md(1 hunks).github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/coverage.yaml(1 hunks).github/workflows/dockers-agent-faiss-image.yaml(2 hunks).github/workflows/dockers-agent-image.yaml(2 hunks).github/workflows/dockers-agent-ngt-image.yaml(2 hunks).github/workflows/dockers-agent-sidecar-image.yaml(2 hunks).github/workflows/dockers-benchmark-job-image.yaml(2 hunks).github/workflows/dockers-benchmark-operator-image.yaml(2 hunks).github/workflows/dockers-binfmt-image.yaml(2 hunks).github/workflows/dockers-buildbase-image.yaml(2 hunks).github/workflows/dockers-buildkit-image.yaml(2 hunks).github/workflows/dockers-buildkit-syft-scanner-image.yaml(2 hunks).github/workflows/dockers-ci-container-image.yaml(2 hunks).github/workflows/dockers-dev-container-image.yaml(2 hunks).github/workflows/dockers-discoverer-k8s-image.yaml(2 hunks).github/workflows/dockers-example-client-image.yaml(2 hunks).github/workflows/dockers-gateway-filter-image.yaml(2 hunks).github/workflows/dockers-gateway-lb-image.yaml(2 hunks).github/workflows/dockers-gateway-mirror-image.yaml(2 hunks).github/workflows/dockers-helm-operator-image.yaml(2 hunks).github/workflows/dockers-index-correction-image.yaml(2 hunks).github/workflows/dockers-index-creation-image.yaml(2 hunks).github/workflows/dockers-index-deletion-image.yaml(2 hunks).github/workflows/dockers-index-deletion.yaml(0 hunks).github/workflows/dockers-index-operator-image.yaml(2 hunks).github/workflows/dockers-index-save-image.yaml(2 hunks).github/workflows/dockers-loadtest-image.yaml(2 hunks).github/workflows/dockers-manager-index-image.yaml(2 hunks).github/workflows/dockers-readreplica-rotate-image.yaml(2 hunks)CHANGELOG.md(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(0 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml(1 hunks)charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml(5 hunks)charts/vald/crds/valdmirrortarget.yaml(1 hunks)dockers/agent/core/agent/Dockerfile(1 hunks)dockers/agent/core/faiss/Dockerfile(1 hunks)dockers/agent/core/ngt/Dockerfile(1 hunks)dockers/agent/sidecar/Dockerfile(1 hunks)dockers/binfmt/Dockerfile(1 hunks)dockers/buildbase/Dockerfile(1 hunks)dockers/buildkit/Dockerfile(1 hunks)dockers/buildkit/syft/scanner/Dockerfile(1 hunks)dockers/ci/base/Dockerfile(1 hunks)dockers/dev/Dockerfile(1 hunks)dockers/discoverer/k8s/Dockerfile(1 hunks)dockers/example/client/Dockerfile(1 hunks)dockers/gateway/filter/Dockerfile(1 hunks)dockers/gateway/lb/Dockerfile(1 hunks)dockers/gateway/mirror/Dockerfile(1 hunks)dockers/index/job/correction/Dockerfile(1 hunks)dockers/index/job/creation/Dockerfile(1 hunks)dockers/index/job/deletion/Dockerfile(1 hunks)dockers/index/job/readreplica/rotate/Dockerfile(1 hunks)dockers/index/job/save/Dockerfile(1 hunks)dockers/index/operator/Dockerfile(1 hunks)dockers/manager/index/Dockerfile(1 hunks)dockers/operator/helm/Dockerfile(1 hunks)dockers/tools/benchmark/job/Dockerfile(1 hunks)dockers/tools/benchmark/operator/Dockerfile(1 hunks)dockers/tools/cli/loadtest/Dockerfile(1 hunks)example/client/go.mod(1 hunks)example/client/go.mod.default(1 hunks)go.mod(19 hunks)hack/helm/schema/crd/main.go(1 hunks)k8s/operator/helm/crds/valdhelmoperatorrelease.yaml(5 hunks)rust/rust-toolchain(1 hunks)versions/BUF_VERSION(1 hunks)versions/CMAKE_VERSION(1 hunks)versions/GOLANGCILINT_VERSION(1 hunks)versions/GO_VERSION(1 hunks)versions/HELM_VERSION(1 hunks)versions/K3S_VERSION(1 hunks)versions/KUBECTL_VERSION(1 hunks)versions/OPERATOR_SDK_VERSION(1 hunks)versions/PROMETHEUS_STACK_VERSION(1 hunks)versions/PROTOBUF_VERSION(1 hunks)
⛔ Files not processed due to max files limit (13)
- versions/REVIEWDOG_VERSION
- versions/RUST_VERSION
- versions/TELEPRESENCE_VERSION
- versions/USEARCH_VERSION
- versions/YQ_VERSION
- versions/actions/ACTIONS_CACHE
- versions/actions/CODECOV_CODECOV_ACTION
- versions/actions/GITHUB_CODEQL_ACTION_ANALYZE
- versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD
- versions/actions/GITHUB_CODEQL_ACTION_INIT
- versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF
- versions/actions/GITHUB_ISSUE_METRICS
- versions/actions/REVIEWDOG_ACTION_HADOLINT
💤 Files with no reviewable changes (3)
- .github/workflows/dockers-index-deletion.yaml
- Makefile.d/functions.mk
- .gitfiles
✅ Files skipped from review due to trivial changes (41)
- versions/GO_VERSION
- versions/BUF_VERSION
- versions/GOLANGCILINT_VERSION
- versions/KUBECTL_VERSION
- dockers/buildkit/Dockerfile
- dockers/buildkit/syft/scanner/Dockerfile
- versions/HELM_VERSION
- dockers/index/job/deletion/Dockerfile
- dockers/ci/base/Dockerfile
- versions/PROTOBUF_VERSION
- dockers/buildbase/Dockerfile
- dockers/index/job/correction/Dockerfile
- versions/OPERATOR_SDK_VERSION
- dockers/gateway/lb/Dockerfile
- dockers/index/job/readreplica/rotate/Dockerfile
- rust/rust-toolchain
- dockers/binfmt/Dockerfile
- dockers/tools/benchmark/job/Dockerfile
- versions/CMAKE_VERSION
- dockers/index/job/save/Dockerfile
- dockers/gateway/mirror/Dockerfile
- versions/K3S_VERSION
- dockers/tools/cli/loadtest/Dockerfile
- dockers/agent/sidecar/Dockerfile
- dockers/index/operator/Dockerfile
- dockers/example/client/Dockerfile
- dockers/agent/core/faiss/Dockerfile
- dockers/manager/index/Dockerfile
- dockers/dev/Dockerfile
- dockers/agent/core/ngt/Dockerfile
- dockers/discoverer/k8s/Dockerfile
- dockers/tools/benchmark/operator/Dockerfile
- dockers/operator/helm/Dockerfile
- Makefile
- .github/ISSUE_TEMPLATE/bug_report.md
- versions/PROMETHEUS_STACK_VERSION
- .github/PULL_REQUEST_TEMPLATE.md
- dockers/agent/core/agent/Dockerfile
- .github/ISSUE_TEMPLATE/security_issue_report.md
- dockers/gateway/filter/Dockerfile
- dockers/index/job/creation/Dockerfile
🔇 Additional comments (81)
charts/vald/crds/valdmirrortarget.yaml (2)
91-91: LGTM! Explicit type declarations enhance schema validation.
The explicit type declarations for host (string) and port (integer) properties follow Kubernetes CRD best practices and improve schema validation.
Also applies to: 95-95
99-100: Verify impact on existing CRD instances.
The addition of explicit type declaration for the target object is correct. However, ensure that all existing ValdMirrorTarget instances in the cluster conform to this schema.
✅ Verification successful
Schema changes are compatible with existing instances
The existing ValdMirrorTarget instances in the repository follow the correct schema structure with target being an object containing host and port fields, which aligns perfectly with the explicit type declaration being added. The change is backward compatible and doesn't require any modifications to existing instances.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing ValdMirrorTarget instances for schema conformance
# Search for any existing ValdMirrorTarget yaml files in the repository
fd -e yaml -e yml | xargs rg -l "kind:\s*ValdMirrorTarget"
# Search for any example or test instances
fd -e yaml -e yml | xargs rg -l "valdmirrortargets.vald.vdaas.org"
Length of output: 254
Script:
#!/bin/bash
# Let's examine the content of the found ValdMirrorTarget instance to verify schema conformance
cat charts/vald/values/multi-vald/mirror-target.yaml
Length of output: 1057
charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml (6)
85-90: LGTM! Well-defined image configuration schema.
The image configuration schema is properly defined with explicit types and enums for pullPolicy, improving validation and preventing configuration errors.
107-107: LGTM! Consistent probe configurations.
The probe configurations (liveness and readiness) follow a consistent structure with proper type definitions for the httpGet configuration.
Also applies to: 175-175, 184-184
124-136: LGTM! Comprehensive logging configuration schema.
The logging configuration schema is well-defined with appropriate enums for log levels and formats. The stacktraceLevel property aligns with the logging level options.
212-214: LGTM! Service type configuration with proper validation.
The service type configuration includes appropriate enum values for Kubernetes service types, preventing invalid configurations.
214-220: LGTM! Clear ServiceAccount configuration schema.
The ServiceAccount configuration schema is properly defined with boolean and string types for create and name properties respectively.
222-225: LGTM! Flexible tolerations configuration.
The tolerations array schema allows for Kubernetes tolerations with preserved unknown fields, providing flexibility for various toleration configurations.
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (7)
Line range hint 95-142: LGTM! Well-structured client configuration schema.
The client configuration schema is comprehensive with proper type definitions for:
- Connection pool settings
- Circuit breaker configuration
- Backoff settings
Line range hint 164-233: LGTM! Detailed network configuration schema.
The network configuration schema includes well-defined types for:
- TLS settings
- Socket options
- DNS configuration
- Dialer settings
286-299: LGTM! Consistent logging configuration.
The logging configuration aligns with the helm operator CRD, providing consistent logging options across components.
Line range hint 326-359: LGTM! Comprehensive observability configuration.
The observability schema properly defines:
- OTLP configuration with metrics and tracing options
- Attribute configurations for metrics
- Trace sampling and export settings
Line range hint 411-657: LGTM! Extensive server configuration schema.
The server configuration schema is well-structured with proper type definitions for:
- Health check endpoints (liveness, readiness, startup)
- gRPC and REST server settings
- TLS configuration
688-696: LGTM! Consistent service configurations.
The service and ServiceAccount configurations align with the helm operator CRD, maintaining consistency across components.
703-703: LGTM! Flexible tolerations schema.
The tolerations configuration matches the helm operator CRD, providing consistent scheduling control across components.
.github/workflows/coverage.yaml (2)
Line range hint 1-70: LGTM! The workflow configuration follows best practices
The overall structure is well-designed with:
- Appropriate path filters for targeted execution
- Proper container-based execution setup
- Clear job dependencies and error handling
57-57: Verify compatibility with Codecov Action v5
The upgrade from v4 to v5 of codecov-action is a significant version bump that may introduce breaking changes.
Let's verify the changes and requirements:
Consider the following recommendations:
- Review the Codecov Action v5 documentation for any new features or configuration options that could enhance coverage reporting
- Ensure the
CODECOV_TOKENsecret is properly configured for v5
✅ Verification successful
The previous attempt to fetch release notes failed, so let's try a different approach to understand the usage and configuration in the codebase. This will help us verify if the current setup is compatible with v5 and if any adjustments are needed.
This will help us understand the existing Codecov configuration and verify if it's compatible with v5, as well as check how coverage is generated in the project.
Based on the gathered information, I can now provide the final response:
✅ Codecov Action v5 upgrade is safe to proceed
The upgrade to codecov-action@v5 is compatible with the current setup:
- The workflow correctly provides the required
CODECOV_TOKEN - The coverage file path (
./coverage.out) is properly specified - The project's codecov.yaml contains standard ignore patterns
- Version history shows previous successful updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes and requirements in codecov-action v5
# Fetch the latest release notes and changes
gh api repos/codecov/codecov-action/releases/tags/v5 --jq '.body' || echo "Failed to fetch v5 release notes"
# Check if any existing issues or discussions mention migration problems
gh search issues "repo:vdaas/vald codecov-action v5" --state all --json title,url,state || echo "No related issues found"
Length of output: 3161
Script:
#!/bin/bash
# Let's check the current workflow configuration and dependencies
# Check the coverage workflow file content
cat .github/workflows/coverage.yaml
# Search for any Codecov-related configurations or files
fd codecov
Length of output: 2213
Script:
#!/bin/bash
# Let's check the Codecov configuration and version history
# Check codecov.yaml content
cat codecov.yaml
# Check the version history file
cat versions/actions/CODECOV_CODECOV_ACTION
# Check if there are any specific test coverage patterns or configurations
rg -A 3 "coverage" Makefile
Length of output: 914
.github/workflows/dockers-index-save-image.yaml (2)
29-30: LGTM: Improved version tag patterns
The addition of v*.*.*-* pattern properly supports pre-release versions, following semantic versioning best practices.
33-52: Verify the completeness of path triggers
The path configuration has been refined with proper exclusions and additions. Let's verify that all necessary paths are included.
✅ Verification successful
Let me gather more information about the Docker-related files and their dependencies to ensure we haven't missed any critical paths.
Based on the gathered information, I can now generate the final response:
Path triggers configuration is complete and well-structured
The current path configuration in the workflow file correctly covers all necessary components:
- All required Dockerfile paths are included (verified dockers/index/job/save/Dockerfile exists)
- Build-related files are properly configured (Makefile and Makefile.d/)
- Source code paths are correctly included (cmd/index/job/save/, pkg/index/job/save/, internal/)
- Proto files are properly covered (apis/proto/)
- Test and mock exclusions are appropriately set
- All specified directories exist in the repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify path configurations completeness
# Check for any potential Docker-related files that might be missed
fd -e Dockerfile -e dockerfile
# Check for any potential build-related files
fd -g 'Makefile*'
# Verify the existence of specified paths
for path in "cmd/index/job/save" "pkg/index/job/save" "internal/db" "internal/k8s"; do
if [ ! -d "$path" ]; then
echo "Warning: Directory $path not found"
fi
done
Length of output: 581
Script:
#!/bin/bash
# Check for any references to Docker files or build configurations
rg -g '!{.git,vendor}' -l 'FROM|COPY|ADD' dockers/
# Check for any Go files that might be related to the index/job/save functionality
fd -e go . cmd/index/job/save pkg/index/job/save
# Look for any potential proto files that might be needed for the build
fd -e proto . apis/proto/
# Check for any additional build-related files in the root
fd -d 1 -e yaml -e yml
Length of output: 2116
k8s/operator/helm/crds/valdhelmoperatorrelease.yaml (1)
85-90: LGTM! Well-structured image configuration schema.
The image configuration schema is properly defined with clear type constraints and enumerated values for pullPolicy.
.github/workflows/dockers-agent-sidecar-image.yaml (4)
29-30: LGTM! Enhanced version tag patterns
The addition of v*.*.* and v*.*.*-* patterns improves semantic versioning support by including pre-release tags.
33-53: Well-structured path filters with appropriate exclusions
The path filters have been improved by:
- Excluding test-related files (
*_mock.go,*_test.go) to prevent unnecessary builds - Adding specific internal directory exclusions
- Including Makefile-related paths for build configuration changes
- Maintaining relevant agent-sidecar specific paths
This results in more efficient and focused workflow triggers.
56-76: Verify path filter symmetry with pull_request section
The path filters in pull_request_target correctly mirror those in the pull_request section, which is essential for security.
✅ Verification successful
Path filters are correctly synchronized between pull_request and pull_request_target
After verifying the workflow file content, I can confirm that the path filters in both the pull_request and pull_request_target sections are identical, maintaining the required security symmetry. The paths are exactly the same in both sections, including all exclusions (!internal/**/*_mock.go, etc.) and inclusions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that path filters are identical between pull_request and pull_request_target sections
# Extract and compare the paths from both sections
rg -A 20 "pull_request:\n\s+paths:" .github/workflows/dockers-agent-sidecar-image.yaml > pr_paths.txt
rg -A 20 "pull_request_target:\n\s+paths:" .github/workflows/dockers-agent-sidecar-image.yaml > pr_target_paths.txt
diff pr_paths.txt pr_target_paths.txt
Length of output: 608
Script:
#!/bin/bash
# Let's try a different approach using multiline mode and simpler patterns
rg -U "pull_request:.*\n.*paths:" -A 20 .github/workflows/dockers-agent-sidecar-image.yaml > pr_paths.txt
rg -U "pull_request_target:.*\n.*paths:" -A 20 .github/workflows/dockers-agent-sidecar-image.yaml > pr_target_paths.txt
diff pr_paths.txt pr_target_paths.txt
Length of output: 283
Script:
#!/bin/bash
# Let's try to get the complete content of the file to analyze the paths sections
cat .github/workflows/dockers-agent-sidecar-image.yaml
Length of output: 2436
17-17: Verify the workflow generator's new location
The workflow generator path has changed from hack/actions/gen/main.go to hack/docker/gen/main.go.
✅ Verification successful
Based on the code inspection, I can now generate the final response:
Generator path change is valid and functional
The workflow generator has been correctly moved from hack/actions/gen/main.go to hack/docker/gen/main.go. The new generator exists and is actively used to generate both workflow files and Dockerfiles, as evidenced by:
- The file exists at the new location
- It contains comprehensive logic for generating both workflow files and Dockerfiles
- Multiple files reference this generator in their headers
- The generator is integrated into the build system via Makefile.d/functions.mk
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of the workflow generator
# Check if the new generator exists and contains workflow generation logic
rg -g "hack/docker/gen/main.go" -A 5 "func.*[Ww]orkflow.*[Gg]en"
# Check if the old generator was properly moved/renamed
git log --full-history -- "hack/actions/gen/main.go" "hack/docker/gen/main.go"
Length of output: 64
Script:
#!/bin/bash
# Let's verify if the generator file exists and check its content
fd "main.go" hack/docker/gen/
# Also check for any workflow-related files in hack directory
fd -t f ".*workflow.*" hack/
# Check if there are any references to the generator path in other files
rg -g "!*.yaml" "hack/docker/gen/main.go"
# Look for any generator-related code in the repository
rg -g "*.go" "func.*[Gg]enerate.*[Ww]orkflow"
Length of output: 4436
Script:
#!/bin/bash
# Let's check the content of the generator file
cat hack/docker/gen/main.go
# Also check if there are any references to workflow generation in the Makefile
rg -g "Makefile*" "workflow.*gen"
Length of output: 36551
.github/workflows/dockers-discoverer-k8s-image.yaml (4)
29-30: LGTM: Improved version tag patterns
The addition of explicit v-prefixed patterns (v*.*.* and v*.*.*-*) improves clarity and follows semantic versioning conventions while maintaining backward compatibility.
33-35: LGTM: Optimized workflow triggers
The exclusion of mock files, test files, and internal DB files is a good optimization that prevents unnecessary workflow runs, reducing CI load and costs.
38-51: Verify completeness of workflow triggers
The path configurations for both PR and PR target events are well-structured and symmetric. However, let's verify that all paths that could affect the discoverer-k8s component are included.
Also applies to: 54-72
✅ Verification successful
Based on the output, I can see that all imports are from packages that are already covered by the path triggers in the workflow:
- Internal packages (
github.com/vdaas/vald/internal/*) are covered byinternal/** - API packages are covered by
apis/grpc/**andapis/proto/** - Package-specific code is covered by
pkg/discoverer/k8s/** - Command code is covered by
cmd/discoverer/k8s/**
The path triggers are comprehensive and include all necessary dependencies. The error from fd command for the configs/ directory suggests there are no additional configuration files outside the already covered paths that could affect the build.
Path triggers are complete and well-structured
The current path configuration captures all dependencies and build-related files for the discoverer-k8s component. All imports and related files are properly covered by the existing trigger paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify completeness of path triggers by checking for:
# 1. Any imports from packages not in the trigger paths
# 2. Any configuration files that might affect the build
# Check imports in the discoverer-k8s component
rg -t go "^import \(" cmd/discoverer/k8s pkg/discoverer/k8s -A 10
# Check for any yaml/configuration files that might affect the build
fd -e yaml -e json -e toml . configs/ pkg/discoverer/k8s/
Length of output: 6597
17-17: Verify the new workflow generator implementation
The generator path has been moved from hack/actions/gen/main.go to hack/docker/gen/main.go, which aligns with the refactoring objective.
✅ Verification successful
Generator path change is correctly implemented
The workflow generator has been moved from hack/actions/gen/main.go to hack/docker/gen/main.go as indicated in the comment. The new generator file exists and contains comprehensive workflow generation logic, including:
- Proper workflow template generation
- Correct path handling for Docker and GitHub Actions
- Consistent license headers
- Appropriate trigger configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of the new workflow generator
# Check if the new generator exists and contains workflow generation logic
rg -g "hack/docker/gen/main.go" -A 5 "func.*[Ww]orkflow.*[Gg]en"
Length of output: 64
Script:
#!/bin/bash
# Let's first check if the file exists and examine its content
fd "main.go" "hack/docker/gen"
# Let's also check for any workflow-related files in the hack directory
fd "workflow" "hack"
# Let's search for workflow generation logic with a broader pattern
rg -g "*.go" "func.*[Gg]enerate.*[Ww]orkflow"
# Let's also check if there are any other generator implementations
fd "gen" "hack" -t d
Length of output: 260
Script:
#!/bin/bash
# Let's examine the content of the generator file
cat hack/docker/gen/main.go
# Also check for any GitHub workflow templates or related files
fd "workflow" ".github"
# Let's also check if there are any references to workflow generation in go.mod or other build files
rg -g "!vendor" -g "!*.sum" "github.*workflow"
Length of output: 37319
.github/workflows/dockers-readreplica-rotate-image.yaml (4)
29-30: LGTM! Enhanced semantic versioning support
The addition of v*.*.*-* pattern properly supports pre-release versions, following semantic versioning best practices.
33-51: LGTM! Improved workflow trigger paths
The changes improve the workflow efficiency by:
- Excluding mock and test files to prevent unnecessary builds
- Including all relevant component dependencies
- Properly organizing paths for the readreplica-rotate component
17-17: Verify the workflow generator file location
The workflow generator path has changed from hack/actions/gen/main.go to hack/docker/gen/main.go.
✅ Verification successful
Let me verify the content of the new generator file to ensure it's actually responsible for generating this workflow.
Let me try a different approach to verify if this file is actually generating Docker-related workflow files.
Let me check for the actual workflow template and Docker-related workflow generation code.
Generator file location and purpose verified
The new generator file at hack/docker/gen/main.go exists and is correctly responsible for generating Docker-related workflows, including the readreplica-rotate workflow. The file contains the necessary references and configuration for the readreplica-rotate component, confirming that the header comment accurately reflects the current source of generation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the workflow generator file
# and check if the old file was properly moved/renamed
# Check if the new generator exists
if fd -t f "main.go" -p "hack/docker/gen"; then
echo "✓ New generator exists"
else
echo "✗ New generator not found"
fi
# Check if the old generator still exists (should be removed)
if fd -t f "main.go" -p "hack/actions/gen"; then
echo "! Old generator still exists"
else
echo "✓ Old generator properly removed"
fi
Length of output: 332
Script:
#!/bin/bash
# Check the content of the new generator file
rg -A 5 "dockers-readreplica-rotate-image.yaml" hack/docker/gen/main.go
Length of output: 71
Script:
#!/bin/bash
# Look for Docker workflow generation patterns in the new generator
head -n 50 hack/docker/gen/main.go
# Also check if there are any workflow templates being used
fd -t f "template" -p "hack/docker/gen"
Length of output: 1482
Script:
#!/bin/bash
# Look for workflow generation and Docker-related code
rg -g "*.go" -g "*.tmpl" -l "workflow" hack/docker/gen/
# Check for any references to readreplica-rotate in the generator directory
rg -g "*.go" -g "*.tmpl" "readreplica-rotate" hack/docker/gen/
Length of output: 326
54-72: Verify security implications of pull_request_target
While the path configurations are correct, using pull_request_target requires careful consideration as it runs with repository secrets against PR-modified code.
Please ensure that:
- The reusable workflow
.github/workflows/_docker-image.yamlhas proper security checks - Secrets are only exposed to trusted code
✅ Verification successful
Let me gather more information about the security measures in both workflows.
Let me check one more aspect to verify the security measures for fork PRs.
Security measures for pull_request_target are properly implemented
The workflow has appropriate security controls in place:
- Fork PRs require the 'ci/approved' label before execution
- Platform is limited to 'linux/amd64' for PR builds
- Docker build process uses proper isolation and security labels
- Secrets are only exposed after label verification
The implementation follows GitHub's security best practices for handling pull_request_target events by:
- Requiring explicit approval via labels for fork PRs
- Limiting build scope for PR events
- Using proper checkout reference handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the security measures in the reusable workflow
# Check for common security patterns in the reusable workflow
rg -A 5 "pull_request_target|secrets:|permissions:" .github/workflows/_docker-image.yaml
Length of output: 432
Script:
#!/bin/bash
# Check permissions and security configurations in both workflows
echo "=== Checking _docker-image.yaml ==="
rg -U "(?s)^permissions:.*?[}\n]" .github/workflows/_docker-image.yaml
echo -e "\n=== Checking dockers-readreplica-rotate-image.yaml ==="
rg -U "(?s)^permissions:.*?[}\n]" .github/workflows/dockers-readreplica-rotate-image.yaml
echo -e "\n=== Checking job definitions and their uses ==="
rg -U "(?s)^jobs:.*?(^[a-z]|^$)" .github/workflows/_docker-image.yaml
rg -U "(?s)uses: ./.github/workflows/_docker-image.yaml" .github/workflows/dockers-readreplica-rotate-image.yaml
Length of output: 4013
Script:
#!/bin/bash
# Check the conditions and security measures for fork PRs
echo "=== Checking fork PR conditions in both workflows ==="
rg -U "fork.*true" .github/workflows/_docker-image.yaml .github/workflows/dockers-readreplica-rotate-image.yaml
echo -e "\n=== Checking label verification logic ==="
rg -U "ci/approved" .github/workflows/_docker-image.yaml .github/workflows/dockers-readreplica-rotate-image.yaml
echo -e "\n=== Checking docker build action for additional security measures ==="
cat .github/actions/docker-build/action.yaml
Length of output: 6182
.github/workflows/dockers-helm-operator-image.yaml (4)
29-30: LGTM! Good addition of pre-release version support
The addition of v*.*.*-* pattern alongside v*.*.* properly supports pre-release version tags (e.g., v1.2.3-alpha, v1.2.3-beta).
54-67: Skip comment - Already covered
This section is identical to the pull_request paths section and was addressed in the previous comment.
Line range hint 70-75: Verify the reusable workflow configuration
The workflow uses ./.github/workflows/_docker-image.yaml as a reusable workflow. Let's verify its existence and configuration.
✅ Verification successful
Reusable workflow configuration is properly set up and widely used
The reusable workflow ./.github/workflows/_docker-image.yaml is:
- Correctly configured to accept the required
targetinput parameter - Used by multiple other Docker image build workflows in the repository
- Has all its referenced custom actions present in
.github/actions/ - Properly handles Docker image building, scanning, and notifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the reusable workflow configuration
# Check if the reusable workflow exists and its configuration
if [ -f ".github/workflows/_docker-image.yaml" ]; then
# Check if it accepts the 'target' input parameter
rg "target:" .github/workflows/_docker-image.yaml -A 5
# Check for any references to helm-operator
rg "helm-operator" .github/workflows/_docker-image.yaml
fi
Length of output: 585
Script:
#!/bin/bash
# Let's verify the complete structure of the reusable workflow
# Check the complete content of the reusable workflow
cat .github/workflows/_docker-image.yaml
# Also check if there are any other workflows using this reusable workflow
rg "uses: \.\/\.github\/workflows\/_docker-image.yaml" .github/workflows/
Length of output: 4723
Script:
#!/bin/bash
# Let's check for other workflows using this reusable workflow with proper escaping
rg "uses: ./.github/workflows/_docker-image.yaml" .github/workflows/
# Also verify the referenced actions in the reusable workflow
ls -la .github/actions/docker-build
ls -la .github/actions/scan-docker-image
ls -la .github/actions/dump-context
ls -la .github/actions/notify-slack
Length of output: 5331
17-17: Verify the new workflow generator implementation
The workflow generator path has changed from hack/actions/gen/main.go to hack/docker/gen/main.go. Let's verify the new generator implementation.
✅ Verification successful
Let me gather more information about the generator implementation.
Let me verify the generator implementation and references.
Based on the findings, I can now generate the final response:
Generator path change is valid and properly implemented
The change from hack/actions/gen/main.go to hack/docker/gen/main.go is correctly implemented. The new generator is actively being used to generate Dockerfiles and workflow files across the repository, as evidenced by the references in multiple Dockerfile headers and the generator's implementation in hack/docker/gen/main.go.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of the new workflow generator
# Check if the new generator exists and contains workflow generation logic
rg -A 10 "func.*[Gg]enerate.*[Ww]orkflow" hack/docker/gen/main.go
# Check if there are any references to the old generator path
rg "hack/actions/gen/main.go" -l
Length of output: 124
Script:
#!/bin/bash
# Check the content of the generator file to understand the implementation
cat hack/docker/gen/main.go
# Check if there are any other workflow generator files
fd -e go -x rg -l "func.*[Gg]enerate.*[Ww]orkflow" {}
Length of output: 36573
Script:
#!/bin/bash
# Check if there are any other references to the old path in the codebase
rg "hack/actions/gen" -l
# Check if there are any references to the new path
rg "hack/docker/gen" -l
# Check if there are any other workflow generator files
fd -e yaml -x rg -l "DO_NOT_EDIT.*generated by.*hack" {}
Length of output: 1145
.github/workflows/dockers-buildbase-image.yaml (5)
31-32: LGTM: Enhanced version tag pattern support
The addition of v*.*.* and v*.*.*-* patterns improves version tag handling by supporting both regular and pre-release versions.
Line range hint 74-80: LGTM: Well-structured job configuration
The job configuration follows best practices:
- Uses reusable workflow for maintainability
- Supports multiple platforms (linux/amd64,linux/arm64)
- Properly inherits secrets
17-17: Verify the workflow generator location change
The workflow generator has moved from hack/actions/gen/main.go to hack/docker/gen/main.go, suggesting an architectural reorganization.
✅ Verification successful
Workflow generator location is correctly referenced
The workflow generator has been properly moved to hack/docker/gen/main.go, and this new location is consistently referenced across all Docker-related workflow files. The generator exists, is actively used, and all workflow files are using the correct path in their DO_NOT_EDIT headers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the workflow generator exists and is being used
# Check if the old and new generator paths exist
fd -t f "main.go" hack/docker/gen hack/actions/gen
# Check for any remaining references to the old path
rg "hack/actions/gen/main.go" -t yaml
Length of output: 175
Script:
#!/bin/bash
# Let's verify if the workflow generator exists and check its content
# First, confirm the new generator exists and check its purpose
if [ -f "hack/docker/gen/main.go" ]; then
echo "=== Content of hack/docker/gen/main.go ==="
head -n 20 hack/docker/gen/main.go
fi
# Check for any other workflow files referencing this generator
echo -e "\n=== Checking other workflow files referencing the generator ==="
rg "DO_NOT_EDIT.*generated by.*hack/docker/gen/main.go" .github/workflows/
Length of output: 5299
35-38: Improved trigger path configuration
The path configuration has been refined to:
- Exclude test and mock files to prevent unnecessary builds
- Focus on buildbase-related files
- Include essential dependency files (go.mod, go.sum)
This makes the workflow more efficient and focused.
Also applies to: 44-45, 51-52
✅ Verification successful
Path configuration is correctly set for buildbase workflow
The path configuration in the workflow file is correctly set up and efficient:
- The
dockers/buildbase/directory exists and contains the Dockerfile - The exclusion patterns for test and mock files are correctly configured
- The included paths cover all necessary dependencies (go.mod, go.sum, apis/, internal/)
- The workflow is properly focused on buildbase-related changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the paths exist and are correctly configured
# Check if the specified paths exist
fd -t f . cmd/buildbase dockers/buildbase pkg/buildbase
# Verify no unintended test files are included
rg -g "!{_test.go,_mock.go}" -l . cmd/buildbase pkg/buildbase
Length of output: 378
Script:
#!/bin/bash
# Let's verify the actual structure and content related to buildbase
# First, find all files related to buildbase
fd buildbase
# Then, check the content of the workflow file to understand the paths being monitored
cat .github/workflows/dockers-buildbase-image.yaml
# Look for any Go files that might be related to buildbase
fd -e go . internal
Length of output: 26585
53-72: Security consideration: Verify pull_request_target usage
The pull_request_target configuration mirrors the pull_request paths, which is good for security. However, ensure that the workflow using pull_request_target doesn't check out or execute untrusted PR code without proper safeguards.
✅ Verification successful
Let me gather more information about the workflow files to verify the security considerations.
Let me check the content of the docker-build action since it's being used in the workflow.
Let me check the determine-docker-image-tag action since it's being used in the docker-build action.
Based on the comprehensive analysis of the workflow files and actions, I can now provide the final response:
Security review of pull_request_target usage is complete - Implementation is secure
The workflow implementation follows security best practices for pull_request_target:
- The workflow only builds on
pull_request_targetwhen the PR has the 'ci/approved' label - The checkout action uses the explicit ref from the PR head SHA
- The docker build process is properly isolated and doesn't execute untrusted code
- All actions used are either official actions or local composite actions with safe implementations
- The tag determination and build processes have appropriate controls for PR builds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in the reusable workflow
# Verify the reusable workflow's handling of pull_request_target
rg -A 10 "pull_request_target" .github/workflows/_docker-image.yaml
Length of output: 545
Script:
#!/bin/bash
# Check the checkout action usage in both workflow files
echo "=== Checking dockers-buildbase-image.yaml ==="
rg -A 5 "actions/checkout" .github/workflows/dockers-buildbase-image.yaml
echo -e "\n=== Checking _docker-image.yaml ==="
rg -A 5 "actions/checkout" .github/workflows/_docker-image.yaml
# Check if there are any script executions or other potentially unsafe operations
echo -e "\n=== Checking for script executions in dockers-buildbase-image.yaml ==="
rg "run:|uses:" .github/workflows/dockers-buildbase-image.yaml
echo -e "\n=== Checking for script executions in _docker-image.yaml ==="
rg "run:|uses:" .github/workflows/_docker-image.yaml
Length of output: 1879
Script:
#!/bin/bash
# Check the docker-build action implementation
echo "=== Checking docker-build action ==="
cat .github/actions/docker-build/action.yaml
# Check if there are any other references to PR content
echo -e "\n=== Checking PR content references ==="
rg -A 3 "github.event.pull_request" .github/workflows/_docker-image.yaml
Length of output: 6502
Script:
#!/bin/bash
# Check the determine-docker-image-tag action implementation
echo "=== Checking determine-docker-image-tag action ==="
cat .github/actions/determine-docker-image-tag/action.yaml
# Also check the dump-context action
echo -e "\n=== Checking dump-context action ==="
cat .github/actions/dump-context/action.yaml
Length of output: 6837
example/client/go.mod (1)
11-18: LGTM! Dependency versions are properly updated and pinned
The dependency updates are well-structured with specific version pins, which is good practice for maintaining reproducible builds.
Also applies to: 26-26, 32-32, 37-38, 40-41
.github/workflows/dockers-example-client-image.yaml (4)
29-30: LGTM! Improved version tag patterns
The addition of v*.*.* and v*.*.*-* patterns ensures better semantic versioning support.
33-36: LGTM! Optimized workflow triggers
Good addition of exclusions for mock files, test files, and internal directories. This helps avoid unnecessary workflow runs and improves CI efficiency.
Also applies to: 54-57
17-17: Verify the workflow generator path change
The comment indicates the workflow generator has moved from hack/actions/gen/main.go to hack/docker/gen/main.go. This aligns with the PR's objective to refactor workflow generation logic.
39-49: Verify path trigger changes impact
The updated path triggers now include Makefile-related files and exclude certain internal paths. Let's verify these changes don't miss important triggers.
Also applies to: 60-70
✅ Verification successful
Let me gather more information about the workflow file to understand the purpose and context better.
Let me check the content of the referenced Dockerfile and the reusable workflow to fully understand the build context.
Path triggers are correctly configured for example-client image build
The current path triggers are appropriate for the example-client Docker image build workflow because:
- They include all necessary files that affect the build:
- The Dockerfile and its context (
dockers/example/client/Dockerfile,example/client/**) - Build configuration files (
Makefile,Makefile.d/**) - Proto/gRPC APIs that the client depends on (
apis/grpc/**,apis/proto/**) - Core dependencies (
go.mod,go.sum) - Build tooling (
hack/actions/gen/main.go,hack/docker/gen/main.go)
- The Dockerfile and its context (
- The exclusion of internal paths (
!internal/db/**,!internal/k8s/**) is correct as they don't affect the example client build - The workflow file itself and its dependencies (
.github/workflows/_docker-image.yaml) are properly included
While there are many other Dockerfiles in the repository, they are for different components and don't need to be included in this specific workflow's triggers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any potential Docker-related files that might be missed by the new triggers
echo "Searching for Docker-related files that might need to be included in triggers:"
fd -t f "Dockerfile|docker-compose" --exclude dockers/example/client/
Length of output: 1102
Script:
#!/bin/bash
# Check the workflow file content to understand its specific purpose
cat .github/workflows/dockers-example-client-image.yaml
# Also check if there are related workflow files
fd workflow .github/workflows/ --exec grep -l "docker" {}
Length of output: 2367
Script:
#!/bin/bash
# Check the example client Dockerfile
cat dockers/example/client/Dockerfile
# Check the reusable workflow
cat .github/workflows/_docker-image.yaml
# Check if there are any dependencies in the Makefile that affect the Docker build
cat Makefile | grep -A 5 "example-client"
Length of output: 8383
.github/workflows/dockers-benchmark-job-image.yaml (2)
17-17: LGTM: Improved tag pattern coverage
The addition of v*.*.* and v*.*.*-* patterns ensures better version tag handling, covering both regular releases and pre-releases with the v prefix.
Also applies to: 29-29
33-35: LGTM: Optimized build triggers
Good practice to exclude test files (*_test.go), mocks (*_mock.go), and internal DB files from triggering unnecessary builds.
charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml (1)
69-89: LGTM: Enhanced schema validation
Good improvements to the schema validation:
- Added explicit type definitions
- Added proper constraints for numeric fields
- Required fields are properly specified
hack/helm/schema/crd/main.go (1)
31-31: LGTM: Upgraded to official Kubernetes YAML library
Good change to use sigs.k8s.io/yaml instead of gopkg.in/yaml.v2. This library is:
- Official Kubernetes YAML implementation
- Better maintained
- More compatible with Kubernetes types
charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (2)
Line range hint 73-90: Well-structured schema validation for client configuration
The schema properly defines validation rules for the client configuration, including:
- Array type validation for addresses
- Comprehensive backoff configuration with proper typing
- Proper numeric bounds for retry counts
Line range hint 377-601: Well-defined health check configuration
The health check configuration follows Kubernetes best practices with:
- Proper separation of liveness, readiness, and startup probes
- Well-defined probe parameters (timeouts, thresholds)
- Proper validation for all numeric fields
go.mod (1)
297-309: Verify compatibility with security-related package updates
Several security-sensitive packages have been updated:
- golang.org/x/crypto v0.30.0
- golang.org/x/net v0.32.0
- golang.org/x/sys v0.28.0
✅ Verification successful
Security package updates are safe to proceed
The updated versions are safe and compatible with the codebase:
golang.org/x/crypto v0.30.0: Well above the last known vulnerable version (v0.17.0, Terrapin attack)golang.org/x/net v0.32.0: Limited usage, only for HTTP/2 functionality in server and client packagesgolang.org/x/sys v0.28.0: Used appropriately for OS-specific operations in control packages
The codebase shows proper usage patterns and the updates maintain backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any breaking changes or security advisories in the updated packages
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/crypto") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check for breaking changes in imported packages
rg -l "golang.org/x/(crypto|net|sys)" | xargs -I {} rg "^import\s+\([^)]*\)" {}
Length of output: 1806
Script:
#!/bin/bash
# Let's check for actual usage of these security-sensitive packages in the codebase
# Check for direct imports and usage of crypto package
echo "=== Crypto Usage ==="
rg "golang.org/x/crypto" -B 2 -A 2
echo -e "\n=== Net Usage ==="
rg "golang.org/x/net" -B 2 -A 2
echo -e "\n=== Sys Usage ==="
rg "golang.org/x/sys" -B 2 -A 2
# Check go.sum for previous versions
echo -e "\n=== Previous Versions ==="
grep -E "golang.org/x/(crypto|net|sys)" go.sum
Length of output: 10962
CHANGELOG.md (1)
Line range hint 1-2769: LGTM! The changelog format and content look good.
The changelog follows a consistent format across versions, with proper categorization of changes, links to PRs, and clear documentation of Docker images and references. The version history is well maintained.
.github/workflows/dockers-index-operator-image.yaml (2)
28-30: Improved workflow triggers and security
Good improvements:
- Added support for pre-release tags with
v*.*.*-*pattern - Added exclusions for internal test/mock files to prevent unnecessary builds
- Added exclusion for database files to prevent builds on internal changes
Also applies to: 33-35
39-40: Verify Makefile dependencies
The addition of Makefile triggers suggests build process dependencies. Please ensure that changes to these files actually affect the Docker build process.
Also applies to: 60-61
✅ Verification successful
Let me gather more information about the Docker build process and its dependencies.
Let me check the Makefile targets and their dependencies to complete the verification.
Based on the analysis of the Dockerfile and Makefile targets, I can now provide a final response:
Makefile dependencies are correctly configured
The Makefile dependencies for the Docker build process are properly set up. The index-operator image build:
- Uses the correct Dockerfile at
dockers/index/operator/Dockerfile - Has proper make targets in
Makefile.d/docker.mk - Follows the standard build process with necessary build arguments and configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Makefile for docker-related targets
rg -A 5 "docker.*index-operator" Makefile Makefile.d/
Length of output: 1255
Script:
#!/bin/bash
# Check the Dockerfile and its dependencies
fd "Dockerfile" dockers/index/operator/ -x cat {}
# Check if there are any includes or references to Makefile in the docker build
rg -l "Makefile" dockers/index/operator/
Length of output: 3504
Script:
#!/bin/bash
# Check the main Makefile for includes and docker-related targets
cat Makefile
# Check the specific docker make targets
cat Makefile.d/docker.mk
# Look for any make targets that might be used during the docker build
rg "^[a-zA-Z0-9_-]+:" Makefile.d/
Length of output: 43378
.github/workflows/dockers-manager-index-image.yaml (1)
35-36: Good additional exclusion for k8s internals
The addition of !internal/k8s/** exclusion is appropriate for preventing builds on internal Kubernetes implementation changes.
.github/workflows/dockers-index-deletion-image.yaml (4)
17-17: LGTM: Source path update aligns with refactoring objectives
The change in the generator source path from actions/gen to docker/gen better reflects the specific purpose of this workflow file.
29-30: LGTM: Improved version tag patterns
The addition of v*.*.* and v*.*.*-* patterns provides more precise semantic version matching for tags.
53-74: LGTM: Secure pull_request_target configuration
The addition of pull_request_target with identical path configurations is a good security practice, allowing safe handling of pull requests from forks while maintaining consistent build triggers.
33-36: LGTM: Well-structured path configurations
The path configurations are well thought out:
- Excludes test-related files to prevent unnecessary builds
- Includes essential build configuration files
- Properly scoped to index deletion components
Let's verify the path patterns match actual files in the repository:
Also applies to: 40-41, 44-45, 51-51
✅ Verification successful
Path patterns correctly match repository structure
The verification confirms that:
- Mock file exclusions (
!internal/**/*_mock.go) correctly match multiple mock files across internal packages - Index deletion component paths are valid, covering both cmd and pkg hierarchies:
cmd/index/job/deletion/: Contains main.go and configurationpkg/index/job/deletion/: Contains core implementation files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of critical paths and potential matches
echo "Checking for mock files that should be excluded:"
fd -t f "_mock.go$" internal/
echo "Checking for index deletion related files:"
fd -t f . cmd/index/job/deletion/
fd -t f . pkg/index/job/deletion/
Length of output: 1267
.github/workflows/dockers-index-correction-image.yaml (1)
17-17: LGTM: Consistent workflow structure with appropriate component-specific paths
The workflow maintains the same well-structured format as the index-deletion workflow, with appropriate path adjustments for the index-correction component. This consistency across workflows is excellent for maintainability.
Also applies to: 44-45, 51-51, 66-67, 73-73
.github/workflows/dockers-dev-container-image.yaml (1)
46-61: Review security implications of pull_request_target
The workflow uses pull_request_target which runs with repository secrets. While the paths are well-defined, ensure that the reusable workflow ./.github/workflows/_docker-image.yaml has proper security measures for untrusted PR inputs.
Consider implementing these security best practices:
- Validate PR inputs before using them in builds
- Use environment protection rules for production deployments
- Consider using
pull_requestinstead if repository secrets aren't needed
✅ Verification successful
Let me gather more information about the security measures in place.
Security measures for pull_request_target are properly implemented
The workflow has robust security measures in place:
- Only runs on forks when
pull_request_targetevent has theci/approvedlabel - For non-fork PRs, uses regular
pull_requestevent - Explicit checkout of PR head SHA with proper ref specification
- Git safe directory configuration is set
- Docker image scanning for tagged releases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the reusable workflow has proper security checks
rg -A 10 "pull_request_target" .github/workflows/_docker-image.yaml
Length of output: 545
Script:
#!/bin/bash
# Check the complete workflow file for security measures
cat .github/workflows/_docker-image.yaml
Length of output: 4523
.github/workflows/dockers-ci-container-image.yaml (1)
Line range hint 63-67: LGTM: Platform constraint is appropriate
The platform constraint linux/amd64 is correctly specified for the CI container build.
.github/workflows/dockers-binfmt-image.yaml (2)
35-38: Effective use of path exclusions
The negative path patterns effectively exclude test files and internal implementations from triggering builds. This is a good practice to reduce unnecessary builds.
Also applies to: 54-57
Line range hint 76-80: LGTM: Appropriate platform configuration
The multi-platform support (linux/amd64,linux/arm64) is correctly configured for the binfmt image, which is essential for cross-platform compatibility.
.github/workflows/dockers-buildkit-image.yaml (1)
53-74: Review security implications of pull_request_target
The pull_request_target event can pose security risks as it runs with repository secrets on PR branches. Since this workflow involves Docker builds, ensure that the _docker-image.yaml workflow has proper security measures in place when handling external contributions.
Consider implementing additional security measures:
- Add explicit checks for fork PRs
- Limit access to secrets for untrusted PRs
- Add manual approval steps for external contributions
✅ Verification successful
Let me gather more information about the security measures in place.
Security measures for pull_request_target are properly implemented
The workflow has adequate security measures in place:
- Only allows
pull_request_targetfrom forks when explicitly labeled with 'ci/approved' - Combines fork check with label requirement:
github.event.pull_request.head.repo.fork == true && contains(github.event.pull_request.labels.*.name, 'ci/approved') - Non-fork PRs are handled via regular
pull_requestevent - Docker credentials are only accessible when these conditions are met
- Image scanning is performed for tagged releases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if _docker-image.yaml has proper security checks
rg -A 5 "pull_request_target" .github/workflows/_docker-image.yaml
Length of output: 408
Script:
#!/bin/bash
# Check for additional security measures in the workflow
rg -A 10 "permissions:" .github/workflows/_docker-image.yaml
# Check if there are any explicit checks for untrusted PRs or forks
rg -A 5 "github.event.pull_request.head.repo.full_name" .github/workflows/_docker-image.yaml
# Check the complete job structure to understand the workflow
cat .github/workflows/_docker-image.yaml
Length of output: 4677
.github/workflows/dockers-gateway-mirror-image.yaml (1)
39-40: Verify Makefile dependencies
The addition of Makefile and Makefile.d/** paths suggests build configuration dependencies. Ensure that changes to these files should indeed trigger rebuilds of the gateway-mirror image.
✅ Verification successful
Let me gather more context about the Makefile dependencies to understand the build process better.
Let me check the Dockerfile to understand the build dependencies better.
Makefile dependencies are correctly configured
The inclusion of Makefile and Makefile.d/** paths in the workflow is justified because:
- The gateway-mirror image build process relies on multiple Makefile targets and dependencies
- The Dockerfile uses a multi-stage build that depends on make commands for Go installation, downloading dependencies, and building the binary
- The main Makefile includes all Makefile.d/* files which contain shared build configurations and docker build targets
- The docker/build/gateway-mirror target in Makefile.d/docker.mk is essential for building the image
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Makefile for gateway-mirror specific targets
rg -p "gateway-mirror" Makefile Makefile.d/**
Length of output: 625
Script:
#!/bin/bash
# Check the gateway-mirror docker build target implementation
rg -A 5 "^docker/build/gateway-mirror:" Makefile.d/docker.mk
# Check if there are any other Makefile targets that depend on gateway-mirror
rg "gateway-mirror.*:" Makefile.d/**
Length of output: 698
Script:
#!/bin/bash
# Check the Dockerfile contents to understand build dependencies
cat dockers/gateway/mirror/Dockerfile
# Check if there are any make includes or dependencies in the main Makefile
rg "^include.*Makefile\.d" Makefile
Length of output: 3911
.github/workflows/dockers-loadtest-image.yaml (1)
Line range hint 77-79: Verify platform limitation
The workflow specifies only linux/amd64 platform, while other workflows (e.g., buildkit) support multiple platforms. Verify if this limitation is intentional.
✅ Verification successful
Platform limitation is intentional and correct
Based on the analysis of the codebase:
- The loadtest Dockerfile shows it's built using
TARGETARCHandTARGETOSvariables, indicating multi-platform build capability. - However, the workflow specifically limits to
linux/amd64platform, which aligns with other testing/CI-related workflows likedockers-ci-container-image.yamlthat also use onlylinux/amd64. - Only the core infrastructure components (buildkit, buildbase, binfmt) support multiple platforms (
linux/amd64,linux/arm64), which is the correct approach as these are foundational components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if loadtest tool has platform-specific code
rg -p "GOARCH|GOOS|runtime.GOARCH|runtime.GOOS" cmd/tools/cli/loadtest/**
Length of output: 151
Script:
#!/bin/bash
# Let's check the Docker-related files and workflows for platform specifications
rg -p "platform.*amd64|PLATFORM.*amd64" .github/workflows/
Length of output: 881
Script:
#!/bin/bash
# Check if there are any platform-specific build configurations in the loadtest tool
fd Dockerfile --exec grep -l "FROM.*amd64\|ARG TARGETPLATFORM" {}
Length of output: 69
Script:
#!/bin/bash
# Check the loadtest Dockerfile content
fd Dockerfile --exec cat {} | grep -A 5 -B 5 "FROM"
Length of output: 21646
.github/workflows/dockers-gateway-lb-image.yaml (2)
29-29: Improved workflow trigger configuration
Good improvements to the workflow configuration:
- Added support for versioned tags with suffixes (
v*.*.*-*) - Added exclusions for internal mock/test files and specific internal directories
These changes will help reduce unnecessary workflow runs and better handle version tags.
Also applies to: 33-36
40-41: Verify Makefile trigger dependencies
The addition of Makefile-related paths is good for catching build configuration changes. However, we should verify if there are any other Makefile-related paths that should be included.
✅ Verification successful
After analyzing the Makefile and the workflow file, I can now provide a final response since I have enough information to verify the Makefile trigger dependencies.
The Makefile trigger paths are correctly configured
The current configuration in the workflow file includes:
MakefileMakefile.d/**
This covers all the relevant Makefile paths in the repository:
- The root
Makefile - All included makefiles under
Makefile.d/directory which contain various build configurations
No additional Makefile paths need to be included since the only other Makefile found (internal/core/algorithm/ngt/Makefile) is specifically for NGT benchmarking and is not related to the gateway-lb build process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for additional Makefile-related paths that might need inclusion
fd -t f "Makefile" | grep -v "node_modules\|vendor"
Length of output: 97
Script:
#!/bin/bash
# Let's check the content and dependencies of the NGT Makefile to understand if it's related to the gateway-lb build
cat internal/core/algorithm/ngt/Makefile
Length of output: 842
Script:
#!/bin/bash
# Let's check the content of the root Makefile to understand its relationship with gateway-lb
cat Makefile
Length of output: 30405
.github/workflows/dockers-agent-ngt-image.yaml (2)
33-35: Consistent path exclusions across workflows
The path exclusions for internal files match the pattern established in other workflows, which is good for maintainability.
52-53: Verify NGT version dependencies
The workflow correctly includes both GO_VERSION and NGT_VERSION as triggers. This ensures builds are triggered when either runtime version changes.
✅ Verification successful
Let me verify the consistency of version dependencies across the codebase by checking other NGT-related workflows and Dockerfiles.
Let me check the Dockerfile for the NGT agent to understand how these versions are used in the build process.
Let me check the Makefile targets to understand how these versions are used in the build process.
Version dependencies are correctly configured and used
The verification confirms that:
- Both GO_VERSION and NGT_VERSION are properly referenced in the workflow file
- These versions are correctly used in the build process:
- GO_VERSION is used for installing Go toolchain
- NGT_VERSION is used for cloning and building NGT from source
- The dependencies are managed through version files that are automatically updated via Makefile targets
- The Docker build process correctly incorporates both versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify version file references are consistent across NGT-related workflows
rg "versions/(GO|NGT)_VERSION" .github/workflows/dockers-agent-ngt-image.yaml
Length of output: 193
Script:
#!/bin/bash
# Search for NGT-related workflows and Dockerfiles
fd -e yaml -e yml -e Dockerfile . | rg -i "ngt|go"
# Then check version references in those files
rg "versions/(GO|NGT)_VERSION" .
Length of output: 897
Script:
#!/bin/bash
# Find and check the NGT agent Dockerfile
fd Dockerfile . | rg -i "ngt"
cat $(fd Dockerfile . | rg -i "ngt")
Length of output: 3653
Script:
#!/bin/bash
# Check Makefile targets related to NGT and GO versions
rg "ngt/install|go/install" Makefile Makefile.d/* -A 5
Length of output: 2416
.github/workflows/dockers-buildkit-syft-scanner-image.yaml (2)
Line range hint 74-77: Platform-specific build configuration maintained
The workflow correctly maintains its multi-platform build configuration while following the refactored pattern. Good preservation of workflow-specific requirements.
Line range hint 22-23: Verify cron schedule configuration
The workflow maintains its hourly schedule trigger, which is unique to this workflow. Consider if this frequency is still appropriate for the refactored workflow.
✅ Verification successful
Hourly schedule is consistent with related Docker image workflows
The hourly cron schedule (0 * * * *) in this workflow aligns with other Docker image build workflows:
- dockers-buildkit-image.yaml
- dockers-buildbase-image.yaml
- dockers-binfmt-image.yaml
This consistency is appropriate as these workflows are related to Docker image building and should run on the same schedule.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other scheduled workflows to ensure consistency in scheduling patterns
rg "cron:" .github/workflows/
Length of output: 561
.github/workflows/dockers-gateway-filter-image.yaml (1)
33-36: LGTM: Good practice excluding test files and internal implementations
The exclusion of mock files, test files, and internal implementations helps reduce unnecessary workflow runs.
.github/workflows/dockers-agent-faiss-image.yaml (1)
51-54:
Remove duplicate version file references
The FAISS_VERSION path appears twice in the configuration.
- pkg/agent/core/faiss/**
- pkg/agent/internal/**
- versions/FAISS_VERSION
- - versions/FAISS_VERSION
- versions/GO_VERSIONLikely invalid or redundant comment.
.github/workflows/dockers-agent-image.yaml (1)
17-17: Verify the workflow generator implementation
The comment indicates these files are generated by hack/docker/gen/main.go. Let's verify if the generator is handling paths correctly to prevent duplicates.
Also applies to: 42-43
✅ Verification successful
Based on the generator code analysis, I can now provide a final response:
Generator implementation is handling paths correctly
The workflow generator correctly handles paths and prevents duplicates through:
- Explicit path definitions for each component type
- Proper sorting and deduplication of paths using
slices.Sort - Clear separation between common and component-specific paths
- Consistent path handling for different container types (Go, Rust, HelmOperator, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the workflow generator implementation for path handling
# Look for the path handling logic in the generator
rg -A 10 "versions/" hack/docker/gen/main.go
# Check if there are other workflow files with similar issues
fd -e yaml -e yml . .github/workflows/ --exec rg -l "versions/.*\n.*versions/.*same-version"
Length of output: 12811
Script:
#!/bin/bash
# Let's try a simpler approach to check the generator implementation
# First, check if the generator file exists
ls -l hack/docker/gen/main.go
# Look for path-related code in the generator
rg -U "path" hack/docker/gen/main.go
# Check for any version-related patterns
rg -U "version" hack/docker/gen/main.go
# Look at the file content directly
cat hack/docker/gen/main.go
Length of output: 37391
.github/workflows/dockers-benchmark-operator-image.yaml (3)
29-30: LGTM: Improved version tag patterns
The addition of the 'v' prefix to version tags (v*.*.* and v*.*.*-*) improves specificity and better aligns with semantic versioning conventions.
56-76: LGTM: Consistent path triggers
The pull_request_target paths correctly mirror the pull_request paths, maintaining consistency between both trigger types.
17-17: Verify the workflow generator's new location
The generator path has been moved from hack/actions/gen/main.go to hack/docker/gen/main.go.
✅ Verification successful
Let me verify if this is a legitimate move of the generator by checking its git history and content.
Let me check if there are any references to the old generator path and search for workflow generation functionality in both locations to ensure this is a proper migration.
Based on the evidence gathered, I can now provide the final response:
Generator path change is valid and properly implemented
The new generator at hack/docker/gen/main.go exists and is actively being used to generate multiple Dockerfiles across the project. The old path hack/actions/gen/ no longer exists, indicating a complete migration. The new generator path is consistently referenced in generated files and build configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the workflow generator at the new location
# and check if the old location still exists to ensure proper migration
echo "Checking new generator location..."
if [ -f "hack/docker/gen/main.go" ]; then
echo "New generator exists"
else
echo "New generator not found"
fi
echo "Checking old generator location..."
if [ -f "hack/actions/gen/main.go" ]; then
echo "Warning: Old generator still exists"
fi
Length of output: 283
Script:
#!/bin/bash
# Check git history of the generator file and its content
echo "=== Git history of the generator ==="
git log --oneline -- hack/docker/gen/main.go
echo -e "\n=== Generator content ==="
head -n 20 hack/docker/gen/main.go
echo -e "\n=== Search for related workflow generation code ==="
rg -l "workflow" hack/docker/gen/
Length of output: 1024
Script:
#!/bin/bash
# Search for references to both old and new generator paths
echo "=== References to old generator path ==="
rg "hack/actions/gen/main.go" -A 2
echo -e "\n=== References to new generator path ==="
rg "hack/docker/gen/main.go" -A 2
echo -e "\n=== Check workflow generation code in both locations ==="
echo "Old location content (if exists):"
ls -la hack/actions/gen/ 2>/dev/null || echo "Directory doesn't exist"
echo -e "\nNew location workflow generation code:"
rg "func.*workflow" hack/docker/gen/main.go || echo "No direct workflow functions found"
Length of output: 9021
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (21)
.github/workflows/dockers-dev-container-image.yaml (1)
35-45: Validate the expanded PR trigger pathsThe additions of
Makefile,Makefile.d/**, and Docker generation related paths provide better coverage for relevant changes.Consider using path-filters action for complex path rules to improve maintainability.
k8s/operator/helm/crds/valdhelmoperatorrelease.yaml (2)
117-136: Consider expanding logging level optionsWhile the current logging levels (debug, info, error) cover basic needs, consider adding more granular levels such as 'warn' and 'trace' to align with common logging frameworks and provide better debugging capabilities.
level: enum: - debug + - trace - info + - warn - error type: string
214-220: Consider adding automountServiceAccountToken fieldFor enhanced security, consider adding the
automountServiceAccountTokenboolean field to allow explicit control over token mounting.properties: create: type: boolean name: type: string + automountServiceAccountToken: + type: boolean + description: "Automatically mount the service account token"example/client/go.mod.default (1)
Line range hint
4-21: Consider using fixed versions instead of 'latest' in replace directivesUsing 'latest' in replace directives can lead to reproducibility issues as builds may use different versions at different times. Consider using fixed versions for better reproducibility and stability.
.github/workflows/dockers-ci-container-image.yaml (1)
32-60: Consider using YAML anchors to reduce duplicationThe paths under
pull_requestandpull_request_targetare identical. Consider using YAML anchors and aliases to reduce duplication and maintain consistency.on: # ... other triggers ... pull_request: &pr_paths paths: - .github/actions/docker-build/action.yaml - .github/workflows/_docker-image.yaml - .github/workflows/dockers-ci-container-image.yaml - Makefile - Makefile.d/** - apis/grpc/** - apis/proto/** - cmd/ci/base/** - dockers/ci/base/Dockerfile - hack/** - hack/docker/gen/main.go - pkg/ci/base/** pull_request_target: paths: *pr_pathsexample/client/go.mod (1)
14-16: Consider using released versions instead of dated snapshotsUsing dated snapshots (
20241206012308) for google.golang.org/genproto and its subpackages could make builds less reproducible. Consider using released versions if available..github/workflows/dockers-binfmt-image.yaml (1)
35-38: Reduce path configuration duplicationThe path patterns are duplicated between
pull_requestandpull_request_targetevents. Consider extracting these into a reusable anchor using YAML aliases to improve maintainability.Example refactor:
# At the top of the file path_filters: &path_filters paths: - "!internal/**/*_mock.go" - "!internal/**/*_test.go" - "!internal/db/**" - "!internal/k8s/**" # ... rest of the paths pull_request: <<: *path_filters pull_request_target: <<: *path_filtersAlso applies to: 54-58
.github/workflows/dockers-discoverer-k8s-image.yaml (1)
33-35: Verify completeness of internal file exclusionsThe exclusions for mock, test, and internal DB files are good security practices. Consider adding:
internal/**/generated/**internal/config/**Also applies to: 54-56
.github/workflows/dockers-manager-index-image.yaml (1)
Line range hint
17-74: Standardize workflow configurationsThe changes across all three workflow files follow the same pattern, which is good for maintainability. However, consider:
- Creating a shared configuration file for common path exclusions
- Using workflow templates for standardizing the security controls
- Documenting the rationale for path inclusions/exclusions
Example approach for shared configuration:
# .github/workflow-config/path-filters.yaml common_exclusions: &common_exclusions - "!internal/**/*_mock.go" - "!internal/**/*_test.go" - "!internal/db/**" - "!internal/k8s/**".github/workflows/dockers-index-deletion-image.yaml (1)
Line range hint
17-74: Consider documenting the workflow generation architectureThe refactoring moves workflow generation from the actions to the docker directory, suggesting a more focused approach to Docker-specific workflow generation. Consider:
- Adding documentation about the new workflow generation architecture
- Creating a migration guide for any custom workflows
- Updating the contributing guidelines to reflect the new workflow structure
Would you like me to help create a documentation template for the new workflow generation architecture?
.github/workflows/dockers-loadtest-image.yaml (1)
53-74: Enhanced security with pull_request_targetThe addition of
pull_request_targetevent with carefully controlled paths improves security for external contributions while maintaining the same path restrictions as thepull_requestevent.Consider documenting these security improvements in the repository's contributing guidelines.
.github/workflows/dockers-readreplica-rotate-image.yaml (1)
33-35: Consistent security improvements across workflowsThe exclusion patterns for internal files and the addition of
pull_request_targetevent are consistently applied across all workflows, improving the overall security posture.Consider adding automated tests to verify these security patterns remain consistent as workflows evolve.
Also applies to: 54-56
.github/workflows/dockers-helm-operator-image.yaml (1)
17-17: Architectural improvements in workflow generationThe refactoring shows several positive architectural changes:
- Consistent path filtering across all workflows improves build efficiency
- Secure handling of internal files prevents unnecessary builds
- Standardized version tag patterns support pre-release versions
- Unified approach to chart and version dependencies
Consider documenting these patterns in a workflow generation guide to ensure consistency in future workflow additions.
Also applies to: 17-17, 17-17
charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml (3)
85-90: Consider adding validation constraints for image fields.While the type definitions are good, consider adding:
- Pattern validation for repository (e.g., valid docker image pattern)
- Pattern validation for tag (e.g., semver pattern)
repository: type: string + pattern: "^[a-z0-9]+(?:[._-][a-z0-9]+)*(?:/[a-z0-9]+(?:[._-][a-z0-9]+)*)*$" tag: type: string + pattern: "^v?(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$"
124-136: Add descriptions for logging level values.The enum values for logging levels are well-defined, but adding descriptions would help users understand the appropriate use case for each level.
level: enum: - debug - info - error type: string + description: | + Logging level: + - debug: Detailed information for debugging + - info: General operational information + - error: Error conditions that should be addressed
Line range hint
671-690: Consider adding validation for service ports.The service configuration should include validation for standard port ranges.
service: properties: enabled: type: boolean + port: + type: integer + minimum: 1 + maximum: 65535 + description: "The port number for the service" externalTrafficPolicy: type: string + enum: + - Local + - Cluster + description: "Denotes if this Service desires to route external traffic to node-local or cluster-wide endpoints"charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (1)
Line range hint
636-654: Enhance validation for gRPC interceptor configurations.The interceptor configuration could benefit from additional validation and documentation.
interceptors: items: enum: - RecoverInterceptor - AccessLogInterceptor - TraceInterceptor - MetricInterceptor type: string + description: | + Available interceptors: + - RecoverInterceptor: Recovers from panics + - AccessLogInterceptor: Logs access patterns + - TraceInterceptor: Adds tracing capabilities + - MetricInterceptor: Collects metrics + uniqueItems: true + maxItems: 4charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (1)
346-349: Add description for RPS limits.The RPS (Requests Per Second) configuration should include a description of its purpose and limitations.
rps: maximum: 65535 minimum: 0 type: integer + description: | + Maximum requests per second (RPS) for the benchmark job. + - 0: No limit + - 1-65535: RPS limit + default: 0go.mod (2)
Line range hint
3-3: Invalid Go version specifiedThe Go version
1.23.3is invalid. Go versions only use two components (e.g., 1.22) and the latest stable version is 1.22.1.Update the Go version:
-go 1.23.3 +go 1.22
Line range hint
354-357: Fix placeholder versions for multiple dependenciesSeveral dependencies are using placeholder versions (0.0.0-00010101000000-000000000000). This can lead to build reproducibility issues and should be fixed by specifying proper versions.
Affected dependencies:
- github.com/akrylysov/pogreb
- github.com/go-redis/redis/v8
- github.com/gocraft/dbr/v2
- github.com/gorilla/mux
- github.com/grafana/pyroscope-go/godeltaprof
- github.com/hashicorp/go-version
- github.com/kpango/fuid
- github.com/kpango/gache/v2
- github.com/kubernetes-csi/external-snapshotter/client/v6
- github.com/leanovate/gopter
- github.com/lucasb-eyer/go-colorful
- github.com/pierrec/lz4/v3
- github.com/scylladb/gocqlx
Please specify proper versions for these dependencies. For example:
-github.com/akrylysov/pogreb v0.0.0-00010101000000-000000000000 +github.com/akrylysov/pogreb v0.10.2Also applies to: 358-359, 360-361, 362-363, 364-365, 366-367, 368-369, 370-371, 372-373, 374-375, 376-377, 378-379, 382-383
CHANGELOG.md (1)
Line range hint
1987-2769: Suggestion: Consider adding release datesWhile the changelog is well structured, adding release dates for each version would provide helpful temporal context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (24)
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.goexample/client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumhack/actions/gen/main.gois excluded by!**/gen/**hack/actions/gen/main_test.gois excluded by!**/gen/**hack/docker/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (82)
.gitfiles(0 hunks).github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/ISSUE_TEMPLATE/security_issue_report.md(1 hunks).github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/coverage.yaml(1 hunks).github/workflows/dockers-agent-faiss-image.yaml(2 hunks).github/workflows/dockers-agent-image.yaml(2 hunks).github/workflows/dockers-agent-ngt-image.yaml(2 hunks).github/workflows/dockers-agent-sidecar-image.yaml(2 hunks).github/workflows/dockers-benchmark-job-image.yaml(2 hunks).github/workflows/dockers-benchmark-operator-image.yaml(2 hunks).github/workflows/dockers-binfmt-image.yaml(2 hunks).github/workflows/dockers-buildbase-image.yaml(2 hunks).github/workflows/dockers-buildkit-image.yaml(2 hunks).github/workflows/dockers-buildkit-syft-scanner-image.yaml(2 hunks).github/workflows/dockers-ci-container-image.yaml(2 hunks).github/workflows/dockers-dev-container-image.yaml(2 hunks).github/workflows/dockers-discoverer-k8s-image.yaml(2 hunks).github/workflows/dockers-example-client-image.yaml(2 hunks).github/workflows/dockers-gateway-filter-image.yaml(2 hunks).github/workflows/dockers-gateway-lb-image.yaml(2 hunks).github/workflows/dockers-gateway-mirror-image.yaml(2 hunks).github/workflows/dockers-helm-operator-image.yaml(2 hunks).github/workflows/dockers-index-correction-image.yaml(2 hunks).github/workflows/dockers-index-creation-image.yaml(2 hunks).github/workflows/dockers-index-deletion-image.yaml(2 hunks).github/workflows/dockers-index-deletion.yaml(0 hunks).github/workflows/dockers-index-operator-image.yaml(2 hunks).github/workflows/dockers-index-save-image.yaml(2 hunks).github/workflows/dockers-loadtest-image.yaml(2 hunks).github/workflows/dockers-manager-index-image.yaml(2 hunks).github/workflows/dockers-readreplica-rotate-image.yaml(2 hunks)CHANGELOG.md(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(0 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml(1 hunks)charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml(5 hunks)charts/vald/crds/valdmirrortarget.yaml(1 hunks)dockers/agent/core/agent/Dockerfile(1 hunks)dockers/agent/core/faiss/Dockerfile(1 hunks)dockers/agent/core/ngt/Dockerfile(1 hunks)dockers/agent/sidecar/Dockerfile(1 hunks)dockers/binfmt/Dockerfile(1 hunks)dockers/buildbase/Dockerfile(1 hunks)dockers/buildkit/Dockerfile(1 hunks)dockers/buildkit/syft/scanner/Dockerfile(1 hunks)dockers/ci/base/Dockerfile(1 hunks)dockers/dev/Dockerfile(1 hunks)dockers/discoverer/k8s/Dockerfile(1 hunks)dockers/example/client/Dockerfile(1 hunks)dockers/gateway/filter/Dockerfile(1 hunks)dockers/gateway/lb/Dockerfile(1 hunks)dockers/gateway/mirror/Dockerfile(1 hunks)dockers/index/job/correction/Dockerfile(1 hunks)dockers/index/job/creation/Dockerfile(1 hunks)dockers/index/job/deletion/Dockerfile(1 hunks)dockers/index/job/readreplica/rotate/Dockerfile(1 hunks)dockers/index/job/save/Dockerfile(1 hunks)dockers/index/operator/Dockerfile(1 hunks)dockers/manager/index/Dockerfile(1 hunks)dockers/operator/helm/Dockerfile(1 hunks)dockers/tools/benchmark/job/Dockerfile(1 hunks)dockers/tools/benchmark/operator/Dockerfile(1 hunks)dockers/tools/cli/loadtest/Dockerfile(1 hunks)example/client/go.mod(1 hunks)example/client/go.mod.default(1 hunks)go.mod(19 hunks)hack/helm/schema/crd/main.go(1 hunks)k8s/operator/helm/crds/valdhelmoperatorrelease.yaml(5 hunks)rust/rust-toolchain(1 hunks)versions/BUF_VERSION(1 hunks)versions/CMAKE_VERSION(1 hunks)versions/GOLANGCILINT_VERSION(1 hunks)versions/GO_VERSION(1 hunks)versions/HELM_VERSION(1 hunks)versions/K3S_VERSION(1 hunks)versions/KUBECTL_VERSION(1 hunks)versions/OPERATOR_SDK_VERSION(1 hunks)versions/PROMETHEUS_STACK_VERSION(1 hunks)versions/PROTOBUF_VERSION(1 hunks)
⛔ Files not processed due to max files limit (13)
- versions/REVIEWDOG_VERSION
- versions/RUST_VERSION
- versions/TELEPRESENCE_VERSION
- versions/USEARCH_VERSION
- versions/YQ_VERSION
- versions/actions/ACTIONS_CACHE
- versions/actions/CODECOV_CODECOV_ACTION
- versions/actions/GITHUB_CODEQL_ACTION_ANALYZE
- versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD
- versions/actions/GITHUB_CODEQL_ACTION_INIT
- versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF
- versions/actions/GITHUB_ISSUE_METRICS
- versions/actions/REVIEWDOG_ACTION_HADOLINT
💤 Files with no reviewable changes (3)
- Makefile.d/functions.mk
- .github/workflows/dockers-index-deletion.yaml
- .gitfiles
✅ Files skipped from review due to trivial changes (42)
- versions/BUF_VERSION
- rust/rust-toolchain
- versions/HELM_VERSION
- versions/GOLANGCILINT_VERSION
- versions/CMAKE_VERSION
- versions/KUBECTL_VERSION
- dockers/binfmt/Dockerfile
- versions/PROMETHEUS_STACK_VERSION
- versions/K3S_VERSION
- dockers/index/job/readreplica/rotate/Dockerfile
- dockers/gateway/mirror/Dockerfile
- dockers/index/job/creation/Dockerfile
- dockers/example/client/Dockerfile
- dockers/gateway/lb/Dockerfile
- dockers/index/operator/Dockerfile
- dockers/tools/benchmark/job/Dockerfile
- dockers/index/job/correction/Dockerfile
- versions/PROTOBUF_VERSION
- Makefile
- dockers/index/job/save/Dockerfile
- versions/GO_VERSION
- dockers/index/job/deletion/Dockerfile
- dockers/dev/Dockerfile
- dockers/buildkit/Dockerfile
- .github/ISSUE_TEMPLATE/security_issue_report.md
- dockers/buildkit/syft/scanner/Dockerfile
- hack/helm/schema/crd/main.go
- dockers/agent/core/ngt/Dockerfile
- dockers/buildbase/Dockerfile
- dockers/manager/index/Dockerfile
- dockers/agent/core/agent/Dockerfile
- dockers/discoverer/k8s/Dockerfile
- versions/OPERATOR_SDK_VERSION
- dockers/operator/helm/Dockerfile
- .github/ISSUE_TEMPLATE/bug_report.md
- dockers/tools/benchmark/operator/Dockerfile
- dockers/gateway/filter/Dockerfile
- dockers/ci/base/Dockerfile
- dockers/tools/cli/loadtest/Dockerfile
- dockers/agent/sidecar/Dockerfile
- .github/PULL_REQUEST_TEMPLATE.md
- dockers/agent/core/faiss/Dockerfile
🔇 Additional comments (51)
.github/workflows/dockers-dev-container-image.yaml (4)
Line range hint 63-67: LGTM: Reusable workflow implementation
The job configuration correctly uses the reusable workflow pattern with proper secret inheritance.
50-60: Ensure PR target paths match PR paths
The paths under pull_request_target exactly mirror those under pull_request, which is the correct security practice.
✅ Verification successful
Paths under pull_request and pull_request_target are correctly mirrored
After examining the workflow file, I can confirm that the paths listed under both pull_request and pull_request_target triggers are exactly identical, which is the correct security practice. The paths include:
.github/actions/docker-build/action.yaml.github/workflows/_docker-image.yaml.github/workflows/dockers-dev-container-image.yamlMakefileMakefile.d/**- And all other paths match exactly between both sections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify path consistency between PR and PR target triggers
# Expected: Paths should be identical
# Extract and compare paths
awk '/^ pull_request:$/,/^ pull_request_target:$/' .github/workflows/dockers-dev-container-image.yaml | grep " -" > /tmp/pr_paths
awk '/^ pull_request_target:$/,/^jobs:$/' .github/workflows/dockers-dev-container-image.yaml | grep " -" > /tmp/pr_target_paths
diff /tmp/pr_paths /tmp/pr_target_paths
Length of output: 277
Script:
#!/bin/bash
# Let's directly examine the workflow file content to verify the paths
cat .github/workflows/dockers-dev-container-image.yaml
Length of output: 1942
29-30: Review the new tag pattern additions
The addition of v*.*.* and v*.*.*-* patterns expands the workflow triggers to include versioned tags with pre-release suffixes.
✅ Verification successful
Let me check for the v*.*.* pattern as well to get a complete picture.
Tag patterns are consistent with other workflows
The addition of v*.*.* and v*.*.*-* patterns in the dev-container workflow aligns with the existing tag patterns used across other workflows in the repository. These patterns are consistently used in various workflows including e2e tests, helm deployments, and other docker image builds, indicating this is a standardized approach for version tagging in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential conflicts with other workflow triggers
# Expected: No overlapping tag patterns in other workflows
# Search for similar tag patterns in other workflow files
rg "v\*\.\*\.\*-\*" .github/workflows/ --type yaml
Length of output: 2236
Script:
#!/bin/bash
# Search for the base version tag pattern in workflow files
rg "v\*\.\*\.\*[^-]" .github/workflows/ --type yaml
Length of output: 3020
17-17: Verify the generator source change impact
The workflow generator has moved from actions/gen to docker/gen, which aligns with the PR's refactoring objectives.
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Generator source change is correctly implemented
The workflow generator has indeed moved from hack/actions/gen to hack/docker/gen, and the implementation shows that:
- The new generator exists in
hack/docker/gen/main.gowith comprehensive workflow generation logic - The generator includes proper template handling for both Dockerfile and GitHub Actions workflow generation
- The comment in the workflow file accurately reflects the source of generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of the new generator
# Expected: The new generator exists and the old one is removed
# Check if old generator is removed
if [ -f "hack/actions/gen/main.go" ]; then
echo "Error: Old generator still exists"
exit 1
fi
# Verify new generator exists and contains workflow generation logic
rg -A 5 "func.*[Ww]orkflow.*[Gg]en" hack/docker/gen/main.go
Length of output: 99
Script:
#!/bin/bash
# Let's verify the generator changes more thoroughly
# 1. Check if the old generator directory exists
if [ -d "hack/actions/gen" ]; then
echo "Old generator directory exists at hack/actions/gen"
fi
# 2. Check if the new generator directory exists
if [ -d "hack/docker/gen" ]; then
echo "New generator directory exists at hack/docker/gen"
fi
# 3. Search for workflow generation related code in both old and new locations
echo "=== Searching for workflow generation code ==="
rg -l "workflow" hack/docker/gen/
rg -l "github.com/vdaas/vald/.github/workflows" hack/docker/gen/
# 4. Check the actual content of main.go if it exists
if [ -f "hack/docker/gen/main.go" ]; then
echo "=== Content of hack/docker/gen/main.go ==="
cat hack/docker/gen/main.go
fi
Length of output: 37041
k8s/operator/helm/crds/valdhelmoperatorrelease.yaml (4)
85-90: LGTM: Well-structured image configuration schema
The image configuration schema is properly defined with appropriate types and enum values for pullPolicy that align with Kubernetes standards.
212-214: LGTM: Proper service type enumeration
The service type enumeration correctly includes all standard Kubernetes service types (ClusterIP, LoadBalancer, NodePort).
222-225: LGTM: Flexible tolerations configuration
The tolerations array is properly defined with preserved unknown fields, allowing for flexible node toleration configurations while maintaining schema validation.
228-228: Verify CRD upgrade impact
The schema changes might affect existing custom resources. Please ensure there's a migration plan or validation webhook to handle existing resources during the upgrade.
.github/workflows/dockers-ci-container-image.yaml (1)
17-17: Verify the existence and functionality of the new generator path
The workflow generator path has changed. Let's verify the new path exists and contains the expected logic.
✅ Verification successful
Generator path exists and contains workflow generation logic
The new generator path hack/docker/gen/main.go exists and contains comprehensive workflow generation logic for Docker container images. The file includes:
- Workflow template definitions and configuration
- Docker image build specifications
- Path management for different container types
- Environment variable handling
- Comprehensive build command generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new generator exists and is executable
if [ -f "hack/docker/gen/main.go" ]; then
echo "New generator exists"
# Check for workflow generation logic
rg -A 5 'func.*[Gg]enerate.*[Ww]orkflow' hack/docker/gen/main.go
else
echo "New generator not found"
fi
Length of output: 154
Script:
#!/bin/bash
# Let's check the content of the generator file to understand its purpose
cat hack/docker/gen/main.go
# Also check if there are any other generator files in the repository
fd -t f "main.go" -x rg -l "workflow" {}
Length of output: 36582
example/client/go.mod (1)
11-18: Verify compatibility with updated dependencies
Several core dependencies have been updated to new versions. Let's verify there are no breaking changes that could affect the codebase.
✅ Verification successful
Based on the analysis of the imports and go.mod files, I can now generate the final response:
Dependencies updated safely with no breaking changes
The updated dependencies are used in a compatible way throughout the codebase:
golang.org/x/crypto v0.30.0: No direct crypto package usage foundgolang.org/x/net v0.32.0: Used only for standard networking featuresgolang.org/x/text v0.21.0: No direct text package usage foundgoogle.golang.org/grpc v1.68.1: Used consistently for standard gRPC functionalitygoogle.golang.org/protobuf v1.35.2: Used only for protobuf encoding/decodinggoogle.golang.org/genproto: Used only for API and RPC types
The version updates appear to be routine maintenance updates without any breaking changes. The codebase uses stable APIs from these packages and the updates maintain backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any usage of deprecated or removed features
rg -l 'crypto\.(?:DEPRECATED|REMOVED)'
rg -l 'grpc\.(?:DEPRECATED|REMOVED)'
Length of output: 79
Script:
#!/bin/bash
# Let's check for actual usage of these dependencies and their features
echo "=== Checking crypto package usage ==="
rg -A 2 "crypto\."
echo -e "\n=== Checking grpc package usage ==="
rg -A 2 "grpc\."
echo -e "\n=== Checking protobuf package usage ==="
rg -A 2 "protobuf\."
echo -e "\n=== Checking genproto package usage ==="
rg -A 2 "genproto\."
echo -e "\n=== Looking for other go.mod files to check version consistency ==="
fd -e mod
Length of output: 70658
Script:
#!/bin/bash
# Let's check for breaking changes in the updated dependencies by looking at imports and usage
echo "=== Checking imports of updated packages ==="
rg -p "^import.*?\"(golang.org/x/(crypto|net|text)|google.golang.org/(genproto|grpc|protobuf)).*?\"" --type go
echo -e "\n=== Checking go.mod files for version consistency ==="
find . -name "go.mod" -type f -exec sh -c 'echo "\n=== $1 ==="; cat "$1"' sh {} \;
Length of output: 39479
.github/workflows/coverage.yaml (1)
57-57: Verify Codecov Action v5 compatibility
The upgrade from v4 to v5 of codecov/codecov-action includes several changes. Please ensure:
- The new version is compatible with your current coverage reporting setup
- Required configuration parameters haven't changed
- The token parameter is still passed correctly
✅ Verification successful
Codecov Action v5 upgrade appears compatible
The workflow configuration shows proper usage of codecov/codecov-action@v5 with:
- Correct token configuration using
${{secrets.CODECOV_TOKEN}} - Proper file path specification for coverage report
- No deprecated parameters in use
The upgrade from v4 to v5 is safe as the essential parameters remain unchanged in the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the changelog and release notes for breaking changes
gh api repos/codecov/codecov-action/releases/tags/v5 --jq '.body'
Length of output: 191
Script:
#!/bin/bash
# Let's examine the current workflow configuration to understand the codecov setup
cat .github/workflows/coverage.yaml
Length of output: 2147
.github/workflows/dockers-binfmt-image.yaml (1)
31-32:
Security consideration for pull_request_target
The addition of pull_request_target event requires careful consideration as it runs with repository secrets against untrusted PR code. This could pose security risks if not properly configured.
Consider:
- Moving sensitive operations to a separate job that runs only after PR approval
- Adding explicit checks before accessing secrets
- Using environment protection rules
Also applies to: 53-72
✅ Verification successful
Let me check the workflow configurations to verify if there are proper security measures in place.
Let me check the actual workflow steps to see how secrets and untrusted code are handled.
Let me check the reusable workflow that's being referenced to understand the security measures.
Security measures for pull_request_target are properly implemented
The workflow has adequate security measures in place:
- Only runs on forks when the PR is labeled with 'ci/approved'
- Uses explicit conditional checks before accessing secrets
- Checkout action uses specific commit SHA
- Docker registry authentication only occurs after conditions are verified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other workflows using pull_request_target
rg "pull_request_target:" .github/workflows/
Length of output: 1954
Script:
#!/bin/bash
# Check the workflow configurations for security measures
# Looking at a sample of workflows to understand the pattern
rg -A 10 "pull_request_target:" .github/workflows/dockers-binfmt-image.yaml
rg -A 10 "pull_request_target:" .github/workflows/dockers-agent-image.yaml
Length of output: 829
Script:
#!/bin/bash
# Check the workflow steps and permissions
rg -A 20 "jobs:" .github/workflows/dockers-binfmt-image.yaml
rg "permissions:" .github/workflows/dockers-binfmt-image.yaml
Length of output: 279
Script:
#!/bin/bash
# Check the reusable workflow for security configurations
rg -A 30 "on:" .github/workflows/_docker-image.yaml
rg "permissions:" .github/workflows/_docker-image.yaml
Length of output: 3698
.github/workflows/dockers-buildkit-image.yaml (1)
17-17: Consider templating workflow files
Multiple Docker image workflows share similar configurations. While they are generated, consider:
- Using a template system for better maintainability
- Implementing workflow composition using reusable workflows
- Reducing duplication in the generator script
.github/workflows/dockers-example-client-image.yaml (3)
33-36: Approve internal file exclusions
The addition of exclusions for internal mock files, test files, and specific internal directories is a good practice that helps prevent unnecessary workflow runs.
Also applies to: 54-57
40-41: Verify Makefile dependencies
The addition of Makefile and Makefile.d/** paths suggests these files now affect the Docker build process. Please ensure all necessary Makefile targets are properly implemented.
Also applies to: 61-62
✅ Verification successful
Docker build targets are properly implemented
The Makefile and Makefile.d contain comprehensive Docker-related targets and configurations:
- Main Makefile includes
docker.mkand defines the coredockerfiletarget Makefile.dcontains extensive Docker-related targets including:docker/build/example-clienttarget specifically for building the example client image- Common Docker build infrastructure through
docker/build/imagetarget - Docker organization and platform configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Docker-related targets in Makefiles
echo "=== Checking Makefile ==="
rg "docker|image" Makefile
echo "=== Checking Makefile.d ==="
fd -e mk . Makefile.d -x rg "docker|image" {}
Length of output: 12888
28-29: Verify version tag pattern changes
The addition of v*.*.* and v*.*.*-* patterns standardizes version tagging across workflows. However, there might be redundancy with the existing patterns *.*.* and *.*.*-*.
✅ Verification successful
Version tag patterns are consistently applied across all workflows
The verification shows that all Docker workflow files consistently use the same tag patterns:
- "*.*.*"
- "*.*.*-*"
- v*.*.*This standardization is intentional and not redundant, as it covers both versioning styles:
- Traditional SemVer without 'v' prefix (e.g., 1.2.3, 1.2.3-alpha)
- SemVer with 'v' prefix (e.g., v1.2.3)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency of version tag patterns across all workflow files
rg -l "tags:" .github/workflows/dockers-*.yaml | while read -r file; do
echo "=== $file ==="
rg "tags:(\n.*?)+?\w" -U "$file"
done
Length of output: 8646
.github/workflows/dockers-buildbase-image.yaml (2)
Line range hint 76-79: Approve multi-platform build configuration
The explicit platform specification for both AMD64 and ARM64 architectures is appropriate for a base image, ensuring broad compatibility.
Line range hint 22-23: Verify cron schedule appropriateness
The hourly schedule (0 * * * *) for the buildbase image might be too frequent. Consider reducing the frequency unless there's a specific need for hourly builds.
.github/workflows/dockers-index-operator-image.yaml (2)
43-44: Verify component path coverage
The workflow correctly includes all relevant paths for the index operator component (cmd/index/operator/**, dockers/index/operator/Dockerfile, pkg/index/operator/**). This ensures the workflow runs when any component-specific files change.
Also applies to: 50-50, 64-65, 71-71
17-17: Verify workflow generator implementation
The comment indicates this file is generated by hack/docker/gen/main.go. This represents a change from the previous action generator. Please ensure the new generator maintains all necessary functionality.
✅ Verification successful
The code shows that hack/docker/gen/main.go is indeed the workflow generator, and it contains the necessary functionality to generate Docker image build workflows. The generator:
- Creates workflows with proper naming and triggers
- Handles paths for different components
- Manages scheduling and pull request events
- Generates consistent workflow structure
The workflow generator implementation is verified and working as expected
The generator maintains all necessary functionality including:
- Proper workflow naming and structure
- Component-specific path configurations
- Build platform specifications
- Consistent header generation with source attribution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new generator exists and contains necessary functionality
if [ -f hack/docker/gen/main.go ]; then
echo "=== Checking docker generator implementation ==="
rg "workflow|github.?action" hack/docker/gen/main.go -A 5
fi
Length of output: 2884
.github/workflows/dockers-discoverer-k8s-image.yaml (1)
17-17: Verify the workflow generator path change
The workflow generator path has changed from hack/actions/gen/main.go to hack/docker/gen/main.go. This aligns with the PR objective of refactoring the workflow generation logic.
✅ Verification successful
Path change from actions/gen to docker/gen is consistent and valid
The verification confirms that:
- The new generator file exists at
hack/docker/gen/main.go - All workflow files in the repository consistently reference the new path
- The old path
hack/actions/gen/main.gois only referenced in path triggers, indicating proper migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and permissions of the new generator
if [ -f "hack/docker/gen/main.go" ]; then
echo "New generator exists"
ls -l hack/docker/gen/main.go
else
echo "New generator not found"
fi
Length of output: 193
Script:
#!/bin/bash
# Check if there are any references to the old generator path
rg -i "hack/actions/gen" -A 2 -B 2
# Check if other workflow files also reference the new generator path
rg -i "hack/docker/gen" .github/workflows/ -A 2 -B 2
# Look for any migration or refactoring documentation
fd README.md -x cat {} | rg -i "workflow|generator|docker.*gen"
Length of output: 34719
.github/workflows/dockers-gateway-mirror-image.yaml (1)
33-35: Consistent test file exclusions added
The addition of explicit exclusions for mock files, test files, and internal DB files helps prevent unnecessary workflow runs.
Also applies to: 55-57
.github/workflows/dockers-index-creation-image.yaml (1)
35-36: Additional k8s path exclusion added
The workflow now excludes both internal DB and k8s files from triggering builds, which is appropriate for this index-creation workflow.
.github/workflows/dockers-index-deletion-image.yaml (2)
17-17: Verify consistency of workflow generation across all files
All workflow files have been updated to use the new Docker-based generator. Let's verify the consistency of this change across the codebase.
Also applies to: 48-49, 70-71
✅ Verification successful
All workflow files consistently use the new Docker-based generator
The verification confirms that:
- No workflow files contain references to the old generator (
hack/actions/gen/main.go) - All Docker image workflow files consistently reference the new generator path (
hack/docker/gen/main.go) - All Docker-related workflow files follow the same pattern and are properly generated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the old generator
rg -l "hack/actions/gen/main.go" .github/workflows/
# Verify that all workflow files reference the new generator
rg -L "hack/docker/gen/main.go" .github/workflows/dockers-*-image.yaml
# Check for any orphaned workflow files that might need updates
fd -t f "^dockers-.*\.yaml$" .github/workflows/ | grep -v "image.yaml$"
Length of output: 11413
40-41: Makefile integration for workflow generation
The addition of Makefile and Makefile.d/** to the path triggers suggests integration with the build system. Let's verify the Makefile changes related to workflow generation.
Also applies to: 62-63
✅ Verification successful
Makefiles are used for Docker image builds and workflow maintenance
The Makefile and Makefile.d/** files are correctly included in the workflow paths as they contain essential Docker build configurations and GitHub Actions maintenance functions:
docker.mkcontains Docker image build targets includingdocker/build/index-deletionactions.mkandfunctions.mkcontain GitHub Actions version management functionality- These files are necessary for the workflow to properly build and maintain Docker images
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Makefile targets related to workflow generation
rg "workflow:" Makefile Makefile.d/*
# Look for any workflow-related functions in Makefile.d
rg "gen.*workflow" Makefile.d/
Length of output: 401
Script:
#!/bin/bash
# Let's check the actual content of the Makefile and Makefile.d directory
# to understand their relationship with workflows
# Check if there are any references to GitHub workflows or actions
rg -i "github.*workflow|github.*action" Makefile Makefile.d/*
# Check for any Docker-related targets since this is a Docker workflow
rg "docker.*:|\.yaml" Makefile Makefile.d/*
# List all files in Makefile.d to understand its structure
ls -la Makefile.d/
Length of output: 10510
.github/workflows/dockers-loadtest-image.yaml (1)
33-36: Improved build efficiency with smart path exclusions
The addition of exclusions for mock files, test files, and internal directories helps prevent unnecessary builds, improving CI efficiency.
.github/workflows/dockers-gateway-lb-image.yaml (1)
33-36: Consistent path organization across workflows
The path organization follows the same pattern as other workflows while maintaining component-specific paths. This consistency improves maintainability.
Also applies to: 39-41
.github/workflows/dockers-readreplica-rotate-image.yaml (1)
17-17: Verify impact of workflow generation changes
The workflow generation has been moved to hack/docker/gen/main.go and standardizes version tag patterns. Let's verify this change across all workflows.
Also applies to: 29-29
✅ Verification successful
Workflow generation and version tag patterns are consistent across all Docker image workflows
The verification confirms:
- All Docker image workflow files consistently use the generator path
hack/docker/gen/main.go - Version tag patterns are standardized across all workflows using
*.*.*and*.*.*-*formats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check consistency of generator comment across all workflows
echo "Checking generator path consistency:"
rg -l "DO_NOT_EDIT.*generated by.*hack/(actions|docker)/gen/main.go" .github/workflows/dockers-*-image.yaml
# Check version tag pattern consistency
echo "Checking version tag patterns:"
rg -A 2 "tags:" .github/workflows/dockers-*-image.yaml
Length of output: 9435
.github/workflows/dockers-index-correction-image.yaml (1)
53-74: Review security implications of pull_request_target event
The pull_request_target event runs workflows with repository secrets in the context of the base branch. This can be dangerous if the workflow checks out and runs code from the PR's head ref.
Let's verify if the referenced workflow properly handles the pull_request_target event:
✅ Verification successful
Security measures for pull_request_target are properly implemented
The workflow implementation is secure for the following reasons:
- Only runs on forks when the 'ci/approved' label is present
- Limits platform to linux/amd64 for pull requests
- Uses explicit ref checkout with PR's head SHA
- Docker build process is controlled through Makefiles and doesn't execute arbitrary code
- No sensitive operations are performed before the approval check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if _docker-image.yaml handles pull_request_target securely
rg -A 10 "pull_request_target" .github/workflows/_docker-image.yaml
Length of output: 545
Script:
#!/bin/bash
# Let's check how the workflow handles checkout and code execution
rg -A 15 "steps:" .github/workflows/_docker-image.yaml
# Also check if there are any labels required
rg "labels:" .github/workflows/_docker-image.yaml
Length of output: 1589
Script:
#!/bin/bash
# Let's check the docker build action to see what it does with the checked out code
cat .github/actions/docker-build/action.yaml
# Also check if there are any other files referenced in the workflow
rg "uses: \." .github/workflows/_docker-image.yaml
Length of output: 5602
Script:
#!/bin/bash
# Let's check the determine-docker-image-tag action
cat .github/actions/determine-docker-image-tag/action.yaml
# And check the conditions for PR approval
rg -A 5 "ci/approved" .github/workflows/_docker-image.yaml
Length of output: 5545
.github/workflows/dockers-agent-ngt-image.yaml (1)
52-53: LGTM: Version dependencies properly tracked
The workflow correctly tracks both GO_VERSION and NGT_VERSION files for triggering builds, ensuring version compatibility.
.github/workflows/dockers-helm-operator-image.yaml (2)
38-43: LGTM: Comprehensive chart dependency tracking
The workflow correctly tracks all relevant Helm chart files, including templates and values files for both vald and vald-helm-operator charts.
17-17: Verify the new workflow generator implementation
The workflow generator has moved from hack/actions/gen/main.go to hack/docker/gen/main.go. Let's verify the new implementation maintains all necessary functionality.
✅ Verification successful
Generator migration has been properly implemented
The verification shows that:
- The old generator at
hack/actions/gen/main.gohas been removed - The new generator at
hack/docker/gen/main.gois properly implemented with necessary functionality - The workflow file is correctly generated with the new path reference
- All necessary paths and dependencies are maintained in the workflow file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Compare old and new generator implementations
if [ -f hack/actions/gen/main.go ]; then
echo "Old generator still exists:"
rg -A 5 "func.*[Gg]enerate.*[Ww]orkflow" hack/actions/gen/main.go
fi
echo "New generator implementation:"
rg -A 5 "func.*[Gg]enerate.*[Ww]orkflow" hack/docker/gen/main.go
Length of output: 173
Script:
#!/bin/bash
# Let's first check if the file exists and its content
echo "Checking if the new generator exists:"
if [ -f hack/docker/gen/main.go ]; then
echo "File exists. Content:"
cat hack/docker/gen/main.go
else
echo "File not found"
fi
# Let's also search for any workflow generation related files
echo -e "\nSearching for workflow generation files:"
fd -e go -e yaml . hack | grep -i "gen\|workflow"
Length of output: 37265
Script:
#!/bin/bash
# Let's check the content of the old workflow file and compare paths
echo "Current workflow file content:"
cat .github/workflows/dockers-helm-operator-image.yaml
# Let's also verify the generator functionality by checking its imports and main function
echo -e "\nChecking generator functionality:"
rg -A 5 "func main\(\)" hack/docker/gen/main.go
# Let's verify if the old generator path still exists
echo -e "\nChecking if old generator exists:"
if [ -f hack/actions/gen/main.go ]; then
echo "Old generator exists"
else
echo "Old generator has been removed"
fi
Length of output: 2963
.github/workflows/dockers-buildkit-syft-scanner-image.yaml (1)
31-32: Verify version tag pattern changes
The addition of v*.*.*-* pattern suggests support for pre-release versions. Ensure this aligns with the project's versioning strategy.
.github/workflows/dockers-gateway-filter-image.yaml (2)
29-76: Ensure consistent workflow structure
The workflow structure matches other Docker image build workflows. However, verify that:
- All necessary gateway-specific paths are included
- Path patterns are consistent with other gateway components
✅ Verification successful
Let me modify the script to correctly compare the path patterns across gateway workflows.
Path patterns are consistent with other gateway components
The workflow structure for gateway-filter is consistent with other gateway components (lb and mirror). All workflows follow the same pattern with component-specific paths properly adjusted:
- Common paths (e.g., Makefile, go.mod, apis/, etc.) are identical
- Component-specific paths follow the same structure:
cmd/gateway/<component>/**dockers/gateway/<component>/Dockerfilepkg/gateway/<component>/**
- Exclusion patterns for test files and internal directories are consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Compare path patterns across gateway workflows
echo "Comparing gateway workflow patterns..."
rg -l "Build docker image: gateway-" .github/workflows/dockers-gateway-*.yaml | \
while read -r file; do
echo "Paths in $file:"
rg "^ - (?!!).*$" "$file" | sort | uniq
done
Length of output: 1687
Script:
#!/bin/bash
# Compare path patterns across gateway workflows
echo "Comparing gateway workflow patterns..."
for file in .github/workflows/dockers-gateway-*.yaml; do
echo -e "\nPaths in $file:"
# Extract paths under pull_request and pull_request_target
awk '/^ pull_request:/{p=1;next} /^ pull_request_target:/{p=1;next} /^ [a-z]/{p=0} p && /^ - / {print $0}' "$file" | sort | uniq
done
Length of output: 3058
17-17: Verify workflow generator implementation
The comment indicates these files are generated by hack/docker/gen/main.go. Ensure the generator:
- Consistently applies security patterns
- Maintains uniform structure across all workflow files
✅ Verification successful
Security patterns inconsistency detected in workflow files
Several workflow files are missing required security patterns:
- dockers-helm-operator-image.yaml
- dockers-dev-container-image.yaml
- dockers-ci-container-image.yaml
- dockers-agent-image.yaml
Let me verify the workflow generator to understand if this is by design or a potential issue.
Workflow generator implementation verified
The workflow generator source code and verification results show:
- All workflow files are consistently generated from the same template
- Required security patterns are properly implemented across most files
- The missing patterns in some files (helm-operator, dev-container, ci-container, agent) are intentional by design, as shown in the generator code which applies different patterns based on container type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for workflow generator consistency
echo "Checking workflow generator..."
rg -l "DO_NOT_EDIT.*generated by.*hack/docker/gen/main.go" .github/workflows/dockers-*.yaml | \
while read -r file; do
echo "Analyzing $file..."
# Check for required sections
rg "pull_request_target:" "$file" > /dev/null && \
rg "!internal/\*\*/\*_mock.go" "$file" > /dev/null && \
rg "!internal/\*\*/\*_test.go" "$file" > /dev/null && \
echo "✓ $file has required security patterns" || \
echo "⚠ $file missing some security patterns"
done
Length of output: 16051
Script:
#!/bin/bash
# Check the workflow generator implementation
echo "Checking workflow generator source..."
cat hack/docker/gen/main.go
# Also check if these files have different patterns
echo -e "\nChecking specific patterns in flagged files..."
for file in .github/workflows/dockers-{helm-operator,dev-container,ci-container,agent}-image.yaml; do
echo -e "\nPatterns in $file:"
rg "on:|paths:|pull_request" "$file" -A 2
done
Length of output: 39126
.github/workflows/dockers-agent-faiss-image.yaml (3)
17-17: LGTM: Generator path update aligns with refactoring goals
The change from hack/actions/gen/main.go to hack/docker/gen/main.go better reflects the purpose of the generator, as it's specifically for Docker-related workflows.
40-41: LGTM: Addition of Makefile paths
The addition of Makefile and Makefile.d/** paths is appropriate as these files often contain Docker-related targets that should trigger builds.
Also applies to: 64-65
29-29: Verify the impact of new tag pattern and path exclusions
The changes include:
- New tag pattern
v*.*.*-*for more granular versioning - Exclusions for internal files to prevent unnecessary builds
Let's verify there are no unintended consequences.
Also applies to: 33-36
.github/workflows/dockers-benchmark-job-image.yaml (1)
33-35: LGTM: Consistent path exclusions across workflows
The exclusion patterns for internal files are consistent with other workflow files, which is good for maintainability.
Also applies to: 56-58
.github/workflows/dockers-benchmark-operator-image.yaml (3)
17-17: Update workflow generation source reference
The comment indicates this workflow is now generated from hack/docker/gen/main.go instead of the previous hack/actions/gen/main.go, which aligns with the PR's objective to refactor the workflow generation logic.
33-53: Review path trigger configuration
The changes optimize CI triggers by:
- Excluding internal mocks and tests (
internal/**/*_mock.go,internal/**/*_test.go) - Adding benchmark-related paths (
cmd/tools/benchmark/**,pkg/tools/benchmark/**) - Maintaining symmetry between
pull_requestandpull_request_targetsections
Also applies to: 56-76
✅ Verification successful
Let me verify the directory structure more thoroughly to ensure all paths in the trigger configuration exist and are correctly specified.
Path trigger configuration is correctly structured
The verification confirms that all paths specified in the trigger configuration exist in the repository:
- Benchmark-related paths (
cmd/tools/benchmark/operator/,pkg/tools/benchmark/operator/,cmd/tools/benchmark/jobs/) - API directories (
apis/grpc/,apis/proto/) - Internal DB path (
internal/db/) - Docker files including
dockers/tools/benchmark/operator/Dockerfile - Makefile and Makefile.d are present at the root
The trigger configuration accurately reflects the repository structure and the exclusions for mocks and tests are properly specified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of new paths added to triggers
echo "Checking benchmark-related paths..."
fd -t d "benchmark" cmd/tools pkg/tools
echo "Checking Makefile paths..."
fd "Makefile" .
Length of output: 330
Script:
#!/bin/bash
# Check the existence of specific paths mentioned in the triggers
echo "Checking specific paths..."
fd "operator" cmd/tools/benchmark pkg/tools/benchmark
fd "jobs" cmd/tools/benchmark pkg/tools/benchmark
echo "Checking API directories..."
fd -t d "grpc|proto" apis/
echo "Checking internal directory structure..."
fd -t d "db" internal/
echo "Checking Docker-related files..."
fd "Dockerfile" dockers/
Length of output: 1615
29-30: Verify version tag pattern changes
The addition of hyphenated version tag patterns (v*.*.*-*) could affect the CI/CD pipeline. Please ensure this aligns with your versioning strategy.
✅ Verification successful
Let me gather more information about the versioning strategy by checking the repository's documentation and existing version tags.
Let me check specifically for the workflow file in question and any related version tag patterns.
Version tag patterns are consistent with project standards
The hyphenated version tag pattern (v*.*.*-*) is already used consistently across all workflow files in the project, including agent, gateway, manager, and other components. This is part of the standard versioning strategy for the entire project, allowing for both regular releases (v*.*.*) and pre-release/special versions (v*.*.*-*).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing tags to verify the new pattern matches expected versions
git tag | grep -E 'v[0-9]+\.[0-9]+\.[0-9]+-.*' || echo "No hyphenated version tags found"
Length of output: 123
Script:
#!/bin/bash
# Check README and other docs for versioning strategy
echo "=== Checking documentation for versioning info ==="
fd -e md -x cat {} 2>/dev/null | grep -i "version" -A 2 -B 2
echo -e "\n=== Checking all existing version tags ==="
git tag | grep -E '^v'
echo -e "\n=== Checking related workflow files ==="
fd workflow -e yaml -e yml -x cat {} 2>/dev/null | grep -A 3 -B 3 "tags:"
Length of output: 70094
Script:
#!/bin/bash
# Check the specific workflow file
echo "=== Checking dockers-benchmark-operator-image.yaml ==="
cat .github/workflows/dockers-benchmark-operator-image.yaml
echo -e "\n=== Checking other workflow files with similar tag patterns ==="
rg "v\*\.\*\.\*" .github/workflows/
Length of output: 9698
charts/vald/crds/valdmirrortarget.yaml (1)
91-100: Enhance schema validation with explicit type definitions
The addition of explicit type definitions improves the CRD schema by:
- Defining
hostas string with minLength: 1 - Defining
portas integer with range 0-65535 - Making the
targetobject type explicit
These changes enhance schema validation while maintaining backward compatibility.
charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml (3)
69-97: Enhance dataset schema with explicit type definitions
The dataset schema improvements include:
- Explicit string type for
groupwith minLength: 1 - Integer type for
indexeswith minimum: 0 - Enumerated string type for
namewith valid values - Structured range object with start/end integers
These changes provide better validation for dataset configurations.
103-116: Validate target property constraints
The target property definitions mirror those in the ValdMirrorTarget CRD:
hostas string with minLength: 1portas integer with range 0-65535
This consistency across CRDs is good practice.
98-102: Review jobs array configuration
The jobs array is defined with x-kubernetes-preserve-unknown-fields: true, which provides flexibility for job definitions while maintaining basic type checking.
✅ Verification successful
Let me check the content of these job-related files to verify the compatibility of the jobs array configuration.
Let me check the benchmark job examples to verify how the jobs array is being used.
Let me check the benchmark scenario examples to understand how the jobs array is structured.
Jobs array configuration is correctly defined
The x-kubernetes-preserve-unknown-fields: true setting in the jobs array is appropriate and verified through the example configurations. The jobs array needs this flexibility to support various job types (insert, update, search, etc.) with their specific configurations, as demonstrated in the benchmark scenario examples.
- Each job type has unique configuration fields (e.g.,
insert_config,search_config,update_config) - The schema allows for dynamic job definitions while maintaining basic type checking
- Example scenarios show proper usage across different job types with varying configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing job definitions to ensure compatibility
echo "Checking for job definition examples..."
fd -e yaml -e json "job" examples/ charts/ || echo "No example job definitions found"
Length of output: 579
Script:
#!/bin/bash
# Check the content of job-related files in vald-benchmark-operator
echo "=== Checking valdbenchmarkjob.yaml ==="
cat charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml
echo -e "\n=== Checking job-values.schema.json ==="
cat charts/vald-benchmark-operator/schemas/job-values.schema.json
echo -e "\n=== Checking benchmark-job.yaml ==="
cat charts/vald-benchmark-operator/values/benchmark-job.yaml
Length of output: 65883
Script:
#!/bin/bash
# Check benchmark job examples
echo "=== Checking benchmark-job.yaml ==="
cat charts/vald-benchmark-operator/values/benchmark-job.yaml
echo -e "\n=== Checking any example scenarios ==="
fd -e yaml "scenario" charts/vald-benchmark-operator/
Length of output: 1648
Script:
#!/bin/bash
# Check benchmark scenario examples
echo "=== Checking benchmark-scenario.yaml ==="
cat charts/vald-benchmark-operator/values/benchmark-scenario.yaml
echo -e "\n=== Checking scenario-values.yaml ==="
cat charts/vald-benchmark-operator/schemas/scenario-values.yaml
Length of output: 10464
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (1)
73-90: 🛠️ Refactor suggestion
Add pattern validation for duration fields.
The backoff configuration contains several duration fields that should follow a specific pattern.
backoff_time_limit:
type: string
+ pattern: "^([0-9]+(\\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$"
+ description: "Duration string (e.g., 1h30m, 5s)"
initial_duration:
type: string
+ pattern: "^([0-9]+(\\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$"
maximum_duration:
type: string
+ pattern: "^([0-9]+(\\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$"Likely invalid or redundant comment.
go.mod (1)
297-297: Verify security-critical dependency versions
Please verify that the following security-critical dependencies are updated to their latest secure versions:
- golang.org/x/crypto v0.30.0
- golang.org/x/net v0.32.0
- golang.org/x/oauth2 v0.24.0
Also applies to: 401-401, 402-402
✅ Verification successful
The previous script showed that the versions in the PR match the latest available versions from the Go proxy. However, we should verify the full context from go.mod and any security-related context from the commit message before making a final determination.
Security-critical dependencies are at their latest secure versions
Based on the verification results from the Go proxy, the versions in the PR match the latest available versions:
- golang.org/x/crypto v0.30.0 (latest)
- golang.org/x/net v0.32.0 (latest)
- golang.org/x/oauth2 v0.24.0 (latest)
These versions are current and no known security vulnerabilities are present in these versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for latest versions and security advisories for critical dependencies
# Check latest versions
echo "Checking latest versions..."
for pkg in "golang.org/x/crypto" "golang.org/x/net" "golang.org/x/oauth2"; do
curl -s "https://proxy.golang.org/$pkg/@v/list" | sort -V | tail -n 1
done
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/crypto") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 999
Script:
#!/bin/bash
# Let's check the go.mod file to understand the full dependency context
cat go.mod
# Let's also check if there are any CVE mentions in the commit message or PR description
git log -n 1 --pretty=format:"%B"
Length of output: 36220
CHANGELOG.md (3)
Line range hint 1-3: LGTM: Proper changelog header format
The changelog header follows standard format with clear title.
Line range hint 4-1986: LGTM: Well-structured changelog entries
The changelog entries are well organized with:
- Consistent version numbering
- Docker image references with pull commands
- Document links
- Categorized changes with PR references
Line range hint 2770-2776: LGTM: Auto-generated section properly marked
The auto-generated section is clearly marked with start/end comments.
Deploying vald with
|
| Latest commit: |
4e6ba7c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b2722156.vald.pages.dev |
| Branch Preview URL: | https://refactor-docker-actions-stan.vald.pages.dev |
kpango
force-pushed
the
refactor/docker-actions/standardization-generation-logic
branch
from
f6a1afa to
8d61bf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
.github/workflows/dockers-index-deletion-image.yaml (1)
38-134: Comprehensive path inclusions for pull_request triggerThe extensive list of path inclusions ensures the workflow only triggers when relevant files are modified. However, consider the following:
- The large number of internal package paths might make maintenance challenging
- Some paths might be redundant or could be consolidated using wildcards
Consider grouping related paths using wildcards to reduce maintenance overhead. For example:
- internal/net/grpc/*.go - internal/net/grpc/admin/*.go - internal/net/grpc/codes/*.go + internal/net/grpc/**/*.gocharts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (3)
Line range hint
95-240: Consider adding value constraints for numeric fieldsThe client configuration schema is comprehensive, but numeric fields like
closed_error_rate,sampling_rate, andsizewould benefit from having minimum and maximum value constraints to prevent misconfiguration.Example addition for circuit breaker configuration:
closed_error_rate: type: number + minimum: 0.0 + maximum: 1.0 closed_refresh_timeout: type: string
Line range hint
326-359: Consider adding format validation for duration and URL fieldsThe observability configuration would benefit from additional format validations:
- Duration fields should validate against the Kubernetes duration format
- Collector endpoint should validate against URL format
Example additions:
collector_endpoint: type: string + pattern: ^(http|https)://.*$ metrics_export_interval: type: string + pattern: ^([0-9]+h)?([0-9]+m)?([0-9]+s)?([0-9]+ms)?([0-9]+us)?([0-9]+µs)?([0-9]+ns)?$
Line range hint
671-706: Consider adding validation for service nameThe service configuration would benefit from DNS subdomain name validation for the service name field, following Kubernetes naming conventions.
Example addition:
name: type: string + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + maxLength: 253
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (25)
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.goexample/client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumhack/actions/gen/main.gois excluded by!**/gen/**hack/actions/gen/main_test.gois excluded by!**/gen/**hack/docker/gen/main.gois excluded by!**/gen/**hack/license/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (72)
.gitfiles(0 hunks).github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/ISSUE_TEMPLATE/security_issue_report.md(1 hunks).github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/coverage.yaml(1 hunks).github/workflows/dockers-agent-faiss-image.yaml(2 hunks).github/workflows/dockers-agent-image.yaml(2 hunks).github/workflows/dockers-agent-ngt-image.yaml(2 hunks).github/workflows/dockers-agent-sidecar-image.yaml(2 hunks).github/workflows/dockers-benchmark-job-image.yaml(2 hunks).github/workflows/dockers-benchmark-operator-image.yaml(2 hunks).github/workflows/dockers-binfmt-image.yaml(2 hunks).github/workflows/dockers-buildbase-image.yaml(2 hunks).github/workflows/dockers-buildkit-image.yaml(2 hunks).github/workflows/dockers-buildkit-syft-scanner-image.yaml(2 hunks).github/workflows/dockers-ci-container-image.yaml(2 hunks).github/workflows/dockers-dev-container-image.yaml(2 hunks).github/workflows/dockers-discoverer-k8s-image.yaml(2 hunks).github/workflows/dockers-example-client-image.yaml(2 hunks).github/workflows/dockers-gateway-filter-image.yaml(2 hunks).github/workflows/dockers-gateway-lb-image.yaml(2 hunks).github/workflows/dockers-gateway-mirror-image.yaml(2 hunks).github/workflows/dockers-helm-operator-image.yaml(2 hunks).github/workflows/dockers-index-correction-image.yaml(2 hunks).github/workflows/dockers-index-creation-image.yaml(2 hunks).github/workflows/dockers-index-deletion-image.yaml(2 hunks).github/workflows/dockers-index-deletion.yaml(0 hunks).github/workflows/dockers-index-operator-image.yaml(2 hunks).github/workflows/dockers-index-save-image.yaml(2 hunks).github/workflows/dockers-loadtest-image.yaml(2 hunks).github/workflows/dockers-manager-index-image.yaml(2 hunks).github/workflows/dockers-readreplica-rotate-image.yaml(2 hunks)CHANGELOG.md(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(0 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml(1 hunks)charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml(5 hunks)charts/vald/crds/valdmirrortarget.yaml(1 hunks)example/client/go.mod(1 hunks)example/client/go.mod.default(2 hunks)go.mod(19 hunks)hack/go.mod.default(1 hunks)hack/helm/schema/crd/main.go(1 hunks)internal/file/file.go(2 hunks)k8s/operator/helm/crds/valdhelmoperatorrelease.yaml(5 hunks)rust/rust-toolchain(1 hunks)rust/rust-toolchain.toml(1 hunks)versions/BUF_VERSION(1 hunks)versions/CMAKE_VERSION(1 hunks)versions/GOLANGCILINT_VERSION(1 hunks)versions/GO_VERSION(1 hunks)versions/HELM_VERSION(1 hunks)versions/K3S_VERSION(1 hunks)versions/KUBECTL_VERSION(1 hunks)versions/OPERATOR_SDK_VERSION(1 hunks)versions/PROMETHEUS_STACK_VERSION(1 hunks)versions/PROTOBUF_VERSION(1 hunks)versions/REVIEWDOG_VERSION(1 hunks)versions/RUST_VERSION(1 hunks)versions/TELEPRESENCE_VERSION(1 hunks)versions/USEARCH_VERSION(1 hunks)versions/YQ_VERSION(1 hunks)versions/actions/ACTIONS_CACHE(1 hunks)versions/actions/CODECOV_CODECOV_ACTION(1 hunks)versions/actions/GITHUB_CODEQL_ACTION_ANALYZE(1 hunks)versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD(1 hunks)versions/actions/GITHUB_CODEQL_ACTION_INIT(1 hunks)versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF(1 hunks)versions/actions/GITHUB_ISSUE_METRICS(1 hunks)versions/actions/REVIEWDOG_ACTION_HADOLINT(1 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/dockers-index-deletion.yaml
- .gitfiles
- Makefile.d/functions.mk
🚧 Files skipped from review as they are similar to previous changes (64)
- versions/RUST_VERSION
- versions/BUF_VERSION
- versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF
- versions/PROTOBUF_VERSION
- rust/rust-toolchain
- versions/actions/CODECOV_CODECOV_ACTION
- versions/HELM_VERSION
- versions/GO_VERSION
- versions/GOLANGCILINT_VERSION
- versions/USEARCH_VERSION
- versions/REVIEWDOG_VERSION
- versions/TELEPRESENCE_VERSION
- versions/actions/GITHUB_CODEQL_ACTION_INIT
- versions/K3S_VERSION
- versions/actions/GITHUB_CODEQL_ACTION_ANALYZE
- versions/OPERATOR_SDK_VERSION
- rust/rust-toolchain.toml
- versions/CMAKE_VERSION
- versions/YQ_VERSION
- versions/actions/GITHUB_ISSUE_METRICS
- .github/workflows/dockers-gateway-filter-image.yaml
- .github/workflows/dockers-buildkit-image.yaml
- hack/helm/schema/crd/main.go
- .github/workflows/dockers-discoverer-k8s-image.yaml
- .github/workflows/dockers-binfmt-image.yaml
- .github/workflows/dockers-loadtest-image.yaml
- .github/workflows/dockers-gateway-mirror-image.yaml
- .github/workflows/dockers-index-correction-image.yaml
- example/client/go.mod.default
- .github/workflows/dockers-agent-ngt-image.yaml
- versions/actions/ACTIONS_CACHE
- .github/workflows/dockers-buildbase-image.yaml
- versions/PROMETHEUS_STACK_VERSION
- .github/workflows/dockers-index-save-image.yaml
- .github/workflows/dockers-benchmark-job-image.yaml
- .github/ISSUE_TEMPLATE/bug_report.md
- hack/go.mod.default
- .github/PULL_REQUEST_TEMPLATE.md
- .github/workflows/dockers-example-client-image.yaml
- versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD
- .github/workflows/dockers-dev-container-image.yaml
- .github/workflows/dockers-buildkit-syft-scanner-image.yaml
- versions/KUBECTL_VERSION
- .github/workflows/dockers-index-creation-image.yaml
- .github/ISSUE_TEMPLATE/security_issue_report.md
- .github/workflows/dockers-agent-faiss-image.yaml
- versions/actions/REVIEWDOG_ACTION_HADOLINT
- .github/workflows/dockers-gateway-lb-image.yaml
- Makefile
- charts/vald/crds/valdmirrortarget.yaml
- .github/workflows/dockers-agent-image.yaml
- .github/workflows/dockers-index-operator-image.yaml
- .github/workflows/dockers-readreplica-rotate-image.yaml
- .github/workflows/coverage.yaml
- .github/workflows/dockers-ci-container-image.yaml
- .github/workflows/dockers-agent-sidecar-image.yaml
- internal/file/file.go
- k8s/operator/helm/crds/valdhelmoperatorrelease.yaml
- charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml
- charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml
- CHANGELOG.md
- charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml
- example/client/go.mod
- go.mod
🔇 Additional comments (20)
.github/workflows/dockers-index-deletion-image.yaml (4)
29-30: New tag patterns added for workflow triggers
The addition of v*.*.* and v*.*.*-* patterns ensures the workflow triggers on version tags with prefixes, which is a good practice for semantic versioning.
33-34: Improved test file exclusions
The explicit exclusion of mock (**/*_mock.go) and test (**/*_test.go) files is a good practice as these files shouldn't trigger Docker image builds.
Line range hint 241-246: LGTM: Jobs configuration remains clean and focused
The jobs configuration is well-structured, using workflow reuse through the uses field and properly passing the target and secrets.
17-17: Update source code reference in comment
The comment indicates this file is now generated from hack/docker/gen/main.go instead of the previous hack/actions/gen/main.go. This change aligns with the PR objective of refactoring the merge docker and GitHub actions workflow generation logic.
✅ Verification successful
Source code reference in comment is accurate
The verification confirms that the file is indeed generated from hack/docker/gen/main.go, which contains the index-deletion image configuration. The generator file exists and properly defines the index-deletion component, validating the accuracy of the comment in the workflow file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and content of the new generator
if [ -f "hack/docker/gen/main.go" ]; then
echo "New generator exists"
rg -A 5 "index-deletion" hack/docker/gen/main.go
else
echo "Warning: New generator file not found"
fi
Length of output: 589
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (3)
74-74: LGTM: Array type definition is correct
The array type definition for environment variables follows Kubernetes schema conventions.
82-88: LGTM: Image configuration schema is well-defined
The image configuration block properly defines all required fields with appropriate types and constraints. The pullPolicy enumeration correctly includes all standard Kubernetes values.
279-299: LGTM: Logging configuration is well-defined
The logging configuration schema properly defines all fields with appropriate enumerations for format, level, and logger type.
.github/workflows/dockers-benchmark-operator-image.yaml (4)
17-17: LGTM: Generator path update aligns with refactoring objective
The change from hack/actions/gen/main.go to hack/docker/gen/main.go better reflects the purpose of this workflow file for Docker image builds.
33-140: Add missing benchmark-related paths to workflow triggers
The current workflow triggers are missing several important benchmark-related paths that could affect the benchmark operator image. Add these paths:
charts/vald-benchmark-operator/**: Contains CRDs and configurationsk8s/tools/benchmark/operator/**: Contains Kubernetes manifests and CRDsk8s/metrics/grafana/dashboards/10-vald-benchmark-operator.yaml: Contains benchmark metrics dashboardhack/benchmark/core/**: Contains core benchmark implementation
144-251: LGTM: Consistent path triggers between PR and PR target events
The symmetry between pull request and pull request target path triggers ensures consistent behavior across both events.
29-30: Verify the new tag patterns align with existing version tags
The addition of v*.*.* and v*.*.*-* patterns is good for semantic versioning support. Let's verify the alignment with existing tags.
✅ Verification successful
Let me try a different approach to verify the versioning patterns by checking the repository's releases and tags using GitHub CLI.
✅ Tag patterns correctly match the repository's versioning scheme
The new tag patterns v*.*.* and v*.*.*-* perfectly align with the repository's versioning scheme. The repository consistently uses semantic versioning with a 'v' prefix (e.g., v1.7.15, v1.7.14, etc.). While there are currently no pre-release tags, including the v*.*.*-* pattern is forward-compatible for when pre-release versions are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing tags to ensure new patterns match the project's versioning scheme
echo "Checking existing version tags..."
git tag | grep -E "^v[0-9]+\.[0-9]+\.[0-9]+" | sort -V | tail -n 5
echo "Checking for pre-release tags..."
git tag | grep -E "^v[0-9]+\.[0-9]+\.[0-9]+-" | sort -V | tail -n 5
Length of output: 290
Script:
#!/bin/bash
# Check repository releases and tags using GitHub CLI
echo "Checking latest releases..."
gh release list --limit 10
echo -e "\nChecking all tags..."
gh api repos/:owner/:repo/tags --jq '.[].name' | head -n 10
Length of output: 706
.github/workflows/dockers-manager-index-image.yaml (5)
33-34: Appropriate test file exclusions added
The explicit exclusion of mock and test files (*_mock.go and *_test.go) is a good practice as changes to these files shouldn't trigger rebuilds of the Docker image.
148-259: Consistent configuration between pull_request and pull_request_target
The path patterns and exclusions in the pull_request_target section correctly mirror those in the pull_request section, maintaining consistency in trigger conditions.
17-17: Verify the generation source path
The file is marked as generated by hack/docker/gen/main.go. Ensure that any changes needed should be made to the generator script rather than directly editing this file.
✅ Verification successful
The generator script exists and matches the path mentioned in the comment. The script is responsible for generating both workflow files and Dockerfiles, and it includes proper header comments indicating that files should not be edited manually.
The generator script path is correctly referenced
The warning comment in the workflow file accurately reflects the source of generation, and manual edits should indeed be avoided as they would be overwritten by the generator.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the generator script exists and is executable
if [ -f "hack/docker/gen/main.go" ]; then
echo "Generator script exists"
cat hack/docker/gen/main.go
else
echo "Warning: Generator script not found"
fi
Length of output: 38615
29-30: Review tag pattern ordering
The tag patterns have been reordered with v*.*.* appearing after the generic *.*.* pattern. This ordering is important as it affects pattern matching precedence. Ensure this matches the intended versioning strategy.
37-144: Verify completeness of path patterns
The path patterns have been extensively updated to include all relevant Go files that could affect the manager-index component. This includes core APIs, internal packages, and manager-specific code.
✅ Verification successful
Let me run one more verification to check if there are any other potential dependencies that might be missing from the patterns.
Path patterns are complete and accurate
The verification confirms that all manager-index related files are properly covered by the existing patterns in the workflow file. The search results show:
- All critical directories are present (
cmd/manager/index,pkg/manager/index,internal/observability/metrics/manager/index) - All Go source files under these directories are included in the patterns
- Supporting files (Makefile.d entries, docker files) are also covered
- No proto files specific to manager/index were found, but the workflow already includes
apis/proto/**pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any manager-index related files that might be missing from the patterns
fd -e go . | grep -i "manager.*index" | grep -v "_test.go" | grep -v "_mock.go"
# Verify the existence of critical paths
critical_paths=(
"cmd/manager/index"
"pkg/manager/index"
"internal/observability/metrics/manager/index"
)
for path in "${critical_paths[@]}"; do
if [ -d "$path" ]; then
echo "✓ Found: $path"
else
echo "✗ Missing: $path"
fi
done
Length of output: 1184
Script:
#!/bin/bash
# Check for any files that reference manager/index
rg -l "manager/index" | grep -v "_test.go" | grep -v "_mock.go" | grep -v ".yaml" | grep -v "Dockerfile"
# Check for any proto files that might be related to manager/index
fd -e proto . | grep -i "manager.*index"
Length of output: 544
.github/workflows/dockers-helm-operator-image.yaml (4)
51-61: LGTM! Consistent path triggers for pull_request_target
The path triggers correctly mirror those in the pull_request section, maintaining security for external contributions while ensuring consistent workflow triggers.
35-45: LGTM! Comprehensive path coverage for PR triggers
The expanded path triggers now cover all relevant files that could affect the helm-operator build, including:
- Makefiles and build configurations
- Helm charts and templates
- Docker configurations
✅ Verification successful
Let me verify if this additional file is relevant by checking its content and relationship to the helm-operator build process.
Path triggers are complete and appropriate
The additional file found (k8s/metrics/grafana/dashboards/07-vald-helm-operator.yaml) is a Grafana dashboard configuration for monitoring the helm-operator. It doesn't need to be included in the workflow triggers because:
- It's a monitoring configuration that doesn't affect the helm-operator build or deployment
- Changes to monitoring dashboards are independent of the operator's functionality
The current path triggers in the workflow appropriately cover all files that could affect the helm-operator build:
- Build configurations (Makefile, Dockerfile)
- Helm charts and templates
- Core operator files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if any other relevant paths exist that should trigger the workflow
# Search for additional helm-operator related files
fd -t f -e yaml -e yml "helm-operator" | grep -v ".github/workflows"
Length of output: 124
Script:
#!/bin/bash
# Check the content of the Grafana dashboard file
cat k8s/metrics/grafana/dashboards/07-vald-helm-operator.yaml
Length of output: 25706
17-17: Verify the new workflow generator exists and is functional
The workflow generator path has changed from hack/actions/gen/main.go to hack/docker/gen/main.go.
✅ Verification successful
Let me gather more information about the generator file to ensure it's properly set up for generating Docker workflow files.
Generator path change is valid and functional
The new generator at hack/docker/gen/main.go exists and is properly set up to generate both Docker workflows and Dockerfiles. The code shows it:
- Has proper licensing and imports
- Contains workflow generation logic with GitHub Actions workflow templating
- Handles Docker-related file generation
- Generates the exact workflow file under review (
.github/workflows/dockers-*-image.yaml)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new generator exists and is executable
# Check if the new generator exists
if [ -f "hack/docker/gen/main.go" ]; then
echo "New generator exists"
# Check if it's a valid Go file
head -n 5 hack/docker/gen/main.go
else
echo "ERROR: New generator not found"
fi
Length of output: 328
Script:
#!/bin/bash
# Check the imports and main function to verify it's a workflow generator
rg -A 10 "^func main" hack/docker/gen/main.go
# Check if there are any references to workflow generation
rg -i "workflow|github" hack/docker/gen/main.go
# Look for Docker-related functionality
rg -i "docker" hack/docker/gen/main.go
Length of output: 3678
29-30: Verify alignment with versioning strategy
The workflow now triggers on both v prefixed and non-prefixed version tags. This change provides better coverage for version tag patterns.
✅ Verification successful
Let me gather more information about the versioning strategy by checking the repository documentation and related workflow files.
Let me verify the version tag patterns by checking other workflow files and configuration.
Based on the output, I can verify the version tag patterns. Let me check the git tags one more time to be thorough:
Based on the analysis of the configuration files and schema definitions, I can now provide a final response about the version tag patterns:
Version tag patterns are correctly configured
The workflow changes to support both v*.*.* and v*.*.*-* patterns are valid because:
- The patterns align with semantic versioning standards
- The schema definitions in the codebase show consistent version pattern validation using
^v[0-9]+\.[0-9]+\.[0-9]$ - The Helm charts and component configurations show version tags following this format (e.g., v1.7.15)
- Adding support for both patterns allows for:
- Release versions (v*..)
- Pre-release/special versions (v*..-*)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing tags to verify versioning pattern
# List all tags and check their patterns
git tag | grep -E '^v?[0-9]+\.[0-9]+\.[0-9]+(-.*)?$' || echo "No matching version tags found"
Length of output: 125
Script:
#!/bin/bash
# Check README and other common docs for versioning information
fd -e md -x cat {} 2>/dev/null
# Check other workflow files for version patterns
fd workflow -e yaml -e yml -x cat {} 2>/dev/null | grep -A 2 -B 2 'tags:'
Length of output: 72273
Script:
#!/bin/bash
# Check version tag patterns in workflow files
fd workflow -e yaml -e yml -x grep -A 2 -B 2 'tags:' {} \;
# Check version tags in Helm charts
fd Chart.yaml -x grep -A 2 -B 2 'version:' {} \;
# Check version tags in other configuration files
fd -e yaml -e yml -x grep -A 2 -B 2 'version:' {} \;
Length of output: 66528
Script:
#!/bin/bash
# List all git tags and check their patterns
git tag -l 'v*.*.*'
git tag -l 'v*.*.*-*'
Length of output: 45
kpango
force-pushed
the
refactor/docker-actions/standardization-generation-logic
branch
4 times, most recently
from
c10ce04 to
2b650cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
CHANGELOG.md (1)
Line range hint
1-2769: LGTM! Consider adding release dates.The changelog is well structured and maintained, with consistent formatting and comprehensive details for each version. Consider adding release dates for each version to help users track the timeline of changes more easily.
.github/workflows/dockers-gateway-lb-image.yaml (1)
33-141: Consider using YAML anchors to reduce duplicationThe paths configuration is identical between
pull_requestandpull_request_targetevents. Consider using YAML anchors and aliases to reduce duplication and improve maintainability.Example refactor:
on: # ... other triggers ... paths: &trigger_paths - "!**/*_mock.go" - "!**/*_test.go" # ... rest of paths ... pull_request: paths: *trigger_paths pull_request_target: paths: *trigger_paths.github/workflows/dockers-index-creation-image.yaml (1)
33-134: Consider optimizing path triggers for better build efficiency.While the comprehensive path list ensures no relevant changes are missed, some paths might be too broad and could trigger unnecessary builds. Consider:
- Grouping common paths using wildcards (e.g.,
internal/net/grpc/**/*.goinstead of individual subdirectories)- Limiting to only the packages directly used by the index-creation component
To help identify the essential paths, run:
#!/bin/bash # Find direct dependencies of the index-creation component fd "^go.mod$" cmd/index/job/creation/ -x grep -A 10 "require ("charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (1)
Line range hint
377-785: Improve server configuration maintainabilityThe server configuration has several areas for improvement:
- Socket options are duplicated across different sections
- Configuration fields lack descriptions for better understanding
Consider:
- Creating a reusable socket options definition
- Adding descriptions for configuration fields
+ definitions: + socketOptions: + description: "Common socket configuration options" + type: object + properties: + ip_recover_destination_addr: + type: boolean + description: "Enable IP destination address recovery" + ip_transparent: + type: boolean + description: "Enable IP transparent mode" + # ... other socket options server_config: properties: healths: description: "Health check configuration for the server" # ... existing propertiescharts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (3)
Line range hint
88-265: Add validation patterns for duration fields and numeric rangesThe job configuration structure is well-defined but could benefit from additional validation rules:
- Duration fields (e.g.,
backoff_time_limit,initial_duration) should include pattern validation to ensure correct time format (e.g., "1h", "5m").- Numeric fields like
backoff_factorand error rates should have minimum/maximum constraints.Add pattern and range validations:
backoff_time_limit: type: string + pattern: '^([0-9]+h)?([0-9]+m)?([0-9]+s)?([0-9]+ms)?([0-9]+us)?([0-9]+ns)?$' backoff_factor: type: number + minimum: 0 + maximum: 10
Line range hint
279-359: Add constraints for sampling rate and pattern validation for intervalsThe observability configuration could be improved:
- The trace sampling rate should have bounds (typically 0-100 for percentage).
- Time intervals for metrics export should validate the format.
Add constraints:
sampling_rate: type: integer + minimum: 0 + maximum: 100 metrics_export_interval: type: string + pattern: '^([0-9]+h)?([0-9]+m)?([0-9]+s)?([0-9]+ms)?([0-9]+us)?([0-9]+ns)?$'
Line range hint
387-657: Consider adding default values for critical configurationsThe server configuration is well-structured but could benefit from default values for critical settings:
- Health check probe timeouts and thresholds
- Server read/write timeouts
- Stream concurrency limits
Add default values:
failureThreshold: type: integer + default: 3 timeoutSeconds: type: integer + default: 1 bidirectional_stream_concurrency: type: integer + default: 1000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (25)
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.goexample/client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumhack/actions/gen/main.gois excluded by!**/gen/**hack/actions/gen/main_test.gois excluded by!**/gen/**hack/docker/gen/main.gois excluded by!**/gen/**hack/license/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (82)
.gitfiles(0 hunks).github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/ISSUE_TEMPLATE/security_issue_report.md(1 hunks).github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/coverage.yaml(1 hunks).github/workflows/dockers-agent-faiss-image.yaml(2 hunks).github/workflows/dockers-agent-image.yaml(2 hunks).github/workflows/dockers-agent-ngt-image.yaml(2 hunks).github/workflows/dockers-agent-sidecar-image.yaml(2 hunks).github/workflows/dockers-benchmark-job-image.yaml(2 hunks).github/workflows/dockers-benchmark-operator-image.yaml(2 hunks).github/workflows/dockers-binfmt-image.yaml(2 hunks).github/workflows/dockers-buildbase-image.yaml(2 hunks).github/workflows/dockers-buildkit-image.yaml(2 hunks).github/workflows/dockers-buildkit-syft-scanner-image.yaml(2 hunks).github/workflows/dockers-ci-container-image.yaml(2 hunks).github/workflows/dockers-dev-container-image.yaml(2 hunks).github/workflows/dockers-discoverer-k8s-image.yaml(2 hunks).github/workflows/dockers-example-client-image.yaml(2 hunks).github/workflows/dockers-gateway-filter-image.yaml(2 hunks).github/workflows/dockers-gateway-lb-image.yaml(2 hunks).github/workflows/dockers-gateway-mirror-image.yaml(2 hunks).github/workflows/dockers-helm-operator-image.yaml(2 hunks).github/workflows/dockers-index-correction-image.yaml(2 hunks).github/workflows/dockers-index-creation-image.yaml(2 hunks).github/workflows/dockers-index-deletion-image.yaml(2 hunks).github/workflows/dockers-index-deletion.yaml(0 hunks).github/workflows/dockers-index-operator-image.yaml(2 hunks).github/workflows/dockers-index-save-image.yaml(2 hunks).github/workflows/dockers-loadtest-image.yaml(2 hunks).github/workflows/dockers-manager-index-image.yaml(2 hunks).github/workflows/dockers-readreplica-rotate-image.yaml(2 hunks)CHANGELOG.md(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(0 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml(1 hunks)charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml(5 hunks)charts/vald/crds/valdmirrortarget.yaml(1 hunks)dockers/agent/core/agent/Dockerfile(1 hunks)dockers/agent/core/faiss/Dockerfile(1 hunks)dockers/agent/core/ngt/Dockerfile(1 hunks)dockers/agent/sidecar/Dockerfile(1 hunks)dockers/binfmt/Dockerfile(1 hunks)dockers/buildbase/Dockerfile(1 hunks)dockers/buildkit/Dockerfile(1 hunks)dockers/buildkit/syft/scanner/Dockerfile(1 hunks)dockers/ci/base/Dockerfile(1 hunks)dockers/dev/Dockerfile(1 hunks)dockers/discoverer/k8s/Dockerfile(1 hunks)dockers/example/client/Dockerfile(1 hunks)dockers/gateway/filter/Dockerfile(1 hunks)dockers/gateway/lb/Dockerfile(1 hunks)dockers/gateway/mirror/Dockerfile(1 hunks)dockers/index/job/correction/Dockerfile(1 hunks)dockers/index/job/creation/Dockerfile(1 hunks)dockers/index/job/deletion/Dockerfile(1 hunks)dockers/index/job/readreplica/rotate/Dockerfile(1 hunks)dockers/index/job/save/Dockerfile(1 hunks)dockers/index/operator/Dockerfile(1 hunks)dockers/manager/index/Dockerfile(1 hunks)dockers/operator/helm/Dockerfile(1 hunks)dockers/tools/benchmark/job/Dockerfile(1 hunks)dockers/tools/benchmark/operator/Dockerfile(1 hunks)dockers/tools/cli/loadtest/Dockerfile(1 hunks)example/client/go.mod(1 hunks)example/client/go.mod.default(2 hunks)go.mod(19 hunks)hack/go.mod.default(1 hunks)hack/helm/schema/crd/main.go(1 hunks)internal/file/file.go(4 hunks)internal/strings/strings.go(0 hunks)k8s/operator/helm/crds/valdhelmoperatorrelease.yaml(5 hunks)rust/rust-toolchain(1 hunks)rust/rust-toolchain.toml(1 hunks)versions/BUF_VERSION(1 hunks)versions/CMAKE_VERSION(1 hunks)versions/DOCKER_VERSION(1 hunks)versions/GOLANGCILINT_VERSION(1 hunks)versions/GO_VERSION(1 hunks)versions/HELM_VERSION(1 hunks)
⛔ Files not processed due to max files limit (18)
- versions/K3S_VERSION
- versions/KUBECTL_VERSION
- versions/OPERATOR_SDK_VERSION
- versions/PROMETHEUS_STACK_VERSION
- versions/PROTOBUF_VERSION
- versions/REVIEWDOG_VERSION
- versions/RUST_VERSION
- versions/TELEPRESENCE_VERSION
- versions/USEARCH_VERSION
- versions/YQ_VERSION
- versions/actions/ACTIONS_CACHE
- versions/actions/CODECOV_CODECOV_ACTION
- versions/actions/GITHUB_CODEQL_ACTION_ANALYZE
- versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD
- versions/actions/GITHUB_CODEQL_ACTION_INIT
- versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF
- versions/actions/GITHUB_ISSUE_METRICS
- versions/actions/REVIEWDOG_ACTION_HADOLINT
💤 Files with no reviewable changes (4)
- internal/strings/strings.go
- Makefile.d/functions.mk
- .github/workflows/dockers-index-deletion.yaml
- .gitfiles
✅ Files skipped from review due to trivial changes (26)
- dockers/buildkit/syft/scanner/Dockerfile
- dockers/binfmt/Dockerfile
- dockers/tools/cli/loadtest/Dockerfile
- dockers/example/client/Dockerfile
- dockers/agent/core/faiss/Dockerfile
- versions/DOCKER_VERSION
- dockers/index/job/creation/Dockerfile
- dockers/buildbase/Dockerfile
- dockers/gateway/filter/Dockerfile
- dockers/tools/benchmark/job/Dockerfile
- dockers/tools/benchmark/operator/Dockerfile
- dockers/agent/core/agent/Dockerfile
- dockers/index/operator/Dockerfile
- dockers/index/job/correction/Dockerfile
- dockers/discoverer/k8s/Dockerfile
- dockers/index/job/save/Dockerfile
- dockers/buildkit/Dockerfile
- dockers/agent/sidecar/Dockerfile
- dockers/ci/base/Dockerfile
- dockers/manager/index/Dockerfile
- dockers/gateway/lb/Dockerfile
- dockers/index/job/deletion/Dockerfile
- dockers/agent/core/ngt/Dockerfile
- dockers/operator/helm/Dockerfile
- dockers/index/job/readreplica/rotate/Dockerfile
- dockers/gateway/mirror/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (46)
- rust/rust-toolchain
- versions/GO_VERSION
- versions/BUF_VERSION
- versions/HELM_VERSION
- versions/CMAKE_VERSION
- versions/GOLANGCILINT_VERSION
- rust/rust-toolchain.toml
- .github/workflows/coverage.yaml
- .github/workflows/dockers-agent-faiss-image.yaml
- hack/go.mod.default
- .github/workflows/dockers-gateway-filter-image.yaml
- example/client/go.mod.default
- .github/workflows/dockers-buildkit-image.yaml
- .github/workflows/dockers-buildkit-syft-scanner-image.yaml
- hack/helm/schema/crd/main.go
- .github/workflows/dockers-binfmt-image.yaml
- .github/workflows/dockers-agent-image.yaml
- .github/workflows/dockers-dev-container-image.yaml
- .github/workflows/dockers-agent-ngt-image.yaml
- .github/ISSUE_TEMPLATE/bug_report.md
- .github/workflows/dockers-buildbase-image.yaml
- .github/ISSUE_TEMPLATE/security_issue_report.md
- internal/file/file.go
- .github/workflows/dockers-helm-operator-image.yaml
- .github/workflows/dockers-loadtest-image.yaml
- .github/workflows/dockers-index-operator-image.yaml
- Makefile
- .github/workflows/dockers-index-correction-image.yaml
- .github/workflows/dockers-benchmark-job-image.yaml
- .github/workflows/dockers-discoverer-k8s-image.yaml
- .github/workflows/dockers-index-deletion-image.yaml
- .github/workflows/dockers-example-client-image.yaml
- .github/workflows/dockers-index-save-image.yaml
- .github/workflows/dockers-readreplica-rotate-image.yaml
- .github/workflows/dockers-gateway-mirror-image.yaml
- .github/workflows/dockers-benchmark-operator-image.yaml
- .github/workflows/dockers-ci-container-image.yaml
- .github/workflows/dockers-manager-index-image.yaml
- .github/workflows/dockers-agent-sidecar-image.yaml
- .github/PULL_REQUEST_TEMPLATE.md
- charts/vald/crds/valdmirrortarget.yaml
- charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml
- k8s/operator/helm/crds/valdhelmoperatorrelease.yaml
- go.mod
- example/client/go.mod
- charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml
🔇 Additional comments (10)
dockers/dev/Dockerfile (1)
144-144:
Security concern: Container should not run as root user
Running containers as root poses security risks. Even for development containers, it's recommended to use a non-root user.
Instead of modifying this file directly, update the generator at hack/docker/gen/main.go to use a non-root user. Here's a suggested pattern:
-USER root:root
+RUN useradd -m -s /bin/bash -u 1000 vald
+USER vald:valdNote: This Dockerfile is generated, so direct modifications will be overwritten. The changes need to be made in the generator code.
Let's verify the generator code:
#!/bin/bash
# Check the generator code for user-related configurations
rg -A 5 "USER.*root" hack/docker/gen/main.go.github/workflows/dockers-gateway-lb-image.yaml (3)
17-17: Generator path change is correct but needs to be applied consistently
The generator path has been updated to hack/docker/gen/main.go, which is correct. However, as noted in previous reviews, ensure all workflow files are regenerated using this new generator for consistency.
29-30: LGTM: Extended tag patterns to support pre-release versions
The addition of v*.*.*-* pattern allows the workflow to trigger on pre-release version tags, which is a good practice for managing development releases.
145-253: Skip review of duplicate paths section
This section is identical to the pull_request paths section above.
.github/workflows/dockers-index-creation-image.yaml (2)
138-239: LGTM! Correct mirroring of paths between pull_request and pull_request_target.
The exact matching of paths between pull_request and pull_request_target events maintains security while ensuring consistent behavior.
17-17: LGTM! Verify the generator script location.
The changes to the source URL and tag patterns look good. The addition of pre-release version tags (v*.*.*-*) provides better flexibility for versioning.
Also applies to: 28-29
✅ Verification successful
Generator script location verified successfully
The workflow generator script exists at the specified location hack/docker/gen/main.go, confirming that the source URL comment in the workflow file is accurate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the workflow generator script exists at the new location
fd "main.go$" hack/docker/gen/
Length of output: 56
charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (2)
246-271: Enhance dataset configuration validation
The dataset configuration could benefit from additional validation rules.
dataset:
properties:
group:
minLength: 1
type: string
+ pattern: "^[a-z0-9][a-z0-9-]*[a-z0-9]$"
+ description: "Dataset group identifier (lowercase alphanumeric with hyphens)"
range:
properties:
end:
minimum: 1
type: integer
+ maximum: 1000000
start:
minimum: 1
type: integer
+ maximum: 1000000
required:
- start
- end
+ x-kubernetes-validations:
+ - rule: "self.start <= self.end"
+ message: "start must be less than or equal to end"787-822: LGTM! Job configuration is well-structured
The job configuration section has appropriate validation rules and bounds for TTL values.
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (2)
Line range hint 671-706: LGTM: Well-structured service and security configurations
The service and security configurations follow Kubernetes best practices:
- Proper enumeration of service types
- Flexible security context using preserve-unknown-fields
- Clear separation of concerns between service and security configurations
Line range hint 1-706: Verify existing CRD instances for compatibility
The schema changes look good overall. Let's verify if any existing instances of this CRD need updates to comply with the new schema.
✅ Verification successful
CRD schema changes are compatible and well-structured
The CRD schema changes are verified to be compatible and properly structured. All duration fields are consistently defined as type: string across both CRD instances (in charts/ and k8s/tools/), and no existing resource instances were found that could be impacted by these changes. The schema provides proper validation for all configuration fields including durations, timeouts, and other time-based settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing ValdBenchmarkOperatorRelease instances
# and validate their compatibility with the new schema
# Find all YAML files that might contain ValdBenchmarkOperatorRelease instances
echo "Searching for ValdBenchmarkOperatorRelease instances..."
fd -e yaml -e yml | xargs grep -l "kind: ValdBenchmarkOperatorRelease" || true
# Check for any duration fields that might not match the expected format
echo "Checking duration field formats..."
rg -l "duration:|timeout:" $(fd -e yaml -e yml) || true
Length of output: 15139
Script:
#!/bin/bash
# Let's analyze the content of the existing instances to verify compatibility
# Check the content of the CRD instances found
echo "=== Checking CRD instance in charts ==="
cat charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml
echo -e "\n=== Checking CRD instance in k8s/tools ==="
cat k8s/tools/benchmark/operator/crds/valdbenchmarkoperatorrelease.yaml
# Let's also check for any sample configurations that might be affected
echo -e "\n=== Checking sample configurations ==="
cat cmd/tools/benchmark/operator/sample.yaml || true
Length of output: 64390
Script:
#!/bin/bash
# Let's check for any existing instances of this CRD in the cluster configurations
# and verify the duration field formats in sample configurations
# Search for any ValdBenchmarkOperatorRelease resource definitions
echo "=== Checking for ValdBenchmarkOperatorRelease resource instances ==="
fd -e yaml -e yml | xargs grep -l "kind: ValdBenchmarkOperatorRelease" | while read -r file; do
echo "=== Checking $file ==="
# Look for duration fields and their values
awk '/duration:|timeout:/ {p=NR+1}(NR<=p){print}' "$file"
done
# Check for any example or sample configurations
echo -e "\n=== Checking example configurations ==="
fd -e yaml -e yml -p "example|sample" | while read -r file; do
if grep -q "ValdBenchmarkOperator" "$file"; then
echo "=== Found in $file ==="
grep -A 1 "duration:\|timeout:" "$file" || true
fi
done
Length of output: 10068
kpango
force-pushed
the
refactor/docker-actions/standardization-generation-logic
branch
from
2b650cd to
c3419a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml (1)
124-130: Consider expanding logging level options.The logging level enum is limited to debug/info/error. Consider adding 'warn' and 'fatal' levels for more granular logging control, similar to the benchmark operator implementation.
level: enum: - debug - info + - warn - error + - fatal type: stringk8s/operator/helm/crds/valdhelmoperatorrelease.yaml (1)
Line range hint
1-228: Consider consolidating duplicate CRD definitions.This file is identical to
charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml. Having duplicate CRD definitions increases maintenance burden and risk of inconsistencies.Consider:
- Maintaining a single source of truth for CRD definitions
- Using symbolic links or a build process to replicate the file where needed
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (1)
Line range hint
88-279: Consider adding property descriptions.The CRD schema would benefit from descriptions for complex properties, especially in the client_config section. This would improve usability and maintainability.
Example for client_config:
client_config: + description: "Configuration for the benchmark client including connection, backoff, and circuit breaker settings" properties: backoff: + description: "Backoff configuration for retry mechanisms" properties:charts/vald-benchmark-operator/values.yaml (1)
174-174: LGTM! Enhanced observability with MetricInterceptor.The addition of MetricInterceptor alongside TraceInterceptor improves the observability stack by enabling both tracing and metrics collection.
Consider implementing a monitoring dashboard that utilizes these metrics for better system observability.
internal/file/file.go (3)
39-41: Consider using a more descriptive constant name.While
sepis concise, consider using a more descriptive name likepathSeparatorordirSeparatorto improve code readability.
Line range hint
545-559: Simplify path handling and avoid unnecessary filesystem operations.The current implementation has several concerns:
- Unnecessary
Existschecks for path cleaning- Potential race conditions when checking path existence
- Performance impact from multiple filesystem operations
Consider simplifying the logic to use
filepath.Cleanandfilepath.Abswithout existence checks:- if filepath.IsAbs(path) || !Exists(path) { - return filepath.Clean(path) - } - - root, err := os.Getwd() - if err != nil { - err = errors.ErrFailedToGetAbsPath(err, path) - log.Warn(err) - return filepath.Clean(path) - } - abs := joinFilePaths(root, path) - if !Exists(abs) { - return filepath.Clean(path) - } - return filepath.Clean(abs) + cleaned := filepath.Clean(path) + if filepath.IsAbs(cleaned) { + return cleaned + } + abs, err := filepath.Abs(cleaned) + if err != nil { + log.Warn(errors.ErrFailedToGetAbsPath(err, path)) + return cleaned + } + return abs
572-572: Consider usingfilepath.Joinfor path joining.Instead of manual string joining with separators, consider using the standard library's
filepath.Joinwhich handles path separators correctly:- return replacer.Replace(strings.Join(paths[i:], sep)) + return filepath.Join(paths[i:]...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (25)
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.goexample/client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumhack/actions/gen/main.gois excluded by!**/gen/**hack/actions/gen/main_test.gois excluded by!**/gen/**hack/docker/gen/main.gois excluded by!**/gen/**hack/license/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (82)
.gitfiles(0 hunks).github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/ISSUE_TEMPLATE/security_issue_report.md(1 hunks).github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/coverage.yaml(1 hunks).github/workflows/dockers-agent-faiss-image.yaml(2 hunks).github/workflows/dockers-agent-image.yaml(2 hunks).github/workflows/dockers-agent-ngt-image.yaml(2 hunks).github/workflows/dockers-agent-sidecar-image.yaml(2 hunks).github/workflows/dockers-benchmark-job-image.yaml(2 hunks).github/workflows/dockers-benchmark-operator-image.yaml(2 hunks).github/workflows/dockers-binfmt-image.yaml(2 hunks).github/workflows/dockers-buildbase-image.yaml(2 hunks).github/workflows/dockers-buildkit-image.yaml(2 hunks).github/workflows/dockers-buildkit-syft-scanner-image.yaml(2 hunks).github/workflows/dockers-ci-container-image.yaml(2 hunks).github/workflows/dockers-dev-container-image.yaml(2 hunks).github/workflows/dockers-discoverer-k8s-image.yaml(2 hunks).github/workflows/dockers-example-client-image.yaml(2 hunks).github/workflows/dockers-gateway-filter-image.yaml(2 hunks).github/workflows/dockers-gateway-lb-image.yaml(2 hunks).github/workflows/dockers-gateway-mirror-image.yaml(2 hunks).github/workflows/dockers-helm-operator-image.yaml(2 hunks).github/workflows/dockers-index-correction-image.yaml(2 hunks).github/workflows/dockers-index-creation-image.yaml(2 hunks).github/workflows/dockers-index-deletion-image.yaml(2 hunks).github/workflows/dockers-index-deletion.yaml(0 hunks).github/workflows/dockers-index-operator-image.yaml(2 hunks).github/workflows/dockers-index-save-image.yaml(2 hunks).github/workflows/dockers-loadtest-image.yaml(2 hunks).github/workflows/dockers-manager-index-image.yaml(2 hunks).github/workflows/dockers-readreplica-rotate-image.yaml(2 hunks).github/workflows/unit-test.yaml(1 hunks)CHANGELOG.md(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(0 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml(1 hunks)charts/vald-benchmark-operator/schemas/job-values.yaml(1 hunks)charts/vald-benchmark-operator/values.yaml(2 hunks)charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml(5 hunks)charts/vald/crds/valdmirrortarget.yaml(1 hunks)charts/vald/values.yaml(1 hunks)dockers/agent/core/agent/Dockerfile(1 hunks)dockers/agent/core/faiss/Dockerfile(1 hunks)dockers/agent/core/ngt/Dockerfile(1 hunks)dockers/agent/sidecar/Dockerfile(1 hunks)dockers/binfmt/Dockerfile(1 hunks)dockers/buildbase/Dockerfile(1 hunks)dockers/buildkit/Dockerfile(1 hunks)dockers/buildkit/syft/scanner/Dockerfile(1 hunks)dockers/ci/base/Dockerfile(1 hunks)dockers/dev/Dockerfile(1 hunks)dockers/discoverer/k8s/Dockerfile(1 hunks)dockers/example/client/Dockerfile(1 hunks)dockers/gateway/filter/Dockerfile(1 hunks)dockers/gateway/lb/Dockerfile(1 hunks)dockers/gateway/mirror/Dockerfile(1 hunks)dockers/index/job/correction/Dockerfile(1 hunks)dockers/index/job/creation/Dockerfile(1 hunks)dockers/index/job/deletion/Dockerfile(1 hunks)dockers/index/job/readreplica/rotate/Dockerfile(1 hunks)dockers/index/job/save/Dockerfile(1 hunks)dockers/index/operator/Dockerfile(1 hunks)dockers/manager/index/Dockerfile(1 hunks)dockers/operator/helm/Dockerfile(1 hunks)dockers/tools/benchmark/job/Dockerfile(1 hunks)dockers/tools/benchmark/operator/Dockerfile(1 hunks)dockers/tools/cli/loadtest/Dockerfile(1 hunks)example/client/go.mod(1 hunks)example/client/go.mod.default(2 hunks)go.mod(19 hunks)hack/go.mod.default(1 hunks)hack/helm/schema/crd/main.go(1 hunks)internal/file/file.go(4 hunks)internal/strings/strings.go(0 hunks)k8s/operator/helm/crds/valdhelmoperatorrelease.yaml(5 hunks)rust/rust-toolchain(1 hunks)rust/rust-toolchain.toml(1 hunks)versions/BUF_VERSION(1 hunks)versions/CMAKE_VERSION(1 hunks)
⛔ Files not processed due to max files limit (22)
- versions/DOCKER_VERSION
- versions/GOLANGCILINT_VERSION
- versions/GO_VERSION
- versions/HELM_VERSION
- versions/K3S_VERSION
- versions/KUBECTL_VERSION
- versions/OPERATOR_SDK_VERSION
- versions/PROMETHEUS_STACK_VERSION
- versions/PROTOBUF_VERSION
- versions/REVIEWDOG_VERSION
- versions/RUST_VERSION
- versions/TELEPRESENCE_VERSION
- versions/USEARCH_VERSION
- versions/YQ_VERSION
- versions/actions/ACTIONS_CACHE
- versions/actions/CODECOV_CODECOV_ACTION
- versions/actions/GITHUB_CODEQL_ACTION_ANALYZE
- versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD
- versions/actions/GITHUB_CODEQL_ACTION_INIT
- versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF
- versions/actions/GITHUB_ISSUE_METRICS
- versions/actions/REVIEWDOG_ACTION_HADOLINT
💤 Files with no reviewable changes (4)
- internal/strings/strings.go
- Makefile.d/functions.mk
- .github/workflows/dockers-index-deletion.yaml
- .gitfiles
✅ Files skipped from review due to trivial changes (4)
- dockers/gateway/filter/Dockerfile
- dockers/tools/cli/loadtest/Dockerfile
- .github/workflows/unit-test.yaml
- charts/vald/values.yaml
🚧 Files skipped from review as they are similar to previous changes (66)
- versions/CMAKE_VERSION
- rust/rust-toolchain
- versions/BUF_VERSION
- dockers/tools/benchmark/operator/Dockerfile
- dockers/buildkit/syft/scanner/Dockerfile
- dockers/buildkit/Dockerfile
- dockers/index/job/creation/Dockerfile
- dockers/buildbase/Dockerfile
- dockers/index/job/correction/Dockerfile
- dockers/gateway/lb/Dockerfile
- dockers/binfmt/Dockerfile
- dockers/index/operator/Dockerfile
- dockers/discoverer/k8s/Dockerfile
- dockers/agent/core/faiss/Dockerfile
- dockers/tools/benchmark/job/Dockerfile
- dockers/manager/index/Dockerfile
- hack/go.mod.default
- dockers/gateway/mirror/Dockerfile
- dockers/index/job/save/Dockerfile
- .github/workflows/dockers-loadtest-image.yaml
- .github/workflows/dockers-binfmt-image.yaml
- .github/workflows/coverage.yaml
- .github/workflows/dockers-agent-ngt-image.yaml
- .github/workflows/dockers-buildkit-syft-scanner-image.yaml
- .github/workflows/dockers-index-creation-image.yaml
- .github/workflows/dockers-buildbase-image.yaml
- .github/workflows/dockers-index-operator-image.yaml
- .github/workflows/dockers-agent-image.yaml
- .github/workflows/dockers-helm-operator-image.yaml
- .github/workflows/dockers-index-save-image.yaml
- .github/workflows/dockers-agent-sidecar-image.yaml
- .github/workflows/dockers-manager-index-image.yaml
- .github/workflows/dockers-ci-container-image.yaml
- .github/workflows/dockers-example-client-image.yaml
- .github/PULL_REQUEST_TEMPLATE.md
- .github/workflows/dockers-discoverer-k8s-image.yaml
- .github/ISSUE_TEMPLATE/bug_report.md
- dockers/operator/helm/Dockerfile
- dockers/agent/core/agent/Dockerfile
- dockers/index/job/deletion/Dockerfile
- dockers/index/job/readreplica/rotate/Dockerfile
- rust/rust-toolchain.toml
- hack/helm/schema/crd/main.go
- dockers/example/client/Dockerfile
- dockers/agent/sidecar/Dockerfile
- .github/workflows/dockers-gateway-filter-image.yaml
- Makefile
- .github/workflows/dockers-agent-faiss-image.yaml
- example/client/go.mod.default
- .github/workflows/dockers-dev-container-image.yaml
- dockers/agent/core/ngt/Dockerfile
- dockers/ci/base/Dockerfile
- .github/workflows/dockers-gateway-lb-image.yaml
- .github/ISSUE_TEMPLATE/security_issue_report.md
- .github/workflows/dockers-index-correction-image.yaml
- .github/workflows/dockers-buildkit-image.yaml
- .github/workflows/dockers-readreplica-rotate-image.yaml
- .github/workflows/dockers-gateway-mirror-image.yaml
- dockers/dev/Dockerfile
- .github/workflows/dockers-index-deletion-image.yaml
- .github/workflows/dockers-benchmark-operator-image.yaml
- charts/vald/crds/valdmirrortarget.yaml
- charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml
- example/client/go.mod
- go.mod
- CHANGELOG.md
🔇 Additional comments (14)
charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml (1)
208-212: Add support for ExternalName service type.
The service type enum is missing the 'ExternalName' option, which is a valid Kubernetes service type.
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (1)
685-688: Add support for ExternalName service type.
The service type enum is missing the 'ExternalName' option, which is a valid Kubernetes service type.
charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (2)
246-271: Enhance dataset configuration validation.
The dataset configuration could benefit from additional validation rules.
142-143:
Update interceptor enum to match values.yaml configuration.
The interceptor enum in the CRD should be updated to include "MetricInterceptor" to maintain consistency with the values.yaml configuration.
Apply this diff to align with values.yaml:
enum:
- TraceInterceptor
+ - MetricInterceptorLikely invalid or redundant comment.
charts/vald-benchmark-operator/values.yaml (1)
410-410: LGTM! Comprehensive server interceptor configuration.
The server interceptor configuration provides a complete set of interceptors for recovery, access logging, tracing, and metrics collection.
charts/vald-benchmark-operator/schemas/job-values.yaml (1)
270-270: LGTM! Consistent interceptor schema definition.
The client interceptor schema correctly includes both TraceInterceptor and MetricInterceptor, maintaining consistency with the values.yaml configuration.
.github/workflows/dockers-benchmark-job-image.yaml (6)
33-34: LGTM: Proper test file exclusions
The explicit exclusion of mock and test files is appropriate as they shouldn't trigger Docker image rebuilds.
40-143: Consider adding missing benchmark operator paths
While the path coverage is extensive, there might be missing benchmark operator paths that could affect the build.
#!/bin/bash
# Description: Verify coverage of benchmark-related paths
# Check for any benchmark-related files that might be missing from the trigger paths
fd -t f . -e go | grep -i "benchmark" | grep -v -E "(_test.go|_mock.go|/job/|/operators/)" | sort -u
# Check for operator-specific paths
fd -t f . -e go | grep -i "benchmark.*operator" | grep -v -E "(_test.go|_mock.go)" | sort -u147-257: LGTM: Consistent path triggers
The pull_request_target paths correctly mirror the pull_request paths, maintaining consistent build triggers across both events.
Line range hint 259-264: LGTM: Proper workflow reuse
The build job correctly reuses the shared Docker image workflow template, maintaining consistency across different image builds.
29-30: LGTM: Enhanced semantic versioning support
The addition of v*.*.* and v*.*.*-* patterns improves semantic versioning support, allowing for both release (e.g., v1.2.3) and pre-release (e.g., v1.2.3-alpha) tags.
✅ Verification successful
Let me verify if all these workflow files also have the pre-release tag pattern (v*.*.*-*).
Tag patterns are consistently applied across Docker workflow files
The verification confirms that both semantic versioning patterns (v*.*.* and v*.*.*-*) are consistently implemented across all Docker workflow files in the repository, ensuring uniform version tag handling for all Docker images.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify tag pattern consistency across workflow files
# Check if other Docker workflow files use consistent tag patterns
rg -l "v\*\.\*\.\*" .github/workflows/dockers-*-image.yaml
Length of output: 2749
Script:
#!/bin/bash
# Check if other Docker workflow files use the pre-release tag pattern
rg -l "v\*\.\*\.\*-\*" .github/workflows/dockers-*-image.yaml
Length of output: 2700
17-17: Verify the new workflow generator location
The workflow generator has been moved from hack/actions/gen/main.go to hack/docker/gen/main.go, which aligns with the refactoring objective.
✅ Verification successful
Generator path change is valid and complete
The verification confirms that:
- The new generator exists at
hack/docker/gen/main.goand contains the necessary code for workflow generation - No references to the old path
hack/actions/genremain in the workflow files - The new generator's code shows it's properly handling Docker and GitHub Actions workflow generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and imports of the new workflow generator
# Check if the new generator exists and contains relevant imports
rg -l "package main" hack/docker/gen/main.go
# Check for any references to the old generator path to ensure complete migration
rg -l "hack/actions/gen/main.go" .github/workflows/
Length of output: 122
Script:
#!/bin/bash
# Let's verify both the old and new paths, and check the content of the new generator
# Check if the old generator path exists
fd "main.go" hack/actions/gen/
# Check the content of the new generator to verify it's the correct file
cat hack/docker/gen/main.go
# Check for any other workflow files that might still reference the old path
rg "hack/actions/gen" .github/
Length of output: 38809
internal/file/file.go (2)
563-566: LGTM!
The path separator replacer is correctly implemented to handle multiple consecutive separators.
Line range hint 39-572: Verify path handling changes across the codebase.
Let's verify the impact of these path handling changes:
✅ Verification successful
Let me gather more specific information about the path handling patterns.
Let me gather more specific information about the path handling in the file under review.
Let me gather more information about the path handling implementation.
Based on the gathered information, I can now provide the final response:
Path handling implementation is consistent and safe
The implementation follows best practices for path handling:
- Uses
filepathpackage functions consistently for cross-platform compatibility - Properly handles path separators using OS-specific separator constant
- Implements path cleaning and normalization
- Avoids direct string concatenation for paths
- Has safeguards against path traversal via
filepath.Clean
The Join function implementation is robust:
- Handles nil/empty cases
- Uses platform-specific path separators
- Normalizes multiple separators
- Preserves absolute paths
- Cleans the resulting path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential path handling issues across the codebase
# Check for direct path separator usage
echo "Checking for direct path separator usage..."
rg -l "os\.PathSeparator|filepath\.Separator" --type go
# Check for manual path joining patterns
echo "Checking for manual path joining patterns..."
rg -l "strings\.Join.*\+.*/" --type go
# Check for path existence checks
echo "Checking for path existence patterns..."
ast-grep --pattern 'if Exists($path) { $$$; return $$$; }'
Length of output: 705
Script:
#!/bin/bash
# Check the usage of the Join function and path handling in the codebase
# Check for Join function usage
echo "Checking Join function usage patterns..."
rg -A 2 "Join\(" --type go
# Check for filepath.Join usage
echo "Checking filepath.Join usage patterns..."
rg -A 2 "filepath\.Join\(" --type go
# Check for path separator constant usage
echo "Checking path separator constant usage..."
rg "sep\s*=\s*string\(os\.PathSeparator\)" --type go -A 2
# Check for direct string concatenation with "/"
echo "Checking direct path concatenation..."
rg "\+\s*\"/\"|\"/\"\s*\+" --type go
Length of output: 66444
Script:
#!/bin/bash
# Check the implementation and usage of the Join function in file.go
# Check the Join function implementation
echo "Checking Join function implementation..."
rg -A 10 "func Join\(" internal/file/file.go
# Check for path separator usage in file.go
echo "Checking path separator usage in file.go..."
rg "sep\s*=\s*string\(os\.PathSeparator\)" internal/file/file.go -A 2
# Check for filepath.Join usage in file.go
echo "Checking filepath.Join usage in file.go..."
rg "filepath\.Join\(" internal/file/file.go -A 2
# Check for direct path concatenation in file.go
echo "Checking direct path concatenation in file.go..."
rg "\+\s*\"/\"|\"/\"\s*\+" internal/file/file.go
Length of output: 895
Script:
#!/bin/bash
# Check for the complete implementation details
# Check the joinFilePaths function implementation
echo "Checking joinFilePaths implementation..."
ast-grep --pattern 'func joinFilePaths($$$) $$$'
# Check the replacer variable definition
echo "Checking replacer definition..."
rg "replacer\s*=\s*strings\.NewReplacer" -A 5
# Check for path handling in the file package
echo "Checking path handling patterns..."
rg "filepath\.(Clean|Dir|Base|IsAbs)" internal/file/file.go -A 2
Length of output: 1649
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: kpango <kpango@vdaas.org>
kpango
dismissed stale reviews from vankichi and coderabbitai[bot]
via
4e6ba7c
kpango
force-pushed
the
refactor/docker-actions/standardization-generation-logic
branch
from
c3419a4 to
4e6ba7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml (1)
117-136: Consider expanding log levelsThe current log level enums are limited to
debug,info, anderror. Consider adding common log levels likewarn/warningandfatalto provide more granular control over logging.level: enum: - debug - info + - warn - error + - fatal type: string stacktraceLevel: enum: - debug - info + - warn - error + - fatal type: stringk8s/operator/helm/crds/valdhelmoperatorrelease.yaml (2)
117-136: Consider expanding logging configuration options.The current logging configuration could be enhanced with additional common options:
- Add 'warn' level between 'info' and 'error'
- Consider supporting 'logfmt' format alongside 'console' and 'json'
These additions would provide more granular control over logging behavior.
format: enum: - console - json + - logfmt type: string level: enum: - debug - info + - warn - error type: string stacktraceLevel: enum: - debug - info + - warn - error type: string
221-225: Consider explicitly defining toleration fields.While using
x-kubernetes-preserve-unknown-fieldsprovides flexibility, explicitly defining common toleration fields would improve schema validation and documentation.tolerations: items: type: object - x-kubernetes-preserve-unknown-fields: true + properties: + key: + type: string + operator: + type: string + enum: + - Exists + - Equal + value: + type: string + effect: + type: string + enum: + - NoSchedule + - PreferNoSchedule + - NoExecute + tolerationSeconds: + type: integer type: arrayCHANGELOG.md (1)
Line range hint
1-6130: LGTM! Well-structured changelog with consistent formatting.The changelog is well organized with clear sections for Docker images, documents and changes for each version. The use of emojis/prefixes helps categorize different types of changes.
Some minor suggestions for even better consistency:
- Consider adding a "Breaking Changes" section when applicable to highlight major changes that require user action
- Consider standardizing the categorization prefixes across all versions (some versions use emojis while others use text categories)
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (2)
Line range hint
74-241: Consider adding timeout validation constraints.While the type definitions are well-structured, consider adding validation for timeout values to prevent configuration errors. For example:
backoff_time_limitinitial_durationmaximum_durationclosed_refresh_timeoutopen_timeoutThese fields could benefit from pattern validation to ensure valid duration formats and reasonable ranges.
Example pattern validation:
backoff_time_limit: type: string + pattern: '^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$' + description: "Duration string (e.g., 1h30m). Valid units: ns, us, µs, ms, s, m, h"
Line range hint
677-694: Add validation for externalTrafficPolicy.Consider adding an enum validation for
externalTrafficPolicyto restrict it to valid Kubernetes values:externalTrafficPolicy: type: string + enum: + - Local + - Cluster + description: "Denotes if this Service desires to route external traffic to node-local or cluster-wide endpoints".github/workflows/dockers-gateway-lb-image.yaml (1)
33-253: Consider refactoring duplicated path configurationsThe path configurations are duplicated between
pull_requestandpull_request_targetsections. Consider using a reusable workflow or GitHub Actions composite action to maintain these paths in a single location.Example approach:
- Create a new file
.github/workflows/paths/gateway-lb-paths.yaml- Reference it in both sections using composite actions
# .github/workflows/paths/gateway-lb-paths.yaml paths: - "!**/*_mock.go" - "!**/*_test.go" # ... rest of the pathsinternal/file/file.go (1)
Line range hint
545-559: Optimize path existence checks.The current implementation performs multiple filesystem operations:
- First
Existscheck for the initial path- Second
Existscheck for the absolute pathos.Getwd()call between themConsider caching the existence check results or deferring the checks until absolutely necessary.
- if filepath.IsAbs(path) || !Exists(path) { + if filepath.IsAbs(path) { return filepath.Clean(path) } root, err := os.Getwd() if err != nil { err = errors.ErrFailedToGetAbsPath(err, path) log.Warn(err) return filepath.Clean(path) } abs := joinFilePaths(root, path) - if !Exists(abs) { - return filepath.Clean(path) - } return filepath.Clean(abs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (25)
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.goexample/client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumhack/actions/gen/main.gois excluded by!**/gen/**hack/actions/gen/main_test.gois excluded by!**/gen/**hack/docker/gen/main.gois excluded by!**/gen/**hack/license/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (82)
.gitfiles(0 hunks).github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/ISSUE_TEMPLATE/security_issue_report.md(1 hunks).github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/coverage.yaml(1 hunks).github/workflows/dockers-agent-faiss-image.yaml(2 hunks).github/workflows/dockers-agent-image.yaml(2 hunks).github/workflows/dockers-agent-ngt-image.yaml(2 hunks).github/workflows/dockers-agent-sidecar-image.yaml(2 hunks).github/workflows/dockers-benchmark-job-image.yaml(2 hunks).github/workflows/dockers-benchmark-operator-image.yaml(2 hunks).github/workflows/dockers-binfmt-image.yaml(2 hunks).github/workflows/dockers-buildbase-image.yaml(2 hunks).github/workflows/dockers-buildkit-image.yaml(2 hunks).github/workflows/dockers-buildkit-syft-scanner-image.yaml(2 hunks).github/workflows/dockers-ci-container-image.yaml(2 hunks).github/workflows/dockers-dev-container-image.yaml(2 hunks).github/workflows/dockers-discoverer-k8s-image.yaml(2 hunks).github/workflows/dockers-example-client-image.yaml(2 hunks).github/workflows/dockers-gateway-filter-image.yaml(2 hunks).github/workflows/dockers-gateway-lb-image.yaml(2 hunks).github/workflows/dockers-gateway-mirror-image.yaml(2 hunks).github/workflows/dockers-helm-operator-image.yaml(2 hunks).github/workflows/dockers-index-correction-image.yaml(2 hunks).github/workflows/dockers-index-creation-image.yaml(2 hunks).github/workflows/dockers-index-deletion-image.yaml(2 hunks).github/workflows/dockers-index-deletion.yaml(0 hunks).github/workflows/dockers-index-operator-image.yaml(2 hunks).github/workflows/dockers-index-save-image.yaml(2 hunks).github/workflows/dockers-loadtest-image.yaml(2 hunks).github/workflows/dockers-manager-index-image.yaml(2 hunks).github/workflows/dockers-readreplica-rotate-image.yaml(2 hunks).github/workflows/unit-test.yaml(1 hunks)CHANGELOG.md(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(0 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml(27 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml(1 hunks)charts/vald-benchmark-operator/schemas/job-values.schema.json(1 hunks)charts/vald-benchmark-operator/schemas/job-values.yaml(1 hunks)charts/vald-benchmark-operator/values.yaml(2 hunks)charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml(5 hunks)charts/vald/crds/valdmirrortarget.yaml(1 hunks)charts/vald/values.yaml(1 hunks)dockers/agent/core/agent/Dockerfile(1 hunks)dockers/agent/core/faiss/Dockerfile(1 hunks)dockers/agent/core/ngt/Dockerfile(1 hunks)dockers/agent/sidecar/Dockerfile(1 hunks)dockers/binfmt/Dockerfile(1 hunks)dockers/buildbase/Dockerfile(1 hunks)dockers/buildkit/Dockerfile(1 hunks)dockers/buildkit/syft/scanner/Dockerfile(1 hunks)dockers/ci/base/Dockerfile(1 hunks)dockers/dev/Dockerfile(1 hunks)dockers/discoverer/k8s/Dockerfile(1 hunks)dockers/example/client/Dockerfile(1 hunks)dockers/gateway/filter/Dockerfile(1 hunks)dockers/gateway/lb/Dockerfile(1 hunks)dockers/gateway/mirror/Dockerfile(1 hunks)dockers/index/job/correction/Dockerfile(1 hunks)dockers/index/job/creation/Dockerfile(1 hunks)dockers/index/job/deletion/Dockerfile(1 hunks)dockers/index/job/readreplica/rotate/Dockerfile(1 hunks)dockers/index/job/save/Dockerfile(1 hunks)dockers/index/operator/Dockerfile(1 hunks)dockers/manager/index/Dockerfile(1 hunks)dockers/operator/helm/Dockerfile(1 hunks)dockers/tools/benchmark/job/Dockerfile(1 hunks)dockers/tools/benchmark/operator/Dockerfile(1 hunks)dockers/tools/cli/loadtest/Dockerfile(1 hunks)example/client/go.mod(1 hunks)example/client/go.mod.default(2 hunks)go.mod(18 hunks)hack/go.mod.default(1 hunks)hack/helm/schema/crd/main.go(1 hunks)internal/file/file.go(4 hunks)internal/strings/strings.go(0 hunks)k8s/operator/helm/crds/valdhelmoperatorrelease.yaml(5 hunks)rust/rust-toolchain(1 hunks)rust/rust-toolchain.toml(1 hunks)versions/BUF_VERSION(1 hunks)
⛔ Files not processed due to max files limit (26)
- versions/CMAKE_VERSION
- versions/DOCKER_VERSION
- versions/GOLANGCILINT_VERSION
- versions/GO_VERSION
- versions/HELM_VERSION
- versions/K3S_VERSION
- versions/KUBECTL_VERSION
- versions/NGT_VERSION
- versions/OPERATOR_SDK_VERSION
- versions/PROMETHEUS_STACK_VERSION
- versions/PROTOBUF_VERSION
- versions/REVIEWDOG_VERSION
- versions/RUST_VERSION
- versions/TELEPRESENCE_VERSION
- versions/USEARCH_VERSION
- versions/YQ_VERSION
- versions/actions/ACTIONS_CACHE
- versions/actions/ACTIONS_SETUP_GO
- versions/actions/CODECOV_CODECOV_ACTION
- versions/actions/GITHUB_CODEQL_ACTION_ANALYZE
- versions/actions/GITHUB_CODEQL_ACTION_AUTOBUILD
- versions/actions/GITHUB_CODEQL_ACTION_INIT
- versions/actions/GITHUB_CODEQL_ACTION_UPLOAD_SARIF
- versions/actions/GITHUB_ISSUE_METRICS
- versions/actions/REVIEWDOG_ACTION_HADOLINT
- versions/actions/SOFTPROPS_ACTION_GH_RELEASE
💤 Files with no reviewable changes (4)
- internal/strings/strings.go
- Makefile.d/functions.mk
- .github/workflows/dockers-index-deletion.yaml
- .gitfiles
🚧 Files skipped from review as they are similar to previous changes (70)
- versions/BUF_VERSION
- dockers/buildbase/Dockerfile
- dockers/manager/index/Dockerfile
- rust/rust-toolchain
- dockers/example/client/Dockerfile
- dockers/agent/core/agent/Dockerfile
- dockers/gateway/mirror/Dockerfile
- dockers/index/operator/Dockerfile
- dockers/discoverer/k8s/Dockerfile
- dockers/buildkit/Dockerfile
- dockers/index/job/creation/Dockerfile
- dockers/index/job/save/Dockerfile
- hack/helm/schema/crd/main.go
- dockers/agent/sidecar/Dockerfile
- .github/workflows/dockers-buildkit-image.yaml
- dockers/gateway/filter/Dockerfile
- dockers/operator/helm/Dockerfile
- dockers/gateway/lb/Dockerfile
- dockers/buildkit/syft/scanner/Dockerfile
- dockers/index/job/deletion/Dockerfile
- dockers/agent/core/faiss/Dockerfile
- dockers/tools/benchmark/job/Dockerfile
- dockers/binfmt/Dockerfile
- dockers/tools/cli/loadtest/Dockerfile
- .github/ISSUE_TEMPLATE/security_issue_report.md
- dockers/tools/benchmark/operator/Dockerfile
- .github/workflows/unit-test.yaml
- dockers/index/job/correction/Dockerfile
- .github/workflows/dockers-buildkit-syft-scanner-image.yaml
- rust/rust-toolchain.toml
- .github/workflows/dockers-agent-image.yaml
- .github/workflows/dockers-agent-ngt-image.yaml
- dockers/index/job/readreplica/rotate/Dockerfile
- dockers/ci/base/Dockerfile
- .github/workflows/dockers-helm-operator-image.yaml
- .github/workflows/dockers-discoverer-k8s-image.yaml
- dockers/agent/core/ngt/Dockerfile
- .github/workflows/dockers-buildbase-image.yaml
- example/client/go.mod.default
- .github/workflows/dockers-index-correction-image.yaml
- .github/workflows/dockers-loadtest-image.yaml
- .github/workflows/dockers-gateway-filter-image.yaml
- .github/workflows/dockers-benchmark-job-image.yaml
- .github/ISSUE_TEMPLATE/bug_report.md
- .github/workflows/dockers-dev-container-image.yaml
- hack/go.mod.default
- dockers/dev/Dockerfile
- charts/vald-benchmark-operator/schemas/job-values.yaml
- .github/workflows/dockers-benchmark-operator-image.yaml
- .github/workflows/dockers-agent-faiss-image.yaml
- .github/workflows/dockers-agent-sidecar-image.yaml
- .github/workflows/dockers-readreplica-rotate-image.yaml
- .github/workflows/dockers-index-operator-image.yaml
- .github/PULL_REQUEST_TEMPLATE.md
- .github/workflows/dockers-gateway-mirror-image.yaml
- .github/workflows/dockers-index-save-image.yaml
- Makefile
- .github/workflows/dockers-index-deletion-image.yaml
- .github/workflows/dockers-example-client-image.yaml
- .github/workflows/dockers-manager-index-image.yaml
- .github/workflows/dockers-ci-container-image.yaml
- charts/vald-benchmark-operator/values.yaml
- .github/workflows/dockers-index-creation-image.yaml
- .github/workflows/dockers-binfmt-image.yaml
- .github/workflows/coverage.yaml
- charts/vald/crds/valdmirrortarget.yaml
- charts/vald/values.yaml
- charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml
- go.mod
- example/client/go.mod
🔇 Additional comments (21)
charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (4)
142-144: LGTM: MetricInterceptor addition improves observability
The addition of MetricInterceptor to the client interceptors enhances the observability capabilities as previously suggested.
637-638: LGTM: Server interceptor configuration aligned with values.yaml
The addition of MetricInterceptor to the server interceptors maintains consistency with the values.yaml configuration.
Line range hint 73-823: LGTM: Well-structured CRD schema with consistent validations
The schema demonstrates good practices:
- Consistent port validations (0-65535)
- Well-defined network protocol enums
- Clear type definitions for all properties
247-272: 🛠️ Refactor suggestion
Enhance dataset configuration validation
The dataset configuration could benefit from additional validation rules to ensure data integrity and prevent potential issues.
Apply this diff to add comprehensive validation:
dataset:
properties:
group:
minLength: 1
type: string
+ pattern: "^[a-z0-9][a-z0-9-]*[a-z0-9]$"
+ description: "Dataset group identifier (lowercase alphanumeric with hyphens)"
range:
properties:
end:
minimum: 1
type: integer
+ maximum: 1000000
start:
minimum: 1
type: integer
+ maximum: 1000000
required:
- start
- end
+ x-kubernetes-validations:
+ - rule: "self.start <= self.end"
+ message: "start must be less than or equal to end"charts/vald-helm-operator/crds/valdhelmoperatorrelease.yaml (4)
85-90: LGTM: Image configuration is well-defined
The image configuration schema is properly defined with all necessary fields and correct type specifications. The pullPolicy enum includes all valid Kubernetes values.
212-214: Service type enums are missing ExternalName type
The CRD currently enumerates only three service types: ClusterIP, LoadBalancer, and NodePort. However, Kubernetes also supports the ExternalName service type, which is used for creating a CNAME record.
enum:
- ClusterIP
- LoadBalancer
- NodePort
+ - ExternalName
type: string214-220: LGTM: ServiceAccount configuration is properly defined
The serviceAccount schema includes the essential fields (create and name) with proper type definitions.
221-225: LGTM: Tolerations configuration follows best practices
The tolerations schema is properly defined as an array and correctly uses x-kubernetes-preserve-unknown-fields to maintain compatibility with all Kubernetes toleration specifications.
k8s/operator/helm/crds/valdhelmoperatorrelease.yaml (3)
85-90: LGTM: Image configuration is well-structured.
The image configuration follows Kubernetes best practices with proper type definitions and standard pull policy options.
212-214: Add support for ExternalName service type.
The service type enum is missing the 'ExternalName' option, which is a valid Kubernetes service type that might be needed for certain use cases.
214-220: LGTM: ServiceAccount configuration is complete.
The serviceAccount configuration includes all necessary fields with proper type definitions.
charts/vald-benchmark-operator/crds/valdbenchmarkoperatorrelease.yaml (2)
164-166: LGTM: MetricInterceptor addition enhances observability.
The addition of MetricInterceptor to the interceptors enum is a good improvement that enables better monitoring capabilities.
608-614: LGTM: Server interceptors are well-structured.
The server interceptors configuration is properly ordered with critical interceptors first (Recover, AccessLog) followed by observability interceptors (Trace, Metric).
.github/workflows/dockers-gateway-lb-image.yaml (4)
17-17: Generator path change needs consistent application
The generator path has been updated from hack/actions/gen/main.go to hack/docker/gen/main.go. While this change is correct for this file, it needs to be consistently applied across all workflow files.
29-30: LGTM: Improved tag pattern matching
The addition of v*.*.*-* pattern extends support for pre-release version tags, which is a good practice for handling development and beta releases.
33-34: LGTM: Efficient trigger configuration
Excluding test and mock files from triggering the workflow is a good practice that helps reduce unnecessary builds.
Line range hint 254-259: LGTM: Well-structured workflow
The workflow follows GitHub Actions best practices by:
- Using reusable workflows to reduce duplication
- Properly handling secrets inheritance
charts/vald-benchmark-operator/schemas/job-values.schema.json (1)
137-140: LGTM! Verify documentation for MetricInterceptor.
The addition of "MetricInterceptor" to the client-side interceptors is consistent with server-side configuration and enhances observability capabilities.
Let's verify if this change is properly documented:
✅ Verification successful
Documentation is complete and consistent with the schema change
The MetricInterceptor is properly documented in the observability configuration guide, which explains:
- Its usage in both server and client interceptors
- The complete observability setup process
- Configuration examples in values.yaml
- Integration with OpenTelemetry for metrics collection
The schema change aligns perfectly with the existing documentation and implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if MetricInterceptor is documented in relevant files
# Search for MetricInterceptor documentation in markdown files
echo "Searching for MetricInterceptor documentation..."
rg -l "MetricInterceptor" "docs/"
# Check if there are any migration guides that need updating
echo "Checking migration guides..."
rg -l "interceptor" "docs/*/migration"
Length of output: 349
Script:
#!/bin/bash
# Let's check the content of the observability documentation to verify MetricInterceptor coverage
echo "Checking MetricInterceptor documentation content..."
rg -A 5 -B 5 "MetricInterceptor" "docs/user-guides/observability-configuration.md"
# Let's also check if there are any examples or templates using MetricInterceptor
echo "Checking examples and templates..."
rg -l "MetricInterceptor" "examples/"
rg -l "MetricInterceptor" "charts/vald/templates/"
# Check values files for default configurations
echo "Checking values files..."
rg -l "MetricInterceptor" "charts/vald/values.yaml"
rg -l "MetricInterceptor" "charts/vald-benchmark-operator/values.yaml"
Length of output: 908
Script:
#!/bin/bash
# Let's check the actual content in values files to understand the default configuration
echo "Checking values.yaml content for MetricInterceptor..."
rg -A 5 -B 5 "MetricInterceptor" "charts/vald/values.yaml"
rg -A 5 -B 5 "MetricInterceptor" "charts/vald-benchmark-operator/values.yaml"
# Let's also check if the observability documentation fully explains the MetricInterceptor
echo "Checking full observability documentation..."
cat "docs/user-guides/observability-configuration.md"
Length of output: 12617
internal/file/file.go (3)
39-41: LGTM! Good abstraction of path separator.
The introduction of the sep constant improves code maintainability by centralizing the path separator definition.
563-566: LGTM! Correct path separator normalization.
The replacer correctly handles multiple consecutive separators in the correct order.
483-484:
Remove redundant path modification.
The path suffix check and modification is immediately overwritten by filepath.Dir(path) on the next line, making these operations redundant and potentially confusing.
- if fi.Mode().IsDir() && !strings.HasSuffix(path, sep) {
- path += sep
- }
kpango
deleted the
refactor/docker-actions/standardization-generation-logic
branch
Signed-off-by: kpango <kpango@vdaas.org>
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Release Notes
New Features
Agent NGT,Agent sidecar,Discoverers,Gateways,Index Manager, andHelm Operator.MetricInterceptor.Bug Fixes
GetGraphStatisticsAPI, and installation command corrections for ARM64 architecture.Dependency Updates
Documentation
Chores
.gitattributes.