-
Notifications
You must be signed in to change notification settings - Fork 247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
helm: add configurable liveness&readiness probes for master topology-updater and worker #1801
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @omerap12 for the patch.
In addition to nfd-master, we should also add support for nfd-master and nfd-topology-updater.
@ArangoGutierrez you were yearning for this, please verify and report back 😊 /assign @ArangoGutierrez |
sure. do you want it in the same PR? |
Yes, enable them all at the same time |
@marquiz do we want to add failureThreshold to the livenessProbes as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to add failureThreshold to the livenessProbes as well?
You could add it as a commented out entry in the values.yaml. Let's not change any defaults in this PR, though
92fe52e
to
1dfe9fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add support for nfd-worker, too, to make this comprehensive and consistent?
Also, adjust the commit message (and the PR title) accordingly, e.g. helm: add support for configuring liveness and readiness probes
or smth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're on it, suggest rewording/re-spelling the commit message (and PR title)
helm: add configurable liveness&readiness probes for master topology-updater and worker
EDIT: *configurable
Added some alignment, maybe that ok? @marquiz |
Looks good to me 👍 |
…updater and worker Signed-off-by: Omer Aplatony <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @omerap12 for the persistence on this. Nice job, looks good to me
/cherry-pick release-0.16
ping @ArangoGutierrez /cherry-pick release-0.16 |
@marquiz: once the present PR merges, I will cherry-pick it on top of release-0.16 in a new PR and assign it to you. In response to this:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: 6b763b4390cc42d8e7b658b9b688132da1e37658
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArangoGutierrez, marquiz, omerap12 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 |
@marquiz: #1801 failed to apply on top of branch "release-0.16":
In response to this:
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. |
Cherry-picked in #1808 |
Fixes: #1730
The following values.yaml:
Will generate: