-
Notifications
You must be signed in to change notification settings - Fork 235
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
fix: Enable TF-based reconciler for SQLInstances without clone source #2731
fix: Enable TF-based reconciler for SQLInstances without clone source #2731
Conversation
For the cases we found in #2706, I think we also want to add the support in the direct SQLInstance reconciler to remove system labels, and recognize/propagate the 0 int value. But I'm good if you want to do that in separate PRs. |
Yeah, this is just a quick fix to unblock the release. |
} | ||
|
||
if (hasTerraformController || hasDCLController) && useDirectReconcilerPredicate == nil { | ||
} else { | ||
logger.Error(fmt.Errorf("no predicate where we have multiple controllers"), "skipping direct controller registration", "group", gvk.Group, "version", gvk.Version, "kind", gvk.Kind) | ||
hasDirectController = false |
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.
Previously we decouple the CRDs and controllers and let the labels on CRD to choose the controller. It only moves from TF to direct but not vice versa (which is an important precondition).
Now the controller decision is switched to the CR object.
I like the reconcile gate idea overall (except that it is very challenge to figure out which reconciler is being used, that could make triaging more difficult). I think my concern is mainly that the reconcile gate has changed the concept about some basic concept about the direct registry and label based reconciliations, that is not covered by any of our guides, tools, and test.
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.
/lgtm
/approve
I'd like to have this PR goes in to unblock users and continue the reconcile gate discussion.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma 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 |
dd9a91c
to
4d28f72
Compare
/lgtm |
c2b5793
into
GoogleCloudPlatform:master
Fixes #2706