-
Notifications
You must be signed in to change notification settings - Fork 0
0.6.0-0.24.0: use selectors where required, raise Shift functions, and set required defaults #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
escalation.new(id) | ||
+ escalation.spec.parameters.forProvider.escalationChainRef.withName(self.chainName) | ||
+ escalation.spec.parameters.forProvider.escalationChainSelector.withMatchLabels({ | ||
'crossplane.io/claim-name': this.chainName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically, the same claim-name could exist in a different namespace, so this is probably not sufficient in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a downside of keeping all the logic in jsonnet as opposed to using Crossplane Compositions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't/won't escalationChainRef.withName()
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answering myself, the withName()
refers to the managed resource, which has a generated name, the withMatchLabels()
is better in that regard.
3049366
to
d55ff11
Compare
077d65e
to
32e4aa4
Compare
BREAKING CHANGE: Escalation Chain constructor now requires a `namespace` argument.
BREAKING CHANGE: Schedule constructor now requires a `namespace` argument.
32e4aa4
to
e54bbdd
Compare
f152f56
to
bbee669
Compare
+ forProvider.shiftsSelector.withMatchLabels(this.scheduleShiftLabels), | ||
shifts: [ | ||
// Inject matching labels to identify Shifts as belonging to this Schedule. | ||
raw.oncall.v1alpha1.onCallShift.metadata.withLabels(this.scheduleShiftLabels) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work because the latter sets the labels on the claim resource while selector.matchLabels
tells Crossplane to look at the managed resources.
I've got a theoretical solution in the pipeline: 88ae384
The idea is to set a label on the managed resource this way, then let selector.matchLabels
find it, however we'll need to verify that the code in Crossplane can actually deal with a selector that returns multiple resources.
This PR includes a suite of changes I've made while testing this out in practice.