-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_postgres_server: support for replicaset scaling #10754
azurerm_postgres_server: support for replicaset scaling #10754
Conversation
9ead6e0
to
2879ac3
Compare
I ran the tests that @aristosvo created, and the tests are failing for me. I tried debugging the failures and some other tests in the TestAccPostgreSQLServer also failed for me. I'm getting mixed results. Not sure if the issue is on my side or in the TF provider. Full results of my tests below:
|
52e31a4
to
5c68e6e
Compare
The acctest for the new functionality has succeeded anyway with a few minor updates: ❯ TF_ACC=1 go test -v ./azurerm/internal/services/postgres/ -run=TestAccPostgreSQLServer_scaleReplica -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/02/28 20:25:44 [DEBUG] not using binary driver name, it's no longer needed
2021/02/28 20:25:45 [DEBUG] not using binary driver name, it's no longer needed
=== RUN TestAccPostgreSQLServer_scaleReplica
=== PAUSE TestAccPostgreSQLServer_scaleReplica
=== CONT TestAccPostgreSQLServer_scaleReplica
--- PASS: TestAccPostgreSQLServer_scaleReplica (1320.66s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/postgres 1324.093s |
0671126
to
e837b66
Compare
e837b66
to
bbd0a78
Compare
Test update to reuse test code, createReplica test works again:
|
9d961ba
to
d6f8908
Compare
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.
@aristosvo, thanks for this PR and it's looking really good. I left a very minor comment I would like updated if you don't mind and if you could add some documentation that would be great! 🚀
if indexOfSku(old) < indexOfSku(new) { | ||
listReplicas, err := replicasClient.ListByServer(ctx, id.ResourceGroup, id.Name) | ||
if err != nil { | ||
return fmt.Errorf("request error for list of replicas for PostgreSQL Server %q (Resource Group %q): %+v", id.Name, id.ResourceGroup, err) |
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.
return fmt.Errorf("request error for list of replicas for PostgreSQL Server %q (Resource Group %q): %+v", id.Name, id.ResourceGroup, err) | |
return fmt.Errorf("listing replicas for PostgreSQL Server %q (Resource Group %q): %+v", id.Name, id.ResourceGroup, err) |
@aristosvo, I am still seeing acceptance tests failing. Also, could we get a test with more than one replica server in it as well? This is so close, just have a few little tweaks to work out and I think we could get this in the next release. 🚀 |
@aristosvo, I figured out why a lot of test cases were failing. There appears to be a bug in the RP where it will return the
Then at the very bottom of that same file and this code to expose the actual state wait function:
You will also have to add the follow to your import statement at the top of the file:
Totally not your fault, or your codes fault, but since you're in there might as well fix this too. 🙂 |
@WodansSon Thanks for your comments! I believe I implemented them all, added a small note in the documentation about the functionality. AccTest for multiple replicas passed:
|
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.
@aristosvo, this is looking good and you implemented the suggestions perfectly, that you so much for pushing the updates. I have one last question, in the note you added to the documentation it looks like there maybe a typo, so I wanted to check with you if it was intended or not. Other than that the LGTM! 🚀
I ran the tests and there is currently only one test case that still fails and that is because it's a bad test. If you are up to it you can fix that to in this PR and then all of the server tests will pass with a 100% pass rate! I investigated the
Then you will need to fix the configuration file
Once you do that all of the server tests will pass again! Thanks for your help! |
Co-authored-by: WS <[email protected]>
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.
@aristosvo, thanks for pushing those changes this LGTM now! 🚀
@WodansSon Thanks!🎉💃🏼🍻 I planned to include the final test fix, but didn't have a keyboard at hand when I saw it 😂 Saw your other PR for it, anyway👍👍 |
This has been released in version 2.51.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.51.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Fixes #10284
Implementation