fix(makefile): remove unsupported workload-manager run flags#332
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes workload-manager development Makefile targets by removing flags unsupported by cmd/workload-manager/main.go, preventing local runs from failing during flag parsing.
Changes:
- Removed unsupported SSH-related flags from
make runandmake run-local. - Replaced the unsupported
--kubeconfigflag with the standardKUBECONFIGenvironment variable formake run-local.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the run and run-local targets in the Makefile by removing SSH-related flags and modifying how the Kubernetes configuration is handled. Feedback indicates that hardcoding the KUBECONFIG environment variable is redundant and restrictive, as the Kubernetes client library defaults to the same path automatically; removing this explicit assignment would allow users to override the configuration more easily and avoid issues with paths containing spaces.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
+ Coverage 47.57% 49.12% +1.55%
==========================================
Files 30 30
Lines 2819 2858 +39
==========================================
+ Hits 1341 1404 +63
+ Misses 1338 1301 -37
- Partials 140 153 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mrinalchaturvedi27
force-pushed
the
fix-workloadmanager-run-flags
branch
from
009d6d5 to
a5d93d6
Compare
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
/lgtm
We would remove these cmd in future
|
[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 |
|
Clean removal — those flags don't exist anymore so they'd just cause a startup failure. LGTM. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This fixes the workload-manager Makefile run targets. They no longer pass unsupported flags, so local development commands do not fail during flag parsing.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?:
What I fixed
I fixed the workload-manager run commands in the Makefile.
Earlier,
make runandmake run-localpassed flags that the workload-manager binary does not have. Because of that, the command could fail duringflag.Parse()before the server started.How I found it
I checked the flags passed from the Makefile and compared them with the flags defined in
cmd/workload-manager/main.go.The binary has these flags:
portruntime-class-nameenable-tlstls-certtls-keyenable-authIt does not have
ssh-username,ssh-port, orkubeconfig.What I changed
--ssh-username=sandboxfrommake run.--ssh-port=22frommake run.make run-local.--kubeconfigfrommake run-local.Why this is correct
This fixes the issue at the place where the bad flags were added. I did not add new flags to the Go binary because these Makefile flags were not connected to any workload-manager behavior.
For local kubeconfig use, client-go already checks the normal kubeconfig locations and respects any
KUBECONFIGvalue the user has set. Somake run-localdoes not need to force a path.Tests
make -n run- Passed. The command now only passes--port=8080.make -n run-local- Passed. The command now only passes--port=8080.git diff --check- Passed.