Fix: make build should build kmeshctl #1419
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that the kmeshctl binary is included in the build artifacts and install/uninstall workflows.
- Adds copying of
kmeshctlin the host staging script - Updates Makefile to install and remove the
kmeshctlbinary
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hack/utils.sh | Added cp /usr/bin/kmeshctl out/ in copy_to_host() |
| Makefile | Included $(APPS4) (kmeshctl) in install/uninstall |
Comments suppressed due to low confidence (2)
Makefile:177
- [nitpick] The variable
APPS4is ambiguous. Rename it to something descriptive likeKMEHSCTL_BINorAPPS_KMESHCTLto make its purpose clear.
$(call printlog, INSTALL, $(INSTALL_BIN)/$(APPS4))
Makefile:178
- The build rule does not include a step to compile kmeshctl before installing it. Consider adding a build or
go buildcommand for kmeshctl so the binary exists when the install target runs.
$(QUIET) install -Dp -m 0500 $(APPS4) $(INSTALL_BIN)
hack/utils.sh
Outdated
| @@ -49,6 +49,7 @@ function copy_to_host() { | |||
| cp /usr/bin/kmesh-daemon out/ | |||
| cp /usr/bin/kmesh-cni out/ | |||
| cp /usr/bin/mdacore out/ | |||
| cp /usr/bin/kmeshctl out/ | |||
There was a problem hiding this comment.
[nitpick] The copy_to_host function lists each binary explicitly. Consider iterating over an array of binary names to reduce duplication and simplify future additions.
Copilot uses AI. Check for mistakes.
Flying-Tom
force-pushed
the
make-fix
branch
2 times, most recently
from
7c931c1 to
d2b7ffa
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Signed-off-by: Tom <[email protected]>
Signed-off-by: Tom <[email protected]>
Signed-off-by: Tom <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1113
Special notes for your reviewer:
Does this PR introduce a user-facing change?: