custom-plugin-monitor: Honor default condition status#1266
Conversation
|
|
|
Welcome @halaney! |
|
Hi @halaney. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: halaney The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi! Can I get some review on this? |
| klog.Warningf("Condition %q has invalid initial status %q, defaulting to %q", | ||
| conditions[i].Type, conditions[i].Status, types.False) | ||
| conditions[i].Status = types.False | ||
| } |
There was a problem hiding this comment.
Currently none of the existing configs/*.json files with "plugin": "custom" actually set status on a condition. This means Status would now come in as "" which doesn't match True/False/Unknown, and by the new logic, go to the default branch and log a warning.
That means each CustomPluginMonitor condition without status will log a warning once at startup ( Condition "NTPProblem" has invalid initial status "", defaulting to "False". )
It may be worth honoring the default as such:
case "":
conditions[i].Status = types.False
There was a problem hiding this comment.
hah, duh! thanks I'll make that change. We've been running this (plus my other open PRs) in our setup, but we use our own custom plugins and do set that.
Users can define a default status, but that's being stomped on currently. Let's honor what's defined, if defined correctly... otherwise continue to default to False and log a warning.
47db5e1 to
450d20e
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: halaney The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Users can define a default status, but that's being stomped on currently. Let's honor what's defined, if defined correctly... otherwise continue to default to False and log a warning.