Default and Enforced Compositions #1585
Conversation
muvaf
changed the title
|
I've added enforced composition as well since it's pretty similar to default one. All manually tested, ready for review. |
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
…erence or label selector is given in composite resource Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
…tion for all of its composite instances Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
- Enforced and default composition selectors do not do compatibility check. - Enforced composition ref in infradef is made immutable. Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
…default or enforced composition Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
muvaf
force-pushed
the
def-comp
branch
2 times, most recently
from
d494cf4
to
c42fe11
Compare
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
|
Tested manually with the latest changes. |
| return nil | ||
| } | ||
| cp.SetCompositionReference(def.Spec.DefaultCompositionRef) | ||
| s.recorder.Event(cp, event.Normal(reasonCompositionSelection, "Default composition has been selected")) |
There was a problem hiding this comment.
Is this information really worth plumbing a recorder down here for? Would we still be able to record "A composition has been selected" without specifically communicating that it was a default or enforced composition if we did this further up in the stack?
There was a problem hiding this comment.
I think what composition is selected is already clear from its field on spec but I find it valuable to see how it's selected to avoid situations where it's relatively hard to figure out what's happening. This could happen in this situation because the person who is creating a requirement or composite resource is kind of far from seeing what default/enforced composition is easily, it's like you have to check the CRD to see it which we never really do. It's also good to educate people who are exploring how stuff works at the place where they do that exploration directly. So, I found it helpful to say how it made it here, if it's exceptional like default/enforced, and what is selected is already shown in spec when you do kubectl describe
| // as its target type reference is immutable. | ||
| if cp.GetCompositionReference().String() == s.def.Spec.EnforcedCompositionRef.String() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Do we need this branch? It seems like if we removed it we'd have a no-op SetCompositionReference below, so the only difference is we might emit an event despite the enforced composition already being set.
There was a problem hiding this comment.
Yeah, I believe the event should be fired only when it's actually fired to keep the chronological order of events digestable. I'd like to avoid a situation where there are events on the object but one of every two event is enforced composition is selected whereas it's actually selected only if there is a drift.
…tion reference Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
|
@negz Thanks for the review! It's ready for another look. |
Description of your changes
This PR allows infra-operators to choose a default composition to be selected in case no selector is given as well as an enforced composition to be selected no matter what.
Fixes #1471
Fixes #1470
How has this code been tested?
Manually tested.
Checklist
I have:
make reviewableto ensure this PR is ready for review.appropriate.
For more about what we believe makes a pull request complete, see our
definition of done.