Align Eventing conditions with Serving conditions. #62
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes 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 |
knative-prow-robot
added
the
approved
knative-prow-robot
added
the
size/L
There was a problem hiding this comment.
@markusthoemmes: 0 warnings.
In response to this:
Proposed Changes
One more step towards reusable functions between Eventing and Serving: This aligns the conditions and helpers for setting those conditions between Eventing and Serving.
For now, the helper code is mostly a copy. Potential collapsing of the repetetiveness might come later. This is more about the calling site than it is about the definition site.
Release Note
NONE/assign @jcrossley3 @houshengbo
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.
|
@jcrossley3 not sure if that's really "possible" with the current scheme. We can have Version on there no problem but I'm not so sure about the condition stuff. Let's do that in a followup? |
|
@markusthoemmes as long as you don't name it |
|
@jcrossley3 I think the "issue" is that the conditionSet is a global singleton and not captured by the type itself. The lifecycle functions capture that global conditionSet and do stuff based on it so we'd have to see how we can generalize that. |
|
I guess we could potentially use the same conditionset for both, but that'd mean that they can't diverge at all which is not necessarily an assumption I'd like to take right now. |
|
I'm absolutely assuming the conditionSet will be identical for both, as well as the other status fields. |
|
I don't. Once we add "Ingress" into Serving, we might want to surface its readiness in a separate condition for example. |
|
|
|
I think that's the wrong condition to use. I thought DependenciesInstalled is for external dependencies, i.e.: Handled by an external actor and not by this operator at all. If you use it here, you lose external control over readiness. |
|
No. I think of it as a placeholder or hook. There's nothing preventing this operator from using it, and I expect it will once ingress selection becomes a part of our API. We never want to do anything that would prevent downstream operators from using it, too, of course. |
|
That will though. It needs to remain untouched by this operator to be usable from a downstream operator. Else they will trample each other's state. |
|
They will need to coordinate, that's true. |
|
So, in terms of staging things and merges: I intended to land the interfaces first and not go into too much detail on how best to consolidate code on the definition site. Once the interfaces are landed, we can separately move definition site consolidation (i.e. making the type definitions more dense) and caller consolidation (reusing business logic) forward. I think it's valuable for us to move forward, especially looking at business logic reuse. I'm happy to discuss this as part of the definition site consolidation at a later stage. Does that make sense? |
|
The following is the coverage report on the affected files.
|
|
/lgtm |
Proposed Changes
One more step towards reusable functions between Eventing and Serving: This aligns the conditions and helpers for setting those conditions between Eventing and Serving.
For now, the helper code is mostly a copy. Potential collapsing of the repetetiveness might come later. This is more about the calling site than it is about the definition site.
Release Note
/assign @jcrossley3 @houshengbo