Always allow converting MySQL[AAD]Users back to v1alpha1 #1393
Conversation
The output of `make manager` goes into bin/ which is already ignored. This entry was causing very confusing things to happen with directories called `manager` further down the directory tree.
(Unless JSON serialisation fails for some reason.) Instead we store the changed fields in an annotation and allow roundtripping that when converting in the other direction.
babbageclunk
requested review from
davefellows,
jananivMS,
matthchr,
Porges,
theunrepentantgeek and
WilliamMortlMicrosoft
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
(Unless JSON serialisation fails for some reason.) Instead we store the changed fields in an annotation and allow roundtripping that when converting in the other direction.
babbageclunk
force-pushed
the
mysqlaaduser-no-err-conversion
branch
from
8b99242 to
f2b1705
Compare
| } | ||
| sort.Strings(dbNames) |
There was a problem hiding this comment.
What's the logic behind sorting this? The collection is "ordered" already (in that the customer has specified some order) - is there a problem with just using whatever dbname they specified first?
There was a problem hiding this comment.
Purely for repeatability and testing - I'll add a comment. Although it's ordered in the text of the yaml, we only see it in a map, by which time the ordering is lost.
| } | ||
| if dst.Spec.DatabaseRoles == nil { | ||
| dst.Spec.DatabaseRoles = make(map[string][]string) | ||
| } | ||
| dst.Spec.DatabaseRoles[src.Spec.DBName] = append([]string(nil), src.Spec.Roles...) |
There was a problem hiding this comment.
Is this just a no-op in the case where there was a stashed annotation? (The assignment has actually already happened but we're just doing it again?)
There was a problem hiding this comment.
Not necessarily a no-op- if a user was doing an edit they might send back a resource with changed roles for the selected database (but the original values in the annotation). But in general probably.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The MySQL happy path test seem to be failing intermittently - it looks like it's just timing out creating the server (before anything in this PR). I'll have a look at bumping up the timeout in a separate PR. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Or I guess if it's failing so frequently that I can't merge it I'll try bumping it up in this PR |
This is perpetually timing out for me at the moment, and testing manually shows that creation can take more than an hour.
babbageclunk
force-pushed
the
mysqlaaduser-no-err-conversion
branch
from
9f3f175 to
27020a9
Compare
Previously if a v1alpha2☹️ ) in the system to go into a loop trying to request the resource as v1alpha1 every second and spamming the log with errors.
MySQLUserorMySQLAADUserhad server-level roles or roles in more or fewer than one database, trying to convert it to a v1alpha1 representation would fail because the older format could only refer to exactly one database. This caused something (likely a cache but I'm not entirely sureChange the from conversion to store the affected fields as JSON in an
azure.microsoft.com/convert-stashannotation, and the to conversion to incorporate the annotation values when going back to v1alpha2.Fixes #1388.
How does this PR make you feel:
If applicable: