add example-client docker image #2705
Conversation
Signed-off-by: vankichi <[email protected]>
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes aimed at enhancing the automation and build processes for a Docker image related to an example client. A new GitHub Actions workflow file is added to automate Docker image builds triggered by specific events. Modifications are made to various Makefiles to support new targets and functions for building the example client and its Docker image. Additionally, a Dockerfile is provided to define the multi-stage build process for the client application. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
.github/workflows/dockers-example-client-image.yaml (2)
21-70: Comprehensive triggers, but consider optimizing path filters.The workflow triggers are well-defined, covering various scenarios including pushes to important branches and tags, as well as pull requests that modify relevant files. The use of
pull_request_targetis appropriate for handling contributions from forks securely.However, the path filters for pull requests and pull_request_target events are quite extensive. While this ensures that the workflow runs when necessary, it might lead to frequent triggering of the workflow, potentially increasing CI load and slowing down development.
Consider reviewing the path filters to ensure they only include files that directly affect the example-client Docker image. For instance, some internal paths like
internal/**might not be necessary unless they contain code specifically used by the example client.
71-76: LGTM: Job definition is concise and utilizes reusable workflow.The job definition effectively uses a reusable workflow, which is a good practice for maintaining consistency across similar jobs and reducing duplication. The use of a parameter for the target allows for flexibility, and inheriting secrets ensures secure access to necessary credentials.
Consider adding a notification step or a separate job that runs if the build fails. This could help alert the team quickly if there are issues with the Docker image build process. For example:
jobs: build: uses: ./.github/workflows/_docker-image.yaml with: target: example-client secrets: inherit notify-on-failure: needs: build if: failure() runs-on: ubuntu-latest steps: - name: Send notification uses: 8398a7/action-slack@v3 with: status: ${{ job.status }} text: 'Docker image build for example-client failed' env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK }}This example uses Slack for notifications, but you can adapt it to your preferred notification method.
Makefile.d/docker.mk (1)
388-399: LGTM: Correctly implemented example-client Docker targets.The new targets
docker/name/example-clientanddocker/build/example-clientare well-implemented and consistent with other targets in the Makefile. They correctly use the appropriate variables and include necessary build arguments.For consistency with other targets, consider adding a comment above the
docker/build/example-clienttarget to describe its purpose, similar to other build targets in the file.You could add a comment like this above the
docker/build/example-clienttarget:.PHONY: docker/build/example-client +## build example-client image docker/build/example-client:Makefile.d/functions.mk (1)
94-131: LGTM with suggestions for improvementThe
go-example-buildfunction is well-structured and serves its purpose of building Go projects in a specified directory. However, there are a few areas where it could be improved:
Error handling: Consider adding error checking after the
go buildcommand to ensure the build was successful before attempting to print the version.Main file location: The function assumes
main.gois in the root of the specified directory. Consider making this more flexible by searching for the main file or allowing it to be specified as a parameter.Version printing: The
-versionflag might not be available for all binaries. Consider making this step optional or handling potential errors.Here's a suggested improvement for error handling and flexible main file location:
define go-example-build echo $(GO_SOURCES_INTERNAL) echo $(PBGOS) echo $(shell find $(ROOTDIR)/$1 -type f -name '*.go' -not -name '*_test.go' -not -name 'doc.go') cd $(ROOTDIR)/$1 && \ + MAIN_FILE=$$(find . -name 'main.go' | head -n 1) && \ + if [ -z "$$MAIN_FILE" ]; then \ + echo "Error: main.go not found in $(ROOTDIR)/$1" >&2; \ + exit 1; \ + fi && \ CFLAGS="$(CFLAGS)" \ CXXFLAGS="$(CXXFLAGS)" \ CGO_ENABLED=$(CGO_ENABLED) \ CGO_CXXFLAGS="$3" \ CGO_FFLAGS="$3" \ CGO_LDFLAGS="$3" \ GO111MODULE=on \ GOARCH=$(GOARCH) \ GOOS=$(GOOS) \ GOPRIVATE=$(GOPRIVATE) \ GO_VERSION=$(GO_VERSION) \ go build \ --ldflags "-w $2 \ -extldflags '$3' \ -X '$(GOPKG)/internal/info.AlgorithmInfo=$5' \ -X '$(GOPKG)/internal/info.BuildCPUInfoFlags=$(CPU_INFO_FLAGS)' \ -X '$(GOPKG)/internal/info.BuildTime=$(DATETIME)' \ -X '$(GOPKG)/internal/info.CGOEnabled=$(if $(filter 1,$(strip $(CGO_ENABLED))),true,false)' \ -X '$(GOPKG)/internal/info.GitCommit=$(GIT_COMMIT)' \ -X '$(GOPKG)/internal/info.GoArch=$(GOARCH)' \ -X '$(GOPKG)/internal/info.GoOS=$(GOOS)' \ -X '$(GOPKG)/internal/info.GoVersion=$(GO_VERSION)' \ -X '$(GOPKG)/internal/info.Version=$(VERSION)' \ -buildid=" \ -modcacherw \ -mod=readonly \ -a \ -tags "osusergo netgo static_build$4" \ -trimpath \ -o $(ROOTDIR)/$6 \ - main.go - $6 -version + $$MAIN_FILE && \ + if [ $$? -eq 0 ]; then \ + $(ROOTDIR)/$6 -version || true; \ + else \ + echo "Error: Build failed" >&2; \ + exit 1; \ + fi endefThese changes will make the function more robust and flexible.
📜 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 (6)
- .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)
🧰 Additional context used
🔇 Additional comments (11)
.github/workflows/dockers-example-client-image.yaml (1)
1-20: LGTM: File header and metadata are well-structured.The copyright notice, edit warning, and workflow name are all correctly formatted and provide clear information about the file's purpose and origin.
Makefile.d/build.mk (5)
34-34: LGTM: Example client added to binary build targetThe addition of
example/client/clientto thebinary/buildtarget is appropriate and consistent with the existing structure. This ensures that the example client is built along with other binaries in the project.
127-127: LGTM: Example client artifact added to zip targetThe addition of
artifacts/vald-example-client-$(GOOS)-$(GOARCH).zipto thebinary/build/ziptarget is appropriate and consistent with the existing structure. This ensures that the example client is included in the zip artifacts along with other binaries in the project.
201-204: LGTM: New rule for example client zip artifactThe new rule for creating the
vald-example-client-$(GOOS)-$(GOARCH).zipartifact is well-structured and consistent with other artifact creation rules in the Makefile. It correctly ensures the target directory exists and uses the appropriatezipcommand to create the artifact.
Line range hint
1-204: Summary: Example client successfully integrated into build processThe changes to
Makefile.d/build.mksuccessfully integrate the example client into the build and packaging process. The additions are consistent with the existing structure and follow established patterns. Key points:
- The example client is now part of the main binary build target.
- A new build rule for the example client is added, using CGO and appropriate flags.
- The example client is included in the zip artifacts target.
- A new rule for creating the example client zip artifact is added.
These changes appear to be well-implemented and maintain the consistency of the Makefile. The only outstanding question is about the
go-example-buildfunction, which may require further clarification.
107-109: LGTM: New build rule for example clientThe new build rule for
example/client/clientis well-structured and consistent with other CGO-enabled builds in the Makefile. It correctly setsCGO_ENABLED = 1and uses appropriate linkage and flags.Could you please clarify the following:
- What is the
go-example-buildfunction, and how does it differ from the standardgo-buildfunction?- Is the dependency on HDF5 necessary for the example client?
To verify the
go-example-buildfunction, please run:✅ Verification successful
Clarifications on
go-example-buildFunction and HDF5 DependencyThe
go-example-buildfunction is defined inMakefile.d/functions.mk. It serves as a custom wrapper around the standardgo buildcommand, incorporating additional configurations to handle CGO settings and specific linker flags required for building the example client.Regarding the HDF5 dependency, the inclusion of
HDF5_LDFLAGSandHDF5_VERSIONin the build command indicates that the example client depends on the HDF5 library. This dependency is necessary for the client's functionality related to HDF5 operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definition of go-example-build function rg --type make "go-example-build\s*=\s*" -A 10Length of output: 48
Script:
#!/bin/bash # Search for all occurrences of go-example-build in makefiles rg --type make "go-example-build" -A 10Length of output: 1199
Makefile.d/docker.mk (3)
32-32: LGTM: Correctly integrated example-client into the build process.The addition of
docker/build/example-clientto thedocker/buildtarget ensures that the example-client Docker image is built along with other images. This change is consistent with the existing structure and enhances the build system.
60-60: LGTM: Correctly integrated example-client into parallel build process.The addition of
docker/build/example-clientto the xpanes command ensures that the example-client Docker image is built in parallel with other images. This change is consistent with the existing structure and improves build efficiency.
Line range hint
1-399: Overall: Well-implemented changes for example-client Docker image.The changes to integrate the example-client Docker image into the build process are well-implemented and consistent with the existing structure of the Makefile. All necessary targets have been added correctly, and the changes enhance the build system by including the new image in both the main build process and the parallel build using xpanes.
A minor suggestion was made to add a comment for consistency, but otherwise, these changes are ready for approval.
Makefile (2)
28-28: LGTM: Variable reordering looks good.The repositioning of the
AGENT_NGT_IMAGEvariable definition improves the overall organization of the Makefile. This change doesn't affect functionality and potentially enhances readability.
39-39: LGTM: New variable for example client image.The addition of the
EXAMPLE_CLIENT_IMAGEvariable is consistent with the existing naming conventions and aligns with the PR objective of adding an example-client docker image. This change sets the groundwork for building and managing the new example client image within the project's build system.
Signed-off-by: vankichi <[email protected]>
Deploying vald with
|
| Latest commit: |
a160aa0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6e4790fa.vald.pages.dev |
| Branch Preview URL: | https://feature-dockers-add-example.vald.pages.dev |
Signed-off-by: vankichi <[email protected]>
Signed-off-by: vankichi <[email protected]>
Signed-off-by: vankichi <[email protected]>
* 🐳 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]>
* 🐳 add example-client docker image * 🐛 fix build error * ♻️ fix * ♻️ fix * ♻️ fix --------- Signed-off-by: vankichi <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
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.