allow overriding the runner's containing executable name #1813
Conversation
Signed-off-by: Etai Lev Ran <elevran@gmail.com>
k8s-ci-robot
added
the
kind/feature
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
|
/approve |
k8s-ci-robot
added
the
approved
cmd/epp/runner/runner.go
Outdated
|
|
||
| // Runner is used to run epp with its plugins | ||
| type Runner struct { | ||
| eppExecutable string // the EPP executable name |
There was a problem hiding this comment.
can we do this with a cmd-line arg instead of a field in Runner struct?
the other fields are part of runner so we could set them via code for testing purposes, executable name on the other hand is only for logging when the pod starts.
is there any advantage or special reason to have it as a field?
There was a problem hiding this comment.
do you mean use argv[0] which has the exe name?
we could, but might be a generic epp across multiple executables. Adding it as explicit field, settable by the runner's containing exe, allows more control.
If you mean allow passing a new command line parameter and propagating to the runner to print, I think it is an overkill.
Anoter option is to make it an explicit pramater in the call to runner.Run but that seems like a bigger change across multiple files/repos.
So a field is easily settable, allowing caller control, and has smallest change scope.
There was a problem hiding this comment.
I mean cmd-line arg like:
--executable-name llm-dSigned-off-by: Etai Lev Ran <elevran@gmail.com>
|
/lgtm my comment is a nit, no need to block the PR on that. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elevran, kfswain, nirrozenbaum 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 |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Allow setting of the runners executable name when logging the commit SHA and build informaiton.
Since
runner.Runlogs a fixedGIE build, this PR allows setting a containing executable name for the runner. The executable defaults toGIEas in the current code.The IGW runner is reused outside of this repo (e.g., llm-d sidecar and inference-scheduler). The version values are set
via linker flags (
-X ....) populating the values from the importing repo and not based on IGW's version being used.A different option would be to extend the version struct to include an executable name directly (i.e., allow overriding via linker flags as well).
Which issue(s) this PR fixes:
NA
Does this PR introduce a user-facing change?: