WIP: pkg/cvo/availableupdates: Add recommend/... conditions for each matching or eval-failed risk #1132
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wking 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 |
openshift-ci
bot
added
the
approved
wking
force-pushed
the
condition-for-each-matching-conditional-update-risk
branch
10 times, most recently
from
7588be6 to
a0a3b54
Compare
|
/cc |
pkg/cvo/availableupdates.go
Outdated
| msg := unknownExposureMessage(risk, err) | ||
| errorMessages = append(errorMessages, msg) | ||
| conditions = append(conditions, metav1.Condition{ | ||
| Type: fmt.Sprintf("%s:%s:%s", ConditionalUpdateConditionTypeRecommended, recommendedReasonEvaluationFailed, cleanName), |
There was a problem hiding this comment.
What is the reason for including the recommendedReasonEvaluationFailed in the type of the condition when we fail to evaluate it? To me a Recommended:RiskName condition with Status=Unknown and Reason=Something sounds like it does the job...
Also, it seems that : is not a valid character for condition type:
// The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
// +required
// +kubebuilder:validation:Required
// +kubebuilder:validation:Pattern=`^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$`
// +kubebuilder:validation:MaxLength=316
But (dns1123SubdomainFmt/)?(qualifiedNameFmt) sounds like we can invent something like recommended.openshift.io/RiskName with sounds good to me.
There was a problem hiding this comment.
Thanks; I've pivoted to recommended/... with a0a3b54 -> c5053ba. I'm a week minus on .openshift.io there; seems unlikely that other folks besides us would be injecting their own conditions, and we don't do that kind of DNS scoping for our core conditions like Failing and otherwise 🤷 But I don't mind adding the scoping if folks think it's worthwhile.
There was a problem hiding this comment.
yeah recommended/risk sounds good to me 👍
wking
force-pushed
the
condition-for-each-matching-conditional-update-risk
branch
from
a0a3b54 to
aad6ef7
Compare
…ching or eval-failed risk To make it easier for clients to explain individual risks, instead of only having access to a generic MultipleReasons 'reason' and a long 'message' covering all the risks. The "recommended/" prefix conforms to [1]: +kubebuilder:validation:Pattern=`^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$` [1]: https://github.com/kubernetes/apimachinery/blob/3e8e52d6a1259ada73f63c1c7d1fad39d4ba9fb4/pkg/apis/meta/v1/types.go#L1609-L1613
wking
force-pushed
the
condition-for-each-matching-conditional-update-risk
branch
from
aad6ef7 to
c5053ba
Compare
wking
changed the title
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
To make it easier for clients to explain individual risks, instead of only having access to a generic
MultipleReasonsreasonand a longmessagecovering all the risks.