-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Migrate state for google_bigtable_instance #4702
Migrate state for google_bigtable_instance #4702
Conversation
@rileykarson It seems that this is no longer required to fix #4701, but do you think it's still worthy of keeping around? I'm kind of on the fence TBH. The original bug is fixed, but these changes still do a few (maybe useful) things:
I will say though that most of my testing has been with Terraform 0.11, and it seems like it's possible that Terraform 0.12 somewhat obviates the need for this due to the new state structure? I defer to your knowledge here. 🙂 |
I don't think we should perform a state migration we don't need to, and I'd rather close this out I think. They're necessary in some cases, but finicky otherwise and can leave users' states in weird places. Additionally, TPG/TPGB For point 3, |
That all makes sense to me. Thanks for taking a look at all this, and for all the additional info! =] |
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
This PR fixes #4701. I accomplished this by providing a
MigrateState
function forgoogle_bigtable_instance
to handle the discrepancy described in the issue.I added a slew of unit tests that test the pure functionality of the new
MigrateState
function (that is, without going through any RPCs); due to the nature of the bug, two different versions would be required to reproduce with acceptance tests, so I thought manually crafting state and migrating it would be more effective.In addition to these unit tests, I performed some thorough integration/acceptance testing of my own across many versions of the
google
provider, using this script. I tested with 1.19.1 (the version I'm currently using in a real project), 2.13.0-2.17.0, and the "latest", from this branch containing my fix. The Gist linked previously also contains sample output from the script (the diffs between create/migrate, and the created state for the latest version of the plugin). Everything came out as expected, with the exception that after I pulled in upstream changes the other day (specifically, #4598 / GoogleCloudPlatform/magic-modules#2409), some of my "maintain ordering" logic no longer worked; GoogleCloudPlatform/magic-modules#2489 addresses this, and the script output in my Gist was produced using those changes as well as the ones in this PR, but themagic-modules
changes have their own problems that I'm hoping a maintainer will be able to help with. (Without the changes in that PR, the diff output from my script isn't "empty" like I'd prefer, since the clusters get reordered.)(Just a note, this is the first time I've ever tried my hand at Go, so feel free to call out anything that could be improved. 🙂)
cc @rileykarson @mackenziestarr