Backport PR #2705 to release/v1.7 for add example-client docker image #2709
Conversation
* 🐳 add example-client docker image Signed-off-by: vankichi <[email protected]> * 🐛 fix build error Signed-off-by: vankichi <[email protected]> * ♻️ fix Signed-off-by: vankichi <[email protected]> * ♻️ fix Signed-off-by: vankichi <[email protected]> * ♻️ fix Signed-off-by: vankichi <[email protected]> --------- Signed-off-by: vankichi <[email protected]>
Deploying vald with
|
| Latest commit: |
e7311df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8b8a991a.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-featur-cewm.vald.pages.dev |
📝 WalkthroughWalkthroughThis pull request introduces several changes aimed at enhancing the build and deployment processes for a client application. A new GitHub Actions workflow file, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| FROM ghcr.io/vdaas/vald/vald-buildbase:nightly AS builder | ||
| LABEL maintainer="vdaas.org vald team <[email protected]>" | ||
| # skipcq: DOK-DL3002 | ||
| USER root:root |
| WORKDIR ${GOPATH}/src/github.com/${ORG}/${REPO} | ||
| SHELL ["/bin/bash", "-o", "pipefail", "-c"] | ||
| #skipcq: DOK-W1001, DOK-SC2046, DOK-SC2086, DOK-DL3008 | ||
| RUN --mount=type=bind,target=.,rw \ |
|
[CHATOPS:HELP] ChatOps commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
.github/workflows/dockers-example-client-image.yaml (1)
19-70: LGTM: Well-defined workflow triggers with a suggestion.The workflow name is clear, and the trigger events are comprehensive, covering necessary scenarios for rebuilding the example client image. The use of path filters for pull requests is an excellent practice to avoid unnecessary builds.
Consider adding a brief comment explaining the purpose of including both
pull_requestandpull_request_targetevents, as this might not be immediately clear to all contributors.Makefile.d/docker.mk (1)
389-399: LGTM: New targets for example-client added correctlyThe new targets
docker/name/example-clientanddocker/build/example-clientare correctly implemented and consistent with the PR objective. They follow the pattern of other similar targets in the file.One minor suggestion:
Consider adding a comment above the
docker/build/example-clienttarget to briefly explain why the ZLIB_VERSION and HDF5_VERSION build arguments are necessary for the example client. This would improve the maintainability of the Makefile..PHONY: docker/build/example-client -## build example client docker image +## Build example client docker image +## Note: ZLIB_VERSION and HDF5_VERSION are required for [brief explanation] docker/build/example-client: @make DOCKERFILE="$(ROOTDIR)/dockers/example/client/Dockerfile" \ IMAGE=$(EXAMPLE_CLIENT_IMAGE) \ DOCKER_OPTS="--build-arg ZLIB_VERSION=$(ZLIB_VERSION) --build-arg HDF5_VERSION=$(HDF5_VERSION)" \ docker/build/imagedockers/example/client/Dockerfile (1)
18-18: Note: This Dockerfile is auto-generated; modify the generator for changes.As mentioned in the comment, this Dockerfile is generated by
hack/docker/gen/main.go. To make sustainable changes, please update the generator script instead of editing the Dockerfile directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
hack/docker/gen/main.gois excluded by!**/gen/**
📒 Files selected for processing (7)
- .github/workflows/dockers-example-client-image.yaml (1 hunks)
- Makefile (2 hunks)
- Makefile.d/build.mk (4 hunks)
- Makefile.d/docker.mk (3 hunks)
- Makefile.d/functions.mk (1 hunks)
- dockers/example/client/Dockerfile (1 hunks)
- example/helm/values.yaml (1 hunks)
🧰 Additional context used
🔇 Additional comments (15)
.github/workflows/dockers-example-client-image.yaml (2)
1-17: LGTM: Proper license and auto-generation notice.The file includes the correct Apache 2.0 license header and a clear note about being auto-generated. This is good practice for maintaining consistency and clarity in the project.
1-76: Overall: Well-structured and efficient workflow configuration.This new workflow file for building the example client Docker image is well-designed and follows GitHub Actions best practices. It leverages reusable workflows, uses appropriate trigger events with path filtering, and considers security aspects.
The file is consistent with the project's coding standards and effectively serves its purpose. Great job on implementing this workflow!
example/helm/values.yaml (1)
65-65: Verify the impact of changing distance_type from cos to l2The distance metric for NGT has been changed from cosine (cos) to Euclidean (l2). This change aligns with the comment "We use L2-Norm for distance_type." However, this modification can significantly impact search results, performance, and how client applications interpret the results.
- Please confirm if this change is intentional and has been thoroughly tested with the example client and other components.
- Ensure that all documentation referencing the distance metric is updated to reflect this change.
- Consider adding a comment explaining the rationale behind this change, especially if it's part of a larger optimization or feature update.
To help verify the impact of this change across the codebase, you can run the following script:
This script will help identify areas of the codebase that might be affected by the distance type change, allowing for a more comprehensive review of its impact.
✅ Verification successful
Change to
distance_typefromcostol2has been verified successfully. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for references to distance_type in the codebase # Search for distance_type in YAML files echo "Searching for distance_type in YAML files:" rg -A 5 'distance_type:' --type yaml # Search for references to distance calculation in Go files echo "Searching for distance calculation references in Go files:" rg -A 5 'distance.*type' --type go # Search for cosine distance references echo "Searching for cosine distance references:" rg -A 5 'cos(ine)?.*distance' --type go --type yaml # Search for L2/Euclidean distance references echo "Searching for L2/Euclidean distance references:" rg -A 5 'l2|euclidean.*distance' --type go --type yamlLength of output: 57838
Makefile.d/build.mk (4)
34-34: LGTM: Example client added to binary build targetThe addition of
example/client/clientto thebinary/buildtarget is correct and maintains the existing structure and order.
107-109: Clarify HDF5 dependency for example clientThe build rule for the example client includes HDF5-related flags and version information. Could you please clarify if the example client actually requires HDF5? If not, consider simplifying the build command to:
example/client/client: $(eval CGO_ENABLED = 1) $(call go-example-build,example/client,-linkmode 'external',$(LDFLAGS), cgo,,$@)This would remove the potentially unnecessary HDF5 dependency.
127-127: LGTM: Example client added to zip artifactsThe addition of the example client to the
binary/build/ziptarget and the corresponding zip artifact creation rule are correct and consistent with the existing structure.Also applies to: 202-204
34-34: Summary of changesThe modifications to
Makefile.d/build.mksuccessfully integrate the example client into the build and packaging process. The changes are consistent with existing patterns and achieve the PR objective. However, please address the query regarding the HDF5 dependency for the example client to ensure we're not introducing unnecessary complexity.Also applies to: 107-109, 127-127, 202-204
Makefile.d/docker.mk (2)
60-60: LGTM: Consistent addition of example-client to docker/xpanes/buildThe addition of
docker/build/example-clientto thedocker/xpanes/buildtarget is consistent with the changes made to thedocker/buildtarget and aligns with the PR objective. The new line is correctly placed to maintain the alphabetical order of the commands.
Line range hint
1-399: Overall: Changes for example-client are well-implementedThe modifications to
Makefile.d/docker.mksuccessfully integrate the example-client into the Docker build process. The changes are consistent across different targets and follow the established patterns in the file. The only points of attention are:
- Verifying the intentional removal of
helm-operatorfrom thedocker/buildtarget.- Considering adding a brief explanation for the ZLIB_VERSION and HDF5_VERSION build arguments in the
docker/build/example-clienttarget.These changes effectively support the PR objective of adding an example-client Docker image to the project.
Makefile.d/functions.mk (1)
94-121: 🛠️ Refactor suggestionReview the new
go-example-buildfunction for consistency and potential improvements.The new
go-example-buildfunction is similar to the existinggo-buildfunction but with some key differences:
- It uses
$(ROOTDIR)/$1instead of$(ROOTDIR)/cmd/$1for finding Go files.- It changes the working directory before running the build command.
- Vald-specific build flags (e.g., AlgorithmInfo, BuildTime) are not included.
- The output binary path is set to
$(ROOTDIR)/$6.- It doesn't run the version check after building.
Consider the following suggestions:
- Add
echo $(PBGOS)for consistency withgo-build.- Evaluate if the working directory change affects relative path resolution.
- Confirm if the removal of Vald-specific build flags is intentional for example clients.
- Consider adding a version check after building, if applicable.
Here's a suggested improvement to address point 1:
define go-example-build echo $(GO_SOURCES_INTERNAL) - echo $(PBGOS) + echo $(PBGOS) echo $(shell find $(ROOTDIR)/$1 -type f -name '*.go' -not -name '*_test.go' -not -name 'doc.go') cd $(ROOTDIR)/$1 && \ CFLAGS="$(CFLAGS)" \ ...To verify the impact of the working directory change, run:
This will help identify any relative imports that might be affected by the working directory change.
Makefile (3)
28-28: LGTM: Variable definition moved.The
AGENT_NGT_IMAGEvariable definition has been relocated to a more appropriate position in the file. This change improves readability and organization without affecting functionality.
39-39: LGTM: New variable for example-client image added.The
EXAMPLE_CLIENT_IMAGEvariable has been introduced, which aligns with the PR objective of adding an example-client docker image. The naming convention is consistent with other image variables in the file.
Line range hint
1-1000: Summary: Makefile changes support example-client image addition.The modifications to this Makefile are minimal and focused, supporting the PR objective of adding an example-client docker image. The changes include:
- Relocating the
AGENT_NGT_IMAGEvariable definition for better organization.- Adding a new
EXAMPLE_CLIENT_IMAGEvariable for the example-client image.These changes are consistent with the existing structure and naming conventions in the Makefile. They provide the necessary groundwork for building and managing the new example-client docker image without introducing any apparent issues or inconsistencies.
dockers/example/client/Dockerfile (2)
21-21:⚠️ Potential issuePin base images to specific versions or digests for reproducible builds.
Currently, the
FROMstatements use unpinned images (ghcr.io/vdaas/vald/vald-buildbase:nightlyandgcr.io/distroless/static:nonroot) without specifying exact versions or digests. This can result in non-reproducible builds if the base images are updated. It's recommended to pin the base images to specific tags or digests to ensure consistency.Run the following script to identify the use of unpinned base images in Dockerfiles:
#!/bin/bash # Description: Find unpinned base images in Dockerfiles. # List FROM statements without a tag or digest rg '^FROM\s+\S+:(?!\S)' -g '*Dockerfile*'Also applies to: 90-90
19-19: 🛠️ Refactor suggestionConsider removing the unused
ARG UPX_OPTIONS.The
ARG UPX_OPTIONS=-9is declared but not utilized in this Dockerfile. Keeping unused build arguments can lead to confusion. It's good practice to remove them to simplify the Dockerfile.Run the following script to check if
UPX_OPTIONSis used elsewhere in the codebase:✅ Verification successful
UPX_OPTIONS is unused in dockers/example/client/Dockerfile
The
ARG UPX_OPTIONS=-9is declared but not utilized within this Dockerfile. It can be safely removed to simplify the Dockerfile.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the usage of UPX_OPTIONS in Dockerfiles. # Find all occurrences of UPX_OPTIONS in Dockerfiles rg 'UPX_OPTIONS' -g '*Dockerfile*'Length of output: 371
Script:
#!/bin/bash # Description: Search for the usage of UPX_OPTIONS across the entire codebase. rg 'UPX_OPTIONS'Length of output: 1379
Script:
#!/bin/bash # Description: Check for the usage of UPX_OPTIONS in dockers/example/client/Dockerfile beyond its declaration. # Search for UPX_OPTIONS usage excluding its declaration rg 'UPX_OPTIONS' dockers/example/client/Dockerfile | grep -v 'ARG UPX_OPTIONS'Length of output: 78
| jobs: | ||
| build: | ||
| uses: ./.github/workflows/_docker-image.yaml | ||
| with: | ||
| target: example-client | ||
| secrets: inherit |
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: 'example-client' target not configured in _docker-image.yaml.
- The
example-clienttarget specified in the main workflow is missing from.github/workflows/_docker-image.yaml.
🔗 Analysis chain
LGTM: Efficient job definition using reusable workflow.
The job definition is concise and leverages a reusable workflow, which is an excellent practice for maintaining consistency and reducing duplication across different workflows.
To ensure the referenced workflow file exists and is correctly configured, please run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and basic structure of the _docker-image.yaml file
if [ -f .github/workflows/_docker-image.yaml ]; then
echo "File _docker-image.yaml exists. Checking its content:"
grep -E "name:|on:|jobs:" .github/workflows/_docker-image.yaml
else
echo "Error: _docker-image.yaml file not found in .github/workflows/"
fi
Length of output: 962
Script:
#!/bin/bash
# Verify that 'example-client' target is correctly configured in _docker-image.yaml
if [ -f .github/workflows/_docker-image.yaml ]; then
echo "Checking for 'example-client' target configuration in _docker-image.yaml:"
rg "target:\s*example-client" .github/workflows/_docker-image.yaml
if [ $? -eq 0 ]; then
echo "'example-client' target is properly configured."
else
echo "Error: 'example-client' target not found in _docker-image.yaml."
fi
else
echo "Error: _docker-image.yaml file not found in .github/workflows/"
fi
Length of output: 442
| docker/build/example-client \ | ||
| docker/build/gateway-filter \ | ||
| docker/build/gateway-lb \ | ||
| docker/build/gateway-mirror \ | ||
| docker/build/helm-operator \ |
There was a problem hiding this comment.
💡 Codebase verification
helm-operator is still referenced in Makefile.d/docker.mk
The removal of docker/build/helm-operator was intended; however, it is still present in the following lines of Makefile.d/docker.mk:
- Line 36
- Line 291
- Line 293
🔗 Analysis chain
Verify the removal of helm-operator from the docker/build target
The addition of docker/build/example-client is consistent with the PR objective. However, please confirm if the removal of docker/build/helm-operator from this target is intentional. If it's not, consider adding it back to maintain consistency with other build targets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if helm-operator is built elsewhere
grep -n "docker/build/helm-operator" Makefile.d/*.mk
Length of output: 529
vankichi
deleted the
backport/release/v1.7/feature/dockers/add-example-client-docker-image
branch
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Improvements
These changes improve the efficiency and automation of the build and deployment processes for the example client application.