Continue reconciling Azure SQL managed users for unknown errors #1336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These could be setup issues like the AAD admin not being set on the server - giving up immediately means the only way to get the user created successfully is to edit the resource to remove the finalizer, delete it, then re-add. The reconcile loop for regular users doesn't use this pattern of returning true, nil when it can't reach the server.
babbageclunk
requested review from
davefellows,
jananivMS,
matthchr,
Porges,
theunrepentantgeek and
WilliamMortlMicrosoft
as code owners
| instance.Status.SetFailedProvisioning(instance.Status.Message) | ||
| return true, nil | ||
| return false, nil |
There was a problem hiding this comment.
Ergh. I dislike returning boolean values unless they have really simple meanings - better to define an enumeration and be declarative. Something for later, perhaps.
There was a problem hiding this comment.
Agreed! Especially here for Ensure and for Delete - the meaning of these booleans are pretty unclear, you basically need to read the AsyncReconciler code to see how they're used. I'm going to make an issue to clean them up - but obviously it's spread across all of the manager types unfortunately.
theunrepentantgeek
approved these changes
Jan 5, 2021
matthchr
approved these changes
Jan 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These could be setup issues like the AAD admin not being set on the server - giving up on the first try means the only way to get the user created successfully is to edit the resource to remove the finalizer, delete it, then re-add. The Ensure for non AAD users doesn't use this pattern of returning true, nil when it can't reach the server.
If applicable: