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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (16)
dockers/manager/index/Dockerfile (4)
Line range hint
18-19: Consider using a specific version tag for the buildbase imageUsing the
nightlytag for the buildbase image (ghcr.io/vdaas/vald/vald-buildbase:nightly) could lead to inconsistent builds if the nightly image changes unexpectedly. To ensure reproducible builds, it's recommended to use a specific version tag.Consider updating the FROM instruction to use a specific version tag:
-FROM ghcr.io/vdaas/vald/vald-buildbase:nightly AS builder +FROM ghcr.io/vdaas/vald/vald-buildbase:v1.7.13 AS builderReplace
v1.7.13with the appropriate version that matches your project's requirements.
Line range hint
47-84: LGTM: Efficient build process with good practicesThe RUN instruction demonstrates several good practices:
- Use of various mount types for efficient caching and temporary storage.
- Installation of only necessary packages with cleanup to reduce image size.
- Chaining of commands to reduce the number of layers.
These practices contribute to faster builds and smaller image sizes.
For improved readability, consider breaking the long RUN instruction into logical groups using multi-line formatting. For example:
RUN --mount=type=bind,target=.,rw \ --mount=type=tmpfs,target=/tmp \ --mount=type=cache,target=/var/lib/apt,sharing=locked,id=${APP_NAME} \ --mount=type=cache,target=/var/cache/apt,sharing=locked,id=${APP_NAME} \ --mount=type=cache,target="${GOPATH}/pkg",id="go-build-${TARGETARCH}" \ --mount=type=cache,target="${HOME}/.cache/go-build",id="go-build-${TARGETARCH}" \ --mount=type=tmpfs,target="${GOPATH}/src" \ set -ex \ # Set up apt configuration && echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache \ && echo 'APT::Install-Recommends "false";' > /etc/apt/apt.conf.d/no-install-recommends \ # Update and install packages && apt-get clean \ && apt-get update -y \ && apt-get upgrade -y \ && apt-get install -y --no-install-recommends --fix-missing \ build-essential ca-certificates curl tzdata locales git \ # Set up locale and timezone && ldconfig \ && echo "${LANG} UTF-8" > /etc/locale.gen \ && ln -fs /usr/share/zoneinfo/${TZ} /etc/localtime \ && locale-gen ${LANGUAGE} \ && update-locale LANG=${LANGUAGE} \ && dpkg-reconfigure -f noninteractive tzdata \ # Clean up && apt-get clean \ && apt-get autoclean -y \ && apt-get autoremove -y \ # Build and install application && make GOPATH="${GOPATH}" GOROOT="${GOROOT}" GO_VERSION="${GO_VERSION}" go/install \ && make GOPATH="${GOPATH}" GOROOT="${GOROOT}" GO_VERSION="${GO_VERSION}" go/download \ && make GOARCH="${TARGETARCH}" GOOS="${TARGETOS}" REPO="${ORG}" NAME="${REPO}" cmd/${PKG}/${APP_NAME} \ && mv "cmd/${PKG}/${APP_NAME}" "/usr/bin/${APP_NAME}"This formatting makes the different stages of the build process more apparent and easier to understand.
85-88: LGTM: Secure final image setupThe final stage of the Dockerfile demonstrates good security practices:
- Using a distroless base image minimizes the attack surface.
- Running as a non-root user enhances security.
- Copying only the necessary files (binary and configuration) keeps the image lean.
For consistency with the builder stage, consider using ARGs for the distroless image tag. This allows for easier updates and maintenance. Add the following ARG at the beginning of the Dockerfile:
ARG DISTROLESS_IMAGE=gcr.io/distroless/static:nonrootThen update the FROM instruction in the final stage:
FROM ${DISTROLESS_IMAGE}This change makes it easier to update or change the base image in the future by modifying a single ARG.
88-88: LGTM: Correct ENTRYPOINT setupThe ENTRYPOINT is correctly set to run the main application (
/usr/bin/index).Consider adding a CMD instruction to provide default arguments, if applicable. This would allow users to easily override the arguments without changing the ENTRYPOINT. For example:
ENTRYPOINT ["/usr/bin/index"] CMD ["--config", "/etc/server/config.yaml"]This change would make the Dockerfile more flexible, allowing users to easily override the default configuration file location or pass other arguments when running the container.
dockers/index/job/correction/Dockerfile (5)
Line range hint
46-84: Efficient use of caching and build optimization.The RUN command effectively uses caching and temporary storage to optimize the build process. This is a good practice for reducing build times and image size. The single RUN command for package installation and cleanup is also efficient.
For improved readability, consider breaking down the long RUN command into logical sections using line continuations () and indentation. For example:
RUN --mount=type=bind,target=.,rw \ --mount=type=tmpfs,target=/tmp \ --mount=type=cache,target=/var/lib/apt,sharing=locked,id=${APP_NAME} \ --mount=type=cache,target=/var/cache/apt,sharing=locked,id=${APP_NAME} \ --mount=type=cache,target="${GOPATH}/pkg",id="go-build-${TARGETARCH}" \ --mount=type=cache,target="${HOME}/.cache/go-build",id="go-build-${TARGETARCH}" \ --mount=type=tmpfs,target="${GOPATH}/src" \ set -ex \ && echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache \ && echo 'APT::Install-Recommends "false";' > /etc/apt/apt.conf.d/no-install-recommends \ && apt-get clean \ && apt-get update -y \ && apt-get upgrade -y \ && apt-get install -y --no-install-recommends --fix-missing \ build-essential \ ca-certificates \ curl \ tzdata \ locales \ git \ && ldconfig \ && echo "${LANG} UTF-8" > /etc/locale.gen \ && ln -fs /usr/share/zoneinfo/${TZ} /etc/localtime \ && locale-gen ${LANGUAGE} \ && update-locale LANG=${LANGUAGE} \ && dpkg-reconfigure -f noninteractive tzdata \ && apt-get clean \ && apt-get autoclean -y \ && apt-get autoremove -y \ && make GOPATH="${GOPATH}" GOROOT="${GOROOT}" GO_VERSION="${GO_VERSION}" go/install \ && make GOPATH="${GOPATH}" GOROOT="${GOROOT}" GO_VERSION="${GO_VERSION}" go/download \ && make GOARCH="${TARGETARCH}" GOOS="${TARGETOS}" REPO="${ORG}" NAME="${REPO}" cmd/${PKG}/${APP_NAME} \ && mv "cmd/${PKG}/${APP_NAME}" "/usr/bin/${APP_NAME}"This formatting makes it easier to read and maintain the Dockerfile without changing its functionality.
85-87: Good security practices with room for improvement.The use of a distroless static image and running as a non-root user are excellent security practices. These choices significantly reduce the attack surface of the container.
Consider using a volume for the configuration file instead of baking it into the image. This allows for easier configuration updates without rebuilding the image. Here's an example of how you could modify the Dockerfile:
FROM gcr.io/distroless/static:nonroot LABEL maintainer="vdaas.org vald team <[email protected]>" COPY --from=builder /usr/bin/index-correction /usr/bin/index-correction VOLUME /etc/server ENV CONFIG_FILE=/etc/server/config.yaml USER nonroot:nonrootThen, you can mount a configuration file when running the container:
docker run -v /path/to/your/config.yaml:/etc/server/config.yaml your-imageThis approach provides more flexibility for configuration management.
88-88: ENTRYPOINT is correctly set, consider adding CMD for default arguments.The ENTRYPOINT is correctly set to run the main application (
index-correction). This ensures that the container will always run this application when started.To improve flexibility, consider adding a CMD instruction to provide default arguments. This allows users to easily override the arguments without changing the ENTRYPOINT. Here's an example:
ENTRYPOINT ["/usr/bin/index-correction"] CMD ["--config", "/etc/server/config.yaml"]With this change, the container will use the default configuration file if no arguments are provided, but users can still override it easily:
docker run your-image --config /path/to/custom/config.yamlThis approach maintains the current behavior by default while adding more flexibility for users.
Line range hint
1-88: Overall, well-structured Dockerfile with good practices.This Dockerfile demonstrates several best practices:
- Use of multi-stage builds to minimize final image size.
- Effective use of caching and temporary storage during the build process.
- Running the application as a non-root user in a distroless image for improved security.
- Clear labeling and organization of build steps.
To further improve this Dockerfile, consider:
Implementing a health check to ensure the application is running correctly:
HEALTHCHECK CMD ["/usr/bin/index-correction", "--health-check"] || exit 1Adding metadata labels for better image management:
LABEL org.opencontainers.image.source="https://github.com/vdaas/vald" LABEL org.opencontainers.image.description="Index correction job for Vald" LABEL org.opencontainers.image.version="1.7.13"If applicable, declare the ports that the application listens on:
EXPOSE 8080These additions would enhance the container's integration with orchestration systems and improve its overall manageability.
Clarify or Remove Unused
UPX_OPTIONSArgument
- The
UPX_OPTIONSargument is defined indockers/index/job/correction/Dockerfilebut is not used within the file.- If
UPX_OPTIONSis intended for use in build commands or scripts invoked by this Dockerfile, please add comments to explain its purpose.- If it is not used, consider removing the
ARG UPX_OPTIONS=-9line to avoid confusion.🔗 Analysis chain
Line range hint
17-17: Consider using a stable version for the base image and clarify UPX_OPTIONS usage.
The builder stage is using a nightly build of
vald-buildbase. While this ensures you have the latest changes, it may introduce instability. Consider using a stable, tagged version instead.The
UPX_OPTIONSargument is defined but not visibly used in this Dockerfile. If it's used in a later stage or in a script called by this Dockerfile, please add a comment explaining its purpose. If it's not used, consider removing it to avoid confusion.Example of using a stable version:
FROM ghcr.io/vdaas/vald/vald-buildbase:v1.7.13 AS builderTo check if
UPX_OPTIONSis used elsewhere in the project:Also applies to: 19-19
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for UPX_OPTIONS usage in the project rg "UPX_OPTIONS" --type dockerfileLength of output: 69
Script:
#!/bin/bash # Search for UPX_OPTIONS usage in the project with correct Dockerfile type rg "UPX_OPTIONS" --type DockerfileLength of output: 69
Script:
#!/bin/bash # Find all Dockerfile variations and search for UPX_OPTIONS usage fd -HI -e Dockerfile | xargs rg "UPX_OPTIONS"Length of output: 1355
dockers/tools/cli/loadtest/Dockerfile (3)
Line range hint
20-20: Consider using a specific version tag for the base image.While using the
nightlytag ensures you're always using the latest version, it can lead to inconsistent builds over time. For better reproducibility and stability, consider using a specific version tag for the base image.Replace:
FROM ghcr.io/vdaas/vald/vald-buildbase:nightly AS builderWith a specific version tag, for example:
FROM ghcr.io/vdaas/vald/vald-buildbase:v1.7.13 AS builderThis ensures consistent builds across different environments and times.
Line range hint
27-43: Consolidate ENV instructions to reduce image layers.While the current setup works, consolidating multiple ENV instructions into a single instruction can help reduce the number of layers in your image, potentially decreasing its size and build time.
Consider replacing the multiple ENV instructions with a single, multi-line ENV instruction:
ENV APP_NAME=loadtest \ DEBIAN_FRONTEND=noninteractive \ GO111MODULE=on \ GOPATH=/go \ GOROOT=/opt/go \ HOME=/root \ INITRD=No \ LANG=en_US.UTF-8 \ LANGUAGE=en_US.UTF-8 \ LC_ALL=en_US.UTF-8 \ ORG=vdaas \ PKG=tools/cli/loadtest \ REPO=vald \ TZ=Etc/UTC \ USER=root \ PATH=${GOPATH}/bin:${GOROOT}/bin:/usr/local/bin:${PATH}This change will create only one layer for all these environment variables instead of multiple layers.
Line range hint
47-86: Excellent use of build optimizations and best practices.The build process demonstrates several best practices:
- Use of
--mountoptions for caching and temporary storage.- Combining multiple commands in a single RUN instruction to reduce layers.
- Proper cleanup of apt cache and temporary files.
- Use of
set -exfor debugging and stopping on errors.These practices contribute to faster builds and smaller image sizes. Great job!
For improved readability, consider breaking the long RUN instruction into logical sections using line breaks and comments. For example:
RUN --mount=type=bind,target=.,rw \ --mount=type=tmpfs,target=/tmp \ --mount=type=cache,target=/var/lib/apt,sharing=locked,id=${APP_NAME} \ --mount=type=cache,target=/var/cache/apt,sharing=locked,id=${APP_NAME} \ --mount=type=cache,target="${GOPATH}/pkg",id="go-build-${TARGETARCH}" \ --mount=type=cache,target="${HOME}/.cache/go-build",id="go-build-${TARGETARCH}" \ --mount=type=tmpfs,target="${GOPATH}/src" \ set -ex \ # Configure APT && echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache \ && echo 'APT::Install-Recommends "false";' > /etc/apt/apt.conf.d/no-install-recommends \ # Update and install packages && apt-get clean \ && apt-get update -y \ && apt-get upgrade -y \ && apt-get install -y --no-install-recommends --fix-missing \ build-essential ca-certificates curl tzdata locales git cmake gcc g++ unzip libssl-dev libhdf5-dev libaec-dev \ # Configure locale and timezone && ldconfig \ && echo "${LANG} UTF-8" > /etc/locale.gen \ && ln -fs /usr/share/zoneinfo/${TZ} /etc/localtime \ && locale-gen ${LANGUAGE} \ && update-locale LANG=${LANGUAGE} \ && dpkg-reconfigure -f noninteractive tzdata \ # Clean up && apt-get clean \ && apt-get autoclean -y \ && apt-get autoremove -y \ # Install Go and build application && make GOPATH="${GOPATH}" GOROOT="${GOROOT}" GO_VERSION="${GO_VERSION}" go/install \ && make GOPATH="${GOPATH}" GOROOT="${GOROOT}" GO_VERSION="${GO_VERSION}" go/download \ && make hdf5/install \ && make GOARCH="${TARGETARCH}" GOOS="${TARGETOS}" REPO="${ORG}" NAME="${REPO}" cmd/${PKG}/${APP_NAME} \ && mv "cmd/${PKG}/${APP_NAME}" "/usr/bin/${APP_NAME}"This structure makes it easier to understand and maintain the build process.
dockers/dev/Dockerfile (2)
89-89: LGTM: Addition of 'file' packageThe addition of the 'file' package is a good choice for a development container. It's a useful utility for determining file types, which can be helpful in various development and debugging scenarios.
Consider adding a comment explaining the purpose of including the 'file' package, to improve documentation for future maintainers.
95-95: LGTM: Addition of 'libhdf5-dev' packageThe addition of the 'libhdf5-dev' package is appropriate for supporting HDF5-related development in this container. This change aligns well with the
make hdf5/installcommand later in the Dockerfile.Consider adding a comment explaining the purpose of including the 'libhdf5-dev' package and its relation to the project's requirements, to improve documentation for future maintainers.
pkg/gateway/mirror/handler/grpc/handler.go (2)
Line range hint
119-119: Typo in error messages: 'canceld' should be 'canceled'There are multiple instances where the word 'canceld' is misspelled in error messages. Please correct 'canceld' to 'canceled' to fix the typographical error.
Also applies to: 170-170, 221-221, 274-274, 325-325, 376-376, 427-427, 586-586, 828-828, 904-904, 1140-1140, 1216-1216, 1340-1340, 1416-1416, 1541-1541, 1618-1618, 1743-1743
Line range hint
828-872: Refactor duplicate error handling code into a common helper functionThe functions
doInsert,doUpdate,doUpsert, anddoRemovecontain similar error handling logic. Consider refactoring the common error handling code into a shared helper function to reduce code duplication and improve maintainability.Also applies to: 1140-1186, 1340-1384, 1541-1586
📜 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 (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 (3 hunks)
- dockers/dev/Dockerfile (3 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 (1 hunks)
- dockers/tools/benchmark/operator/Dockerfile (1 hunks)
- dockers/tools/cli/loadtest/Dockerfile (1 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 due to trivial changes (22)
- 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/discoverer/k8s/Dockerfile
- dockers/gateway/lb/Dockerfile
- dockers/gateway/mirror/Dockerfile
- dockers/index/job/creation/Dockerfile
- dockers/index/job/save/Dockerfile
- dockers/index/operator/Dockerfile
- dockers/operator/helm/Dockerfile
- dockers/tools/benchmark/job/Dockerfile
- dockers/tools/benchmark/operator/Dockerfile
- go.mod
- internal/observability/metrics/mem/mem.go
- versions/PROMETHEUS_STACK_VERSION
- versions/TELEPRESENCE_VERSION
- versions/actions/DOCKER_SETUP_BUILDX_ACTION
🚧 Files skipped from review as they are similar to previous changes (18)
- Makefile
- Makefile.d/docker.mk
- Makefile.d/git.mk
- Makefile.d/helm.mk
- Makefile.d/k8s.mk
- Makefile.d/proto.mk
- 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
🧰 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] 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 (20)
dockers/manager/index/Dockerfile (1)
Line range hint
22-45: LGTM: Well-structured environment setupThe ARG and ENV instructions are well-organized and provide a flexible setup for cross-platform builds. The use of ARGs for architecture, OS, and versions allows for easy customization of the build process.
dockers/index/job/readreplica/rotate/Dockerfile (2)
88-88: LGTM: ENTRYPOINT instruction is correct.The ENTRYPOINT instruction correctly executes the
readreplica-rotatebinary. Although this line is marked as changed in the diff, its content remains unchanged from the previous version. This marking is likely due to the addition of the COPY instruction above it, which may have shifted the line number.
Line range hint
1-88: Summary: Configuration file added, build process unchanged.This PR introduces a configuration file to the
readreplica-rotateapplication's Docker image. The main changes are:
- Addition of a COPY instruction to include a
sample.yamlfile as the server configuration.- No changes to the build process or runtime environment.
While the addition of a configuration file is a positive step, ensure that:
- The configuration file contains appropriate default values for production use.
- The process for customizing or overriding this configuration is well-documented.
- The sample configuration doesn't introduce any security risks or suboptimal default settings.
The build process and runtime environment remain consistent with the previous version, which helps maintain stability.
To ensure the configuration file is properly included and doesn't contain any sensitive information, you can run the following commands:
✅ Verification successful
Configuration file verified: No sensitive information found.
The
sample.yamlconfiguration file does not contain any sensitive information such as passwords, secrets, or actual key paths. All sensitive fields use placeholder values (e.g.,/path/to/key), ensuring that no security risks are introduced.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the content of the sample configuration file cat cmd/index/job/readreplica/rotate/sample.yaml # Check if the configuration file contains any potentially sensitive information grep -i "password\|secret\|key" cmd/index/job/readreplica/rotate/sample.yamlLength of output: 3028
dockers/tools/cli/loadtest/Dockerfile (3)
Line range hint
88-94: Excellent security practices in the final stage.The final stage of the Dockerfile demonstrates several security best practices:
- Using a distroless base image (
gcr.io/distroless/static:nonroot) minimizes the attack surface by including only essential components.- Copying only the necessary files (the compiled binary and a sample configuration file) from the builder stage keeps the final image lean.
- Setting the user to
nonroot:nonrootenhances security by avoiding running processes as root.These practices significantly improve the security posture of the resulting container. Great job!
96-96: Correct usage of ENTRYPOINT instruction.The ENTRYPOINT is properly set to run the main application (
/usr/bin/loadtest). Using the exec form of ENTRYPOINT (["executable", "param1", "param2"]) is the correct approach as it allows for proper signal handling and avoids issues with shell string parsing.
Line range hint
1-96: Overall, excellent Dockerfile with room for minor improvements.This Dockerfile demonstrates a strong understanding of Docker best practices, including:
- Multi-stage builds for a lean final image
- Proper use of build arguments and environment variables
- Efficient RUN instructions with proper cleanup
- Security-focused final stage with distroless base image and non-root user
The suggested improvements (specific base image version, consolidated ENV instructions, and improved readability) are minor and do not detract from the overall quality of the Dockerfile.
Great job on creating a well-optimized and secure Dockerfile for the loadtest tool!
dockers/ci/base/Dockerfile (4)
48-48: LGTM: Addition of RUSTUP_HOME environment variableThe addition of the
RUSTUP_HOMEenvironment variable is a good practice. It explicitly sets the location for Rust's toolchain manager, rustup, ensuring consistency in Rust tool locations across the build environment. This can help prevent potential issues related to Rust tool discovery and usage.
84-84: LGTM: Addition of 'file' packageThe addition of the 'file' package is a good enhancement to the CI container. This utility is useful for determining file types, which can be beneficial in various build and test scenarios, especially when dealing with different file formats or debugging issues related to file types. Its inclusion adds flexibility to the CI environment without introducing any apparent drawbacks.
Line range hint
1-130: LGTM: Well-structured and optimized DockerfileThe overall structure and content of this Dockerfile demonstrate good practices and optimizations for a CI container:
- Proper use of ARG for build-time variables
- Combining RUN commands to reduce the number of layers
- Utilization of caching mechanisms to speed up builds
- Installation of only necessary packages
- Use of multi-stage builds for optimization
The file is well-organized and seems tailored for CI purposes, incorporating various tools and dependencies required for the build and test processes. The recent changes (addition of RUSTUP_HOME, 'file' package, and 'libhdf5-dev' package) integrate well with the existing structure.
90-90: LGTM with a query: Addition of 'libhdf5-dev' packageThe addition of the 'libhdf5-dev' package to the installed packages list is noted. This change suggests that the project now requires HDF5 development libraries, which are often needed for building software that uses HDF5 data storage.
While this change is likely necessary based on updated project requirements, it would be helpful to have some clarification:
- What prompted the decision to install this package instead of removing it?
- Are there any new dependencies or features in the project that require HDF5?
- Have you considered the impact on the final image size and build time?
To assess the impact of this change, you can run the following command to check the size difference:
This will help quantify the impact on the image size.
dockers/dev/Dockerfile (1)
48-48: LGTM: Addition of CARGO_HOME environment variableThe addition of the
CARGO_HOMEenvironment variable is a good practice for Rust development. It's correctly set using the previously definedRUST_HOMEvariable, maintaining consistency in the Dockerfile.internal/net/grpc/errdetails/errdetails.go (1)
414-414: LGTM! Consider adding a test for this line.The change simplifies the string construction for stack trace entries without altering the functionality. It's a minor improvement that might slightly enhance performance by avoiding the creation of a temporary slice for
strings.Join.To ensure the robustness of this change, please consider adding a test case that covers this line. This will improve the overall code coverage and help maintain the reliability of the
DebugInfoFromInfoDetailfunction.If the script doesn't find relevant test cases, consider adding a new test that specifically covers the modified line in the
DebugInfoFromInfoDetailfunction.🧰 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 (7)
119-126: LGTM: Constants improve code readabilityThe addition of these constants enhances code readability and maintainability. They are well-named and their usage in the
getDetailmethod for URL generation is clear.
Line range hint
1-432: Overall: Well-implemented improvements to URL generation and stack trace handlingThe changes in this file significantly enhance the URL generation logic for various types of files, including Go runtime, Google's gRPC and protobuf libraries, and Vald repository files. The addition of the
ShortStringmethod forStackTraceprovides a useful concise representation.These improvements will make debugging and code exploration easier for developers working with Vald. The code is well-structured and maintainable.
To ensure the robustness of these changes, consider adding comprehensive test coverage for the new and modified code segments, as suggested in the individual comments.
🧰 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 tests
315-331: LGTM: Enhanced URL generation for Google's gRPC and protobuf librariesThis addition significantly improves URL generation for Google's gRPC and protobuf libraries. The logic handles both versioned and non-versioned file paths, ensuring accurate links to the source code on GitHub.
To ensure robustness, consider adding test cases for this URL generation logic. Here's a script to verify the current implementation:
#!/bin/bash # Verify URL generation for Google's gRPC and protobuf libraries sample_grpc_file="google.golang.org/[email protected]/server.go" sample_protobuf_file="google.golang.org/[email protected]/proto/decode.go" go run ./internal/info/info_test.go -test.run TestURLGeneration -file "${sample_grpc_file}" -file "${sample_protobuf_file}"🧰 Tools
🪛 GitHub Check: codecov/patch
[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
430-432: LGTM: Added ShortString method for concise stack trace representationThe new
ShortStringmethod provides a concise representation of the stack trace, which can be very useful for logging or quick debugging purposes. It's a valuable addition to theStackTracestruct.To ensure the method works as expected, consider adding a unit test. Here's a script to verify the current implementation:
#!/bin/bash # Verify ShortString method of StackTrace go test ./internal/info -run TestStackTraceShortString🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 430-431: internal/info/info.go#L430-L431
Added lines #L430 - L431 were not covered by tests
420-422: LGTM: Efficient initialization of valdReplacerThe lazy initialization of
valdReplacerin thepreparemethod is a good practice, ensuring it's only created when needed. This approach can improve performance, especially in cases where the replacer might not be used.To ensure correct initialization, consider adding a test case. Here's a script to verify the current implementation:
#!/bin/bash # Verify initialization of valdReplacer go run -ldflags "-X 'github.com/vdaas/vald/internal/info.GitCommit=testcommit'" ./internal/info/info_test.go -test.run TestValdReplacerInitialization🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 420-421: internal/info/info.go#L420-L421
Added lines #L420 - L421 were not covered by tests
352-357: LGTM: Streamlined URL generation for Vald repository filesThe use of
valdReplacersimplifies and standardizes URL generation for Vald repository files. The addition of line numbers to the URL enhances the ability to navigate directly to specific code locations.To ensure reliability, consider adding test coverage for this URL generation logic. Here's a script to verify the current implementation:
🧰 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
311-314: LGTM: Improved URL generation for Go runtime filesThis change enhances the URL generation for Go runtime files, allowing direct links to the relevant source code on GitHub. It's a valuable addition for debugging and code exploration.
Consider adding test coverage for this URL generation logic. Here's a script to verify the current implementation:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 311-314: internal/info/info.go#L311-L314
Added lines #L311 - L314 were not covered by testspkg/gateway/mirror/handler/grpc/handler.go (1)
26-26: Previous comment is still valid: Avoid shadowing the standardiopackageThe import of
github.com/vdaas/vald/internal/ioasioshadows the standard libraryiopackage. This can lead to conflicts and unexpected behavior, especially when using standardiofunctionalities. Please alias the customiopackage to prevent shadowing.
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