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
✨ MachineHealthCheck now supports external remediation templates #3606
✨ MachineHealthCheck now supports external remediation templates #3606
Conversation
Hi @jan-est. 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/test-infra repository. |
/assign @ncdc |
CC @n1r1 |
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.
/ok-to-test
add85dc
to
0eef674
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.
Overall looks good,
We also need to add some tests |
0eef674
to
abc12ec
Compare
Hmm I could have sworn that used to be possible. |
/approve cancel Waiting for another round of review before approving :) |
cb60b2c
to
99393b6
Compare
@jan-est: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
fb29193
to
050798a
Compare
050798a
to
9aa6fe4
Compare
LGTM |
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.
Just one minor nit.
Also it may be nice to have tests to assert that the expected conditions are being set whenever appropriate.
api/v1alpha3/condition_consts.go
Outdated
ExternalRemediationTemplateNotFound = "ExternalRemediationTemplateNotFound" | ||
|
||
// ExternalRemediationRequestAvailable is set on machinehealthchecks when MachineHealthCheck controller uses external remediation. | ||
// ExternalRemediationTemplateAvailable is set to false if creating external remediation request fails. |
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.
// ExternalRemediationTemplateAvailable is set to false if creating external remediation request fails. | |
// ExternalRemediationRequestAvailable is set to false if creating external remediation request fails. |
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.
How badly this PR blocks 3713? Maybe we could enhance testing in another PR?
9aa6fe4
to
a34fad7
Compare
a34fad7
to
83bf1a0
Compare
/hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc 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 |
What this PR does / why we need it:
This PR implements External Remediation proposal 3190