Refactor use Absolute path for Makefile #2670
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications primarily to various Makefiles, focusing on updating path references to utilize the Changes
Possibly related PRs
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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
kpango
requested review from
a team,
kmrmt and
hlts2
and removed request for
a team
|
[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/refactor%2Fmakefile%2Fadd-absolute-path/c08cf5719843e0e2620ef48942c1fb95bc2d0559 |
Profile Report
|
kpango
force-pushed
the
refactor/makefile/add-absolute-path
branch
from
ae7e07f
to
2481a88
Compare
Deploying vald with
|
| Latest commit: |
78ec9c8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dd73f850.vald.pages.dev |
| Branch Preview URL: | https://refactor-makefile-add-absolu.vald.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
internal/net/grpc/errdetails/errdetails.go (1)
414-414: LGTM! Consider usingfmt.Sprintffor better readability.The change simplifies the string concatenation for stack entries, which is good. The use of
stack.ShortString()instead ofstack.String()is also an improvement, likely reducing the verbosity of the stack trace.For even better readability and performance, consider using
fmt.Sprintf:debug.StackEntries = append(debug.GetStackEntries(), fmt.Sprintf("id: %d stack_trace: %s", i, stack.ShortString()))This approach is more idiomatic Go and can be slightly more efficient for string formatting.
internal/info/info.go (1)
303-306: Consider usingpath.Basefor extracting the function name.Replacing the manual string manipulation with
path.Basefrom thepathpackage can improve code clarity and handle edge cases more robustly.Apply this diff to enhance the function name extraction:
-index := strings.LastIndex(funcName, "/") -if index != -1 { - funcName = funcName[index+1:] -} +funcName = path.Base(funcName)Remember to import the
pathpackage at the top of the file:+ "path"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- Makefile (2 hunks)
- Makefile.d/docker.mk (2 hunks)
- Makefile.d/functions.mk (0 hunks)
- Makefile.d/git.mk (1 hunks)
- Makefile.d/helm.mk (2 hunks)
- Makefile.d/k8s.mk (9 hunks)
- Makefile.d/proto.mk (1 hunks)
- internal/info/info.go (5 hunks)
- internal/net/grpc/errdetails/errdetails.go (1 hunks)
💤 Files with no reviewable changes (1)
- Makefile.d/functions.mk
🚧 Files skipped from review as they are similar to previous changes (5)
- Makefile
- Makefile.d/docker.mk
- Makefile.d/git.mk
- Makefile.d/k8s.mk
- Makefile.d/proto.mk
🔇 Additional comments (22)
Makefile.d/helm.mk (15)
40-40: LGTM: Improved path robustnessThe use of
$(ROOTDIR)in the helm package command ensures that the correct path is used regardless of the current working directory. This change improves the robustness of the Makefile.
47-47: LGTM: Consistent path updateThe use of
$(ROOTDIR)in the helm package command for vald-helm-operator is consistent with the previous change and provides the same benefits of improved path robustness.
55-55: LGTM: Consistent path updateThe use of
$(ROOTDIR)in the helm package command for vald-benchmark-operator maintains consistency with the previous changes and improves path robustness.
59-59: LGTM: Consistent path updateThe use of
$(ROOTDIR)in the helm package command for vald-readreplica maintains consistency with the previous changes and improves path robustness.
67-67: LGTM: Consistent path updates for documentationThe use of
$(ROOTDIR)in the paths for the Vald README and its dependencies ensures consistency with previous changes and improves the robustness of the documentation generation process.Also applies to: 70-73
77-77: LGTM: Consistent path updates for vald-helm-operator documentationThe use of
$(ROOTDIR)in the paths for the vald-helm-operator README and its dependencies maintains consistency with previous changes and ensures robust documentation generation.Also applies to: 80-83
87-87: LGTM: Consistent path update for vald-readreplica documentationThe use of
$(ROOTDIR)in the path for the vald-readreplica README maintains consistency with previous changes and ensures robust documentation generation.
90-90: LGTM: Consistent path updates for vald-benchmark-operator documentationThe use of
$(ROOTDIR)in the paths for the vald-benchmark-operator README and its dependencies maintains consistency with previous changes and ensures robust documentation generation.Also applies to: 92-95
99-102: LGTM: Completed path updates for vald-readreplica documentationThe use of
$(ROOTDIR)in the paths for the vald-readreplica README and its dependencies completes the modifications for this target, maintaining consistency and ensuring robust documentation generation.
115-115: LGTM: Consistent path updates for Vald schema generationThe use of
$(ROOTDIR)in the paths for the Vald schema and its dependency maintains consistency with previous changes and ensures robust schema generation.Also applies to: 117-118
123-123: LGTM: Consistent path updates for vald-helm-operator schema generationThe use of
$(ROOTDIR)in the paths for the vald-helm-operator schema and its dependency maintains consistency with previous changes and ensures robust schema generation.Also applies to: 125-126
131-131: LGTM: Consistent path updates for vald-benchmark-job schema generationThe use of
$(ROOTDIR)in the paths for the vald-benchmark-job schema and its dependency maintains consistency with previous changes and ensures robust schema generation.Also applies to: 133-134
139-139: LGTM: Consistent path updates for vald-benchmark-scenario schema generationThe use of
$(ROOTDIR)in the paths for the vald-benchmark-scenario schema and its dependency maintains consistency with previous changes and ensures robust schema generation.Also applies to: 141-142
147-147: LGTM: Consistent path updates for vald-benchmark-operator schema generationThe use of
$(ROOTDIR)in the paths for the vald-benchmark-operator schema and its dependency maintains consistency with previous changes and ensures robust schema generation.Also applies to: 149-150
Line range hint
40-150: LGTM: Comprehensive path refactoring improves Makefile robustnessThis PR consistently updates all relevant paths in the Makefile to use
$(ROOTDIR). This change affects various targets including Helm chart packaging, documentation generation, and schema generation for multiple components (Vald, vald-helm-operator, vald-benchmark-operator, vald-readreplica, etc.).The refactoring improves the robustness of the Makefile by ensuring that correct paths are used regardless of the current working directory. This consistency across all targets enhances maintainability and reduces the risk of path-related errors.
Overall, these changes represent a positive improvement to the build system.
internal/info/info.go (7)
40-43: Addition of 'valdReplacer' field enhances URL handling in stack traces.The introduction of the
valdReplacer *strings.Replacerfield to theinfostruct allows for efficient and consistent URL replacements when generating stack traces. This enhances the functionality of theinfopackage by improving the accuracy of stack trace URLs.
119-126: Definition of new constants improves code readability and maintainability.Introducing constants like
goSrc,goMod, andgoogleGolangsimplifies path handling and makes the code more readable. This practice reduces magic strings and potential errors from mistyped strings.
311-331: Enhanced URL construction logic improves stack trace accuracy.The updated logic for generating URLs in the
getDetailmethod correctly handles various cases, such as Go source files and Google libraries. This refinement ensures that stack trace URLs point to the exact locations in the respective repositories, aiding in debugging and navigation.
352-352: Appropriate use ofvaldReplacerfor URL replacement.Using
i.valdReplacer.Replaceeffectively replaces the relevant parts of the file path to construct accurate URLs for stack traces related to the Vald repository.
354-354: Consistent URL replacement withvaldReplacer.Continuing to utilize
i.valdReplacer.Replaceensures consistent handling of file paths when generating URLs, which enhances maintainability.
420-422: Lazy initialization ofvaldReplacerprevents nil pointer dereference.By initializing
valdReplacerwithin thepreparemethod only if it isnil, the code ensures thatvaldReplaceris ready when needed without redundant initializations.
430-432: Addition ofShortStringmethod provides concise stack trace representation.The
ShortStringmethod offers a succinct way to represent stack trace information, which can be useful for logging purposes where full details are unnecessary.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2670 +/- ##
==========================================
- Coverage 23.99% 23.98% -0.01%
==========================================
Files 539 539
Lines 47179 47195 +16
==========================================
+ Hits 11319 11321 +2
- Misses 35090 35105 +15
+ Partials 770 769 -1 ☔ View full report in Codecov by Sentry. |
kpango
force-pushed
the
refactor/makefile/add-absolute-path
branch
from
4d33376
to
db42790
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
internal/info/info.go (2)
315-323: LGTM: Improved URL generation for gRPC filesThe new logic for generating URLs for gRPC files is a good addition. It correctly handles the transformation from the Go package path to the GitHub repository URL, including version information when available.
Consider adding a brief comment explaining this logic, as it's not immediately obvious why this transformation is necessary. For example:
+// Transform google.golang.org/grpc package paths to GitHub URLs case strings.HasPrefix(file, googleGolang+"/grpc"): url = "https://github.com/grpc/grpc-go/blob/" _, versionSource, ok := strings.Cut(file, "@") if ok && versionSource != "" { url += versionSource } else { url = strings.ReplaceAll(file, googleGolang+"/grpc@", url) }
324-331: LGTM: Improved URL generation for protobuf filesThe new logic for generating URLs for protobuf files is a good addition, similar to the gRPC URL generation. It correctly handles the transformation from the Go package path to the GitHub repository URL, including version information when available.
Consider refactoring the gRPC and protobuf URL generation logic into a separate function to reduce code duplication. For example:
+func generateGoogleURL(file, repoURL string) string { + url := repoURL + _, versionSource, ok := strings.Cut(file, "@") + if ok && versionSource != "" { + return url + versionSource + } + return strings.ReplaceAll(file, googleGolang+"/"+strings.TrimPrefix(repoURL, "https://github.com/")+"@", url) +} case strings.HasPrefix(file, googleGolang+"/grpc"): - url = "https://github.com/grpc/grpc-go/blob/" - _, versionSource, ok := strings.Cut(file, "@") - if ok && versionSource != "" { - url += versionSource - } else { - url = strings.ReplaceAll(file, googleGolang+"/grpc@", url) - } + url = generateGoogleURL(file, "https://github.com/grpc/grpc-go/blob/") case strings.HasPrefix(file, googleGolang+"/protobuf"): - url = "https://github.com/protocolbuffers/protobuf-go/blob/" - _, versionSource, ok := strings.Cut(file, "@") - if ok && versionSource != "" { - url += versionSource - } else { - url = strings.ReplaceAll(file, googleGolang+"/protobuf@", url) - } + url = generateGoogleURL(file, "https://github.com/protocolbuffers/protobuf-go/blob/")This refactoring would make the code more maintainable and easier to extend for other Google packages in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumhack/actions/gen/main.gois excluded by!**/gen/**hack/docker/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
- Makefile (2 hunks)
- Makefile.d/docker.mk (2 hunks)
- Makefile.d/functions.mk (0 hunks)
- Makefile.d/git.mk (1 hunks)
- Makefile.d/helm.mk (2 hunks)
- Makefile.d/k8s.mk (9 hunks)
- Makefile.d/proto.mk (1 hunks)
- dockers/agent/core/agent/Dockerfile (3 hunks)
- dockers/agent/core/faiss/Dockerfile (2 hunks)
- dockers/agent/core/ngt/Dockerfile (2 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 (2 hunks)
- dockers/dev/Dockerfile (2 hunks)
- dockers/discoverer/k8s/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/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 (2 hunks)
- dockers/tools/benchmark/operator/Dockerfile (1 hunks)
- dockers/tools/cli/loadtest/Dockerfile (2 hunks)
- go.mod (4 hunks)
- hack/cspell/main.go (1 hunks)
- hack/tools/deadlink/main.go (1 hunks)
- internal/core/algorithm/faiss/faiss.go (1 hunks)
- internal/core/algorithm/faiss/option.go (1 hunks)
- internal/core/algorithm/usearch/option.go (1 hunks)
- internal/core/algorithm/usearch/usearch.go (1 hunks)
- internal/info/info.go (5 hunks)
- internal/k8s/job/job.go (1 hunks)
- internal/net/grpc/errdetails/errdetails.go (1 hunks)
- internal/observability/metrics/mem/mem.go (1 hunks)
- pkg/gateway/lb/handler/grpc/handler.go (1 hunks)
- pkg/gateway/mirror/handler/grpc/handler.go (1 hunks)
- pkg/gateway/mirror/service/gateway.go (1 hunks)
- pkg/index/job/correction/service/corrector.go (1 hunks)
- pkg/index/job/readreplica/rotate/service/rotator.go (1 hunks)
- tests/performance/max_vector_dim_test.go (1 hunks)
- versions/PROMETHEUS_STACK_VERSION (1 hunks)
- versions/TELEPRESENCE_VERSION (1 hunks)
- versions/actions/DOCKER_SETUP_BUILDX_ACTION (1 hunks)
💤 Files with no reviewable changes (1)
- Makefile.d/functions.mk
🚧 Files skipped from review as they are similar to previous changes (48)
- Makefile
- Makefile.d/docker.mk
- Makefile.d/git.mk
- Makefile.d/helm.mk
- Makefile.d/k8s.mk
- Makefile.d/proto.mk
- dockers/agent/core/agent/Dockerfile
- dockers/agent/core/faiss/Dockerfile
- dockers/agent/core/ngt/Dockerfile
- dockers/agent/sidecar/Dockerfile
- dockers/binfmt/Dockerfile
- dockers/buildbase/Dockerfile
- dockers/buildkit/Dockerfile
- dockers/buildkit/syft/scanner/Dockerfile
- dockers/ci/base/Dockerfile
- dockers/dev/Dockerfile
- dockers/discoverer/k8s/Dockerfile
- dockers/gateway/filter/Dockerfile
- dockers/gateway/lb/Dockerfile
- dockers/gateway/mirror/Dockerfile
- dockers/index/job/correction/Dockerfile
- dockers/index/job/creation/Dockerfile
- dockers/index/job/readreplica/rotate/Dockerfile
- dockers/index/job/save/Dockerfile
- dockers/index/operator/Dockerfile
- dockers/manager/index/Dockerfile
- dockers/operator/helm/Dockerfile
- dockers/tools/benchmark/job/Dockerfile
- dockers/tools/benchmark/operator/Dockerfile
- dockers/tools/cli/loadtest/Dockerfile
- go.mod
- hack/cspell/main.go
- hack/tools/deadlink/main.go
- internal/core/algorithm/faiss/faiss.go
- internal/core/algorithm/faiss/option.go
- internal/core/algorithm/usearch/option.go
- internal/core/algorithm/usearch/usearch.go
- internal/k8s/job/job.go
- internal/net/grpc/errdetails/errdetails.go
- internal/observability/metrics/mem/mem.go
- pkg/gateway/lb/handler/grpc/handler.go
- pkg/gateway/mirror/service/gateway.go
- pkg/index/job/correction/service/corrector.go
- pkg/index/job/readreplica/rotate/service/rotator.go
- tests/performance/max_vector_dim_test.go
- versions/PROMETHEUS_STACK_VERSION
- versions/TELEPRESENCE_VERSION
- versions/actions/DOCKER_SETUP_BUILDX_ACTION
🔇 Additional comments (6)
internal/info/info.go (5)
119-126: LGTM: New constants improve code clarityThe addition of these constants for Go source paths and CGO settings enhances code readability and maintainability. They provide clear, descriptive names for commonly used string values, which can help prevent typos and make the code more self-documenting.
303-306: LGTM: Improved function name extractionThe updated logic for extracting the function name is more robust, using
strings.LastIndexto find the last occurrence of "/". This change correctly handles fully qualified function names, extracting only the short name.As noted in a previous review, consider adding test coverage for this function name extraction logic to ensure its correctness across various scenarios.
311-313: LGTM: Improved URL generation for Go runtime filesThe updated logic for generating URLs for Go runtime files is more accurate. It now correctly points to the specific version of the Go source code on GitHub, which will greatly improve the usefulness of the stack trace information for debugging.
352-352: LGTM: Improved URL generation for Vald repository filesThe use of
valdReplacerfor generating URLs for Vald repository files is a good improvement. It simplifies the code and makes it more consistent. ThevaldReplaceris properly initialized in thepreparemethod, ensuring it's always available when needed.Also applies to: 354-354
430-432: LGTM: NewShortStringmethod adds useful functionalityThe addition of the
ShortStringmethod to theStackTracestruct is a good improvement. It provides a concise representation of the stack trace, including only the URL and function name. This can be very useful for quick debugging or logging purposes where a full stack trace might be too verbose.pkg/gateway/mirror/handler/grpc/handler.go (1)
26-26: Avoid shadowing the standard libraryiopackageThe import statement on line 26 imports
github.com/vdaas/vald/internal/ioasio, which shadows the standard libraryiopackage. This can lead to conflicts and unexpected behavior, especially if standardiofunctionalities (e.g.,io.EOF) are used elsewhere in the code. To prevent this, consider aliasing the customiopackage.
kpango
force-pushed
the
refactor/makefile/add-absolute-path
branch
from
db42790
to
d66748b
Compare
Signed-off-by: kpango <[email protected]>
kpango
force-pushed
the
refactor/makefile/add-absolute-path
branch
from
d66748b
to
78ec9c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
internal/info/info.go (1)
Line range hint
1-432: Overall improvements with a need for additional test coverageThe changes made to
internal/info/info.gosignificantly improve the URL generation for stack traces, covering various scenarios including Go runtime files, Google's gRPC and protobuf libraries, and Vald repository files. The addition of theShortStringmethod forStackTracealso enhances the usability of the package.However, there's a consistent lack of test coverage for the new code segments. To ensure the reliability and maintainability of these improvements, it's strongly recommended to add comprehensive unit tests covering:
- URL generation for different file types and scenarios
- Initialization and usage of
valdReplacer- The new
ShortStringmethodAdding these tests will help prevent regressions and ensure the correct functioning of the package as it evolves.
Consider creating a separate test file (e.g.,
internal/info/info_test.go) with test cases for each new functionality. This will not only improve code coverage but also serve as documentation for how these functions are expected to behave in different scenarios.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 303-305: internal/info/info.go#L303-L305
Added lines #L303 - L305 were not covered by tests
[warning] 311-314: internal/info/info.go#L311-L314
Added lines #L311 - L314 were not covered by tests
[warning] 316-321: internal/info/info.go#L316-L321
Added lines #L316 - L321 were not covered by tests
[warning] 323-323: internal/info/info.go#L323
Added line #L323 was not covered by tests
[warning] 325-330: internal/info/info.go#L325-L330
Added lines #L325 - L330 were not covered by testspkg/gateway/mirror/handler/grpc/handler.go (1)
Line range hint
57-57: Correct the typo in error messages: 'canceld' should be 'canceled'There are multiple instances in the code where
'canceld'is used instead of'canceled'in error messages. This typo may lead to confusion during debugging and error handling. Please correct the spelling to ensure clarity.Apply this diff to fix the typos:
@@ -57,7 +57,7 @@ func (s *server) Register( switch { case errors.Is(err, context.Canceled): err = status.WrapWithCanceled( - mirror.RegisterRPCName+" API canceld", err, reqInfo, resInfo, + mirror.RegisterRPCName+" API canceled", err, reqInfo, resInfo, ) attrs = trace.StatusCodeCancelled(err.Error())@@ -125,7 +125,7 @@ func (s *server) Exists( switch { case errors.Is(err, context.Canceled): err = status.WrapWithCanceled( - vald.ExistsRPCName+" API canceld", err, reqInfo, resInfo, + vald.ExistsRPCName+" API canceled", err, reqInfo, resInfo, ) attrs = trace.StatusCodeCancelled(err.Error())@@ -271,7 +271,7 @@ func (s *server) Search( switch { case errors.Is(err, context.Canceled): err = status.WrapWithCanceled( - vald.SearchRPCName+" API canceld", err, reqInfo, resInfo, + vald.SearchRPCName+" API canceled", err, reqInfo, resInfo, ) attrs = trace.StatusCodeCancelled(err.Error())... (similar diffs for other occurrences)
Also applies to: 125-125, 271-271, 359-359, 452-452, 544-544, 636-636, 728-728, 820-820, 912-912, 1004-1004, 1096-1096, 1188-1188
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumhack/actions/gen/main.gois excluded by!**/gen/**hack/docker/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
- Makefile (2 hunks)
- Makefile.d/docker.mk (2 hunks)
- Makefile.d/functions.mk (1 hunks)
- Makefile.d/git.mk (1 hunks)
- Makefile.d/helm.mk (2 hunks)
- Makefile.d/k8s.mk (9 hunks)
- Makefile.d/proto.mk (1 hunks)
- dockers/agent/core/agent/Dockerfile (3 hunks)
- dockers/agent/core/faiss/Dockerfile (2 hunks)
- dockers/agent/core/ngt/Dockerfile (2 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 (2 hunks)
- dockers/dev/Dockerfile (2 hunks)
- dockers/discoverer/k8s/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/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 (2 hunks)
- dockers/tools/benchmark/operator/Dockerfile (1 hunks)
- dockers/tools/cli/loadtest/Dockerfile (2 hunks)
- go.mod (4 hunks)
- hack/cspell/main.go (1 hunks)
- hack/tools/deadlink/main.go (1 hunks)
- internal/core/algorithm/faiss/faiss.go (1 hunks)
- internal/core/algorithm/faiss/option.go (1 hunks)
- internal/core/algorithm/usearch/option.go (1 hunks)
- internal/core/algorithm/usearch/usearch.go (1 hunks)
- internal/info/info.go (6 hunks)
- internal/k8s/job/job.go (1 hunks)
- internal/net/grpc/errdetails/errdetails.go (1 hunks)
- internal/observability/metrics/mem/mem.go (1 hunks)
- pkg/gateway/lb/handler/grpc/handler.go (1 hunks)
- pkg/gateway/mirror/handler/grpc/handler.go (1 hunks)
- pkg/gateway/mirror/service/gateway.go (1 hunks)
- pkg/index/job/correction/service/corrector.go (1 hunks)
- pkg/index/job/readreplica/rotate/service/rotator.go (1 hunks)
- tests/performance/max_vector_dim_test.go (1 hunks)
- versions/PROMETHEUS_STACK_VERSION (1 hunks)
- versions/TELEPRESENCE_VERSION (1 hunks)
- versions/actions/DOCKER_SETUP_BUILDX_ACTION (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/observability/metrics/mem/mem.go
🚧 Files skipped from review as they are similar to previous changes (46)
- Makefile
- Makefile.d/docker.mk
- Makefile.d/git.mk
- Makefile.d/helm.mk
- Makefile.d/k8s.mk
- Makefile.d/proto.mk
- dockers/agent/core/agent/Dockerfile
- dockers/agent/core/faiss/Dockerfile
- dockers/agent/core/ngt/Dockerfile
- dockers/agent/sidecar/Dockerfile
- dockers/binfmt/Dockerfile
- dockers/buildbase/Dockerfile
- dockers/buildkit/Dockerfile
- dockers/buildkit/syft/scanner/Dockerfile
- dockers/ci/base/Dockerfile
- dockers/dev/Dockerfile
- dockers/discoverer/k8s/Dockerfile
- dockers/gateway/filter/Dockerfile
- dockers/gateway/lb/Dockerfile
- dockers/gateway/mirror/Dockerfile
- dockers/index/job/correction/Dockerfile
- dockers/index/job/creation/Dockerfile
- dockers/index/job/readreplica/rotate/Dockerfile
- dockers/index/job/save/Dockerfile
- dockers/index/operator/Dockerfile
- dockers/manager/index/Dockerfile
- dockers/operator/helm/Dockerfile
- dockers/tools/benchmark/job/Dockerfile
- dockers/tools/benchmark/operator/Dockerfile
- dockers/tools/cli/loadtest/Dockerfile
- go.mod
- hack/cspell/main.go
- hack/tools/deadlink/main.go
- internal/core/algorithm/faiss/faiss.go
- internal/core/algorithm/faiss/option.go
- internal/core/algorithm/usearch/option.go
- internal/core/algorithm/usearch/usearch.go
- internal/k8s/job/job.go
- pkg/gateway/lb/handler/grpc/handler.go
- pkg/gateway/mirror/service/gateway.go
- pkg/index/job/correction/service/corrector.go
- pkg/index/job/readreplica/rotate/service/rotator.go
- tests/performance/max_vector_dim_test.go
- versions/PROMETHEUS_STACK_VERSION
- versions/TELEPRESENCE_VERSION
- versions/actions/DOCKER_SETUP_BUILDX_ACTION
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/info/info.go
[warning] 303-305: internal/info/info.go#L303-L305
Added lines #L303 - L305 were not covered by tests
[warning] 311-314: internal/info/info.go#L311-L314
Added lines #L311 - L314 were not covered by tests
[warning] 316-321: internal/info/info.go#L316-L321
Added lines #L316 - L321 were not covered by tests
[warning] 323-323: internal/info/info.go#L323
Added line #L323 was not covered by tests
[warning] 325-330: internal/info/info.go#L325-L330
Added lines #L325 - L330 were not covered by tests
[warning] 352-352: internal/info/info.go#L352
Added line #L352 was not covered by tests
[warning] 354-354: internal/info/info.go#L354
Added line #L354 was not covered by tests
[warning] 356-356: internal/info/info.go#L356
Added line #L356 was not covered by tests
[warning] 394-394: internal/info/info.go#L394
Added line #L394 was not covered by tests
[warning] 420-421: internal/info/info.go#L420-L421
Added lines #L420 - L421 were not covered by tests
[warning] 430-431: internal/info/info.go#L430-L431
Added lines #L430 - L431 were not covered by testsinternal/net/grpc/errdetails/errdetails.go
[warning] 414-414: internal/net/grpc/errdetails/errdetails.go#L414
Added line #L414 was not covered by tests
🔇 Additional comments (8)
internal/net/grpc/errdetails/errdetails.go (1)
414-414: Approved with minor suggestionsThe change from
strings.Jointo direct string concatenation is functionally equivalent and slightly improves readability. However, there are two points to consider:
Performance: While the impact is likely negligible in this case, direct string concatenation can be less efficient than
strings.Joinfor multiple strings. If this function is called frequently with large stack traces, it might be worth benchmarking both approaches.Test coverage: The static analysis indicates that this line is not covered by tests.
To address the test coverage issue, please add a unit test that exercises this line. Here's a script to verify the current test coverage:
If the coverage is indeed lacking, consider adding a test case that exercises the
DebugInfoFromInfoDetailfunction with a non-emptyStackTrace.✅ Verification successful
Change approved
The change from
strings.Jointo direct string concatenation is functionally equivalent, improves readability, and is covered by existing tests for theDebugInfoFromInfoDetailfunction.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for the DebugInfoFromInfoDetail function # Run tests with coverage go test -coverprofile=coverage.out ./internal/net/grpc/errdetails # Check coverage for the specific function go tool cover -func=coverage.out | grep DebugInfoFromInfoDetail # Clean up rm coverage.outLength of output: 351
Script:
#!/bin/bash # Description: Search for test cases for the DebugInfoFromInfoDetail function # Search for test functions related to DebugInfoFromInfoDetail in test files rg 'func TestDebugInfoFromInfoDetail' ./internal/net/grpc/errdetails/*_test.goLength of output: 138
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 414-414: internal/net/grpc/errdetails/errdetails.go#L414
Added line #L414 was not covered by testsinternal/info/info.go (6)
119-126: LGTM: New constants improve code clarityThe addition of these constants enhances code readability and maintainability. They are well-named and will be useful in the URL generation logic.
303-306: Function name extraction logic looks good, but needs test coverageThe logic for extracting the function name is correct and improves the readability of stack traces. However, as noted in a previous comment, this segment lacks test coverage.
Consider adding unit tests to ensure this functionality works as intended and to prevent future regressions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 303-305: internal/info/info.go#L303-L305
Added lines #L303 - L305 were not covered by tests
352-357: Improved URL generation for Vald repository filesThe use of
valdReplacerfor generating URLs for Vald repository files is a good improvement. It ensures that the correct GitHub URLs are generated for both files ingo/srcand directly in the Vald repository.However, this new logic is not covered by tests. Consider adding unit tests to verify the correct URL generation for different scenarios (e.g., files in
go/src, files directly in the Vald repository, different file paths).To verify the correctness of the URL generation for Vald repository files, you can run the following script:
#!/bin/bash # Description: Verify URL generation for Vald repository files # Test: Check if the URL is correctly generated for Vald repository files vald_repo="github.com/vdaas/vald" git_commit="main" # Replace with actual git commit if different go_src_file="go/src/${vald_repo}/internal/info/info.go" direct_file="${vald_repo}/internal/info/info.go" expected_url="https://${vald_repo}/blob/${git_commit}/internal/info/info.go" echo "Go src file: ${go_src_file}" echo "Direct file: ${direct_file}" echo "Expected URL: ${expected_url}" echo "Checking if the generated URLs match the expected URL for both file paths..." # Note: This script can't actually run the Go code, so we're just demonstrating the verification logic🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 352-352: internal/info/info.go#L352
Added line #L352 was not covered by tests
[warning] 354-354: internal/info/info.go#L354
Added line #L354 was not covered by tests
[warning] 356-356: internal/info/info.go#L356
Added line #L356 was not covered by tests
420-422: Proper initialization ofvaldReplacerThe lazy initialization of
valdReplacerin thepreparemethod is a good practice. It ensures that the replacer is properly set up before being used in the URL generation logic.However, this new initialization logic is not covered by tests. Consider adding unit tests to verify that
valdReplaceris correctly initialized with the expected replacement rules.To verify the correct initialization of
valdReplacer, you can run the following script:#!/bin/bash # Description: Verify initialization of valdReplacer # Test: Check if valdReplacer is correctly initialized vald_repo="github.com/vdaas/vald" git_commit="main" # Replace with actual git commit if different expected_replacement="https://${vald_repo}/blob/${git_commit}" echo "Vald repo: ${vald_repo}" echo "Git commit: ${git_commit}" echo "Expected replacement: ${expected_replacement}" echo "Checking if valdReplacer is initialized with the correct replacement rule..." # Note: This script can't actually run the Go code, so we're just demonstrating the verification logic🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 420-421: internal/info/info.go#L420-L421
Added lines #L420 - L421 were not covered by tests
430-432: NewShortStringmethod forStackTraceThe addition of the
ShortStringmethod to theStackTracestruct is a good improvement. It provides a concise representation of the stack trace, which can be useful for logging or quick debugging.However, this new method is not covered by tests. Consider adding unit tests to verify that
ShortStringreturns the expected output for various input scenarios.To verify the correctness of the
ShortStringmethod, you can run the following script:#!/bin/bash # Description: Verify ShortString method of StackTrace # Test: Check if ShortString returns the expected output url="https://github.com/vdaas/vald/blob/main/internal/info/info.go" func_name="(*info).getDetail" expected_output="${url} ${func_name}" echo "URL: ${url}" echo "Function name: ${func_name}" echo "Expected output: ${expected_output}" echo "Checking if ShortString returns the expected output..." # Note: This script can't actually run the Go code, so we're just demonstrating the verification logic🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 430-431: internal/info/info.go#L430-L431
Added lines #L430 - L431 were not covered by tests
311-314: Improved URL generation for Go runtime filesThe special handling for Go runtime files is a good addition. It ensures that the correct GitHub URL is generated based on the Go version being used. This will make it easier to navigate to the relevant source code when debugging.
However, this new logic is not covered by tests. Consider adding unit tests to verify the correct URL generation for different scenarios (e.g., different Go versions, various file paths).
To verify the correctness of the URL generation, you can run the following script:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 311-314: internal/info/info.go#L311-L314
Added lines #L311 - L314 were not covered by testsMakefile.d/functions.mk (1)
77-77: Correctly setting CGOEnabled in build flags.The
CGOEnabledflag is now properly set based on theCGO_ENABLEDenvironment variable. This ensures that build metadata accurately reflects whether CGO was enabled during the build process.
Signed-off-by: kpango <[email protected]>
…Makefile (#2673) * Refactor use Absolute path for Makefile (#2670) Signed-off-by: kpango <[email protected]> * Apply suggestions from code review Signed-off-by: Kiichiro YUKAWA <[email protected]> --------- Signed-off-by: kpango <[email protected]> Signed-off-by: Kiichiro YUKAWA <[email protected]> Co-authored-by: Yusuke Kato <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit