-
Notifications
You must be signed in to change notification settings - Fork 101
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
Default ProviderConfig is automatically selected #202
Conversation
…derConfig reference use the default one Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
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.
Note that this PR adds that new initializer to the default initializer list but there are managed resources who supply their own initializers. So, we'll need to update them manually.
Would it make since for this just to be in the managed reconciler body? Do you think there will be cases where we don't want this behavior?
Alternatively, we could have some sort of defaulter interface to separate this from NewNameAsExternalName()
which feels like something which may be overridden more frequently. Not strictly opposed to what you have here, but just wanted to float a few more ideas :)
Actually, the reason we had introduced the
Keep them coming, it's always helpful for me to reconsider my thought process :) |
I'd like us to have a pretty high bar for adding more logic to the managed reconciler body given how complex it is already. |
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 approach didn't occur to me, but it seems like a good one. I presume we'll also need to update any controllers we still have lying around that don't use the managed resource reconciler?
Yes, that looks like the case :/ |
Description of your changes
If
spec.providerConfigRef
is empty, it will be filled withdefault
.Note that this PR adds that new initializer to the default initializer list but there are managed resources who supply their own initializers. So, we'll need to update them manually.
Fixes crossplane/crossplane#1686
Checklist
I have:
make reviewable
to ensure this PR is ready for review.clusterrole.yaml
to include any new types.