Skip to content
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

Don't attempt to delete default users on replica #360

Merged

Conversation

selmanj
Copy link
Contributor

@selmanj selmanj commented Aug 25, 2017

Fixes #347

Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this have any tests that failed before the change? If not, can we concoct a case that fails without the change?

@selmanj
Copy link
Contributor Author

selmanj commented Aug 25, 2017

It did not. I was going to write a test but thought that the additional test might not be worth it since it's a failure situation that users shouldn't run into anymore (and each test has a cost, after all; in this case it's in the minutes).

However I think there's something to be said about someone who wants their own root@'%' user that's managed within terraform; if they end up adding replicas later, they'll end up hitting this same path. In that case it probably makes sense to have this failure scenario covered.

@selmanj
Copy link
Contributor Author

selmanj commented Aug 28, 2017

I added a test ensuring we don't delete a root user found on the replica. It currently fails on master and passes on this branch.

The test itself is not great to read; would love input on how to make this more readable using the resource.TestCase/resource.TestStep structs.

Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@selmanj selmanj merged commit 84fa7cc into hashicorp:master Sep 7, 2017
@ryan-mf
Copy link

ryan-mf commented Sep 19, 2017

This was merged into master 12 days ago, but terraform-provider-google hasn't had a release since 1.3 on 8/17/2017:
https://github.com/terraform-providers/terraform-provider-google/releases

Now that Terraform is modular, we have to wait for the module to get its own release. Any idea when there will be a new release for terraform-provider-google?

@selmanj
Copy link
Contributor Author

selmanj commented Sep 20, 2017

I agree @ryan-mf; we're focusing on that now (we are working on making our underlying test suite happy). Should be very soon...

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
* Don't attempt to delete default users on replica

* Test that we don't attempt to delete root user on replica
luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
@ghost
Copy link

ghost commented Mar 31, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Google Cloud SQL failover replica resource shouldn't try to delete root user
3 participants