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
| 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.
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 <yusencao@outlook.com>
Signed-off-by: Tom <yusencao@outlook.com>
Signed-off-by: Tom <yusencao@outlook.com>
hzxuzhonghu
left a comment
There was a problem hiding this comment.
@lec-bit any other comment
|
[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 DetailsNeeds 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?: