Switch KRShaped interface to retrieve entire condition set#1307
Conversation
knative-prow-robot
added
the
size/M
|
/assign @n3wscott |
|
|
||
| GetStatus() *Status | ||
|
|
||
| GetTopLevelConditionType() apis.ConditionType |
There was a problem hiding this comment.
Here is a hitch in this plan: for things like alternate broker implementations, the reconciler for the broker gets to redefine what the ConditionSet for the resource is, and it ignores the type's conditions set, it also avoids all the Mark* methods. So it might not be true that this condition set is the one in play for the resource under reconciliation.
There was a problem hiding this comment.
We we could either say alternate brokers are not KRShaped and need to manage things on their own or if they share the same top-level condition, define a set with only that.
There was a problem hiding this comment.
For the alternate broker in https://github.com/google/knative-gcp we have a separate API type that defines its own ConditionSet specific to the alternate implementation. There's a separate generated reconciler for that type that the alternate broker controller uses to reconcile. I'm not sure if this is the right pattern long-term.
There was a problem hiding this comment.
@n3wscott do you have an example of a single type that works with multiple ConditionSets? I don't know of any.
There was a problem hiding this comment.
Assuming "Top Level" means Ready/Succeeded, then generally conformance dictates that this should not vary. I think that the rest can vary at all levels of severity, assuming our semantics are adhered to.
There was a problem hiding this comment.
OK, just note we have an issue if we ever use any other method on the ConditionSet other than the toplevel method.
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: n3wscott, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
7 participants
Issue #1226
As I looked at integrating this downstream I've realized this will be more flexible so that we can have generic awareness of all the conditions that exist for a resource. This will also allow us to ensure they are initialized.