Cleanup helm flags which have default values from values yaml #1429
Conversation
✅ 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
needs-ok-to-test
|
Hi @rahulgurnani. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
rahulgurnani
force-pushed
the
helm-cleanup
branch
from
312dabd to
fc20338
Compare
k8s-ci-robot
added
size/M
k8s-ci-robot
added
the
do-not-merge/work-in-progress
| | `inferenceExtension.extraServicePorts` | List of additional service ports to expose. Defaults to `[]`. | | ||
| | `inferenceExtension.flags` | List of flags which are passed through to endpoint picker. | | ||
| | `provider.name` | Name of the Inference Gateway implementation being used. Possible values: `gke`. Defaults to `none`. | | ||
| | `inferenceExtension.enableLeaderElection` | Enable leader election for high availability. When enabled, only one EPP pod (the leader) will be ready to serve traffic. It is recommended to set `inferenceExtension.replicas` to a value greater than 1 when this is set to `true`. Defaults to `false`. | |
There was a problem hiding this comment.
Reading inferenceExtension.flags.has-enable-leader-election is a bit confusing as it looks like a special flag. At least we should document as part of the flags section.
Can we update the documentation like I suggested in the previous PR #1302 (comment) , basically document this as part of the flags section, provide a concrete example, and list has-enable-leader-election as an important example flag.
There was a problem hiding this comment.
Updated. I think referring to values.yaml for example is more suitable. Let me know what you think.. Thanks!
There was a problem hiding this comment.
I think the way its formatted currently would allow this key info to be easily skipped by a reader. I agree with @liu-cong that we should probably bullet list a couple important flags, and then the last bullet can be your link to the exhaustive list.
There was a problem hiding this comment.
I reformatted the flags. I think they are more readable in the README. Also added a sample instruction on the top. Thanks!
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
rahulgurnani
force-pushed
the
helm-cleanup
branch
from
00f4762 to
645cdc3
Compare
k8s-ci-robot
removed
the
lgtm
rahulgurnani
force-pushed
the
helm-cleanup
branch
from
645cdc3 to
8278aa7
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
rahulgurnani
force-pushed
the
helm-cleanup
branch
from
bc7ba86 to
8278aa7
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfswain, rahulgurnani 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 |
k8s-ci-robot
added
the
approved
4 participants
Follow-up of
#1302
Couple of review comments were unaddressed.