Azure SQL FailoverGroup improvements #1361
Conversation
matthchr
requested review from
babbageclunk,
davefellows,
jananivMS,
Porges,
theunrepentantgeek and
WilliamMortlMicrosoft
as code owners
| ./pkg/resourcemanager/keyvaults/unittest/ \ | ||
| ./pkg/resourcemanager/azuresql/azuresqlfailovergroup | ||
| # ./api/... \ |
There was a problem hiding this comment.
Leave comment here explaining why these are commented out
There was a problem hiding this comment.
Was the problem the missing line-continuation on the secrets line? I don't get why the api tests weren't running in that case though.
There was a problem hiding this comment.
Oh, I just got to the azure pipeline change - there was nothing running it before, right.
matthchr
force-pushed
the
feature/azure-sql-failover-group
branch
2 times, most recently
from
bd4b514 to
ce6491b
Compare
| ./pkg/resourcemanager/keyvaults/unittest/ \ | ||
| ./pkg/resourcemanager/azuresql/azuresqlfailovergroup | ||
| # ./api/... \ |
There was a problem hiding this comment.
Was the problem the missing line-continuation on the secrets line? I don't get why the api tests weren't running in that case though.
| @@ -24,6 +33,7 @@ type AzureSqlFailoverGroupSpec struct { | |||
| // +kubebuilder:validation:Required | |||
| Server string `json:"server"` | |||
| FailoverPolicy ReadWriteEndpointFailoverPolicy `json:"failoverPolicy"` | |||
| // TODO: This field should be a ptr as it must not be specified in the failover policy is Manual | |||
There was a problem hiding this comment.
It seems like treating zero as unspecified in this case would be ok, right? Although I guess it's potentially a valid value in the case that the policy is automatic.
There was a problem hiding this comment.
No because in the case that the failover policy is automatic this field is required, and I believe 0 is a valid value. So you'd be unable to represent the 0 + automatic combination.
I just ignored it when the mode is manual (which is a smell IMO).
I've updated the TODO a bit more to clarify this though.
| ./pkg/resourcemanager/keyvaults/unittest/ \ | ||
| ./pkg/resourcemanager/azuresql/azuresqlfailovergroup | ||
| # ./api/... \ |
There was a problem hiding this comment.
Oh, I just got to the azure pipeline change - there was nothing running it before, right.
pkg/resourcemanager/azuresql/azuresqlfailovergroup/azuresqlfailovergroup.go
Outdated
Show resolved
Hide resolved
pkg/resourcemanager/azuresql/azuresqlfailovergroup/azuresqlfailovergroup.go
Outdated
Show resolved
Hide resolved
pkg/resourcemanager/azuresql/azuresqlfailovergroup/azuresqlfailovergroup.go
Outdated
Show resolved
Hide resolved
pkg/resourcemanager/azuresql/azuresqlfailovergroup/azuresqlfailovergroup.go
Show resolved
Hide resolved
pkg/resourcemanager/azuresql/azuresqlfailovergroup/azuresqlfailovergroup_reconcile.go
Outdated
Show resolved
Hide resolved
pkg/resourcemanager/azuresql/azuresqlfailovergroup/azuresqlfailovergroup_reconcile.go
Outdated
Show resolved
Hide resolved
- Fix bug preventing reconciliation of updates after a FailoverGroup was created. - Fix bug where status of long running operation was not properly monitored.
matthchr
force-pushed
the
feature/azure-sql-failover-group
branch
from
ce6491b to
2c418cc
Compare
Closes #1297
What this PR does / why we need it:
This could cause intermittent errors of the form: "The Failover Group 'fg-test' is busy with another operation and cannot
perform 'update Failover Group' operation. Please try again later."
Status.Messagefield if the KeyVault was not accessible.How does this PR make you feel:
If applicable: