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

Refresh google_compute_instance machine_type on read #9320

Closed
wants to merge 1 commit into from

Conversation

bjorand
Copy link
Contributor

@bjorand bjorand commented Oct 11, 2016

I've changed a machine type manually in GCE and ran terraform refresh to update my local tfstate but machine_type was not updated. This MR set() machine_type in resourceComputeInstanceRead.

@paddycarver
Copy link
Contributor

Hey @bjorand! Really sorry for the delayed review on this. :( This looks great, and thanks for contributing it back! The only thing I'd like to see is a test that shows the current implementation breaking, which passes after the patch is applied. After the lengthy delay in reviewing, I understand if you don't have the bandwidth or interest to add it yourself at the moment; if that's the case, could you just turn on allow edits from maintainers on this PR, I'll be happy to add the test so we can get this merged in. Thanks!

@stack72
Copy link
Contributor

stack72 commented Feb 1, 2017

Fixes #11552

@bjorand
Copy link
Contributor Author

bjorand commented Feb 1, 2017

@paddyforan Do not be sorry about your reply, mine is a bit delayed too!

The only thing I'd like to see is a test that shows the current implementation breaking, which passes after the patch is applied.

Do you mean writing a new acceptance test which covers this commit ? Feel free to add this test if you want as I currently have little free time available to do so.

@paddycarver
Copy link
Contributor

Hey @bjorand! Thanks for getting back. Auspicious timing, I was just talking with the excellent @mbfrahry about this. I did mean trying to get an acceptance test that covers the bug the commit is fixing, but totally understand busy schedules. I just also didn't want to pull the Github equivalent of just taking the keyboard from you. :)

But @mbfrahry is putting the finishing touches on the test case right now, so we'll close this PR when he adds the new one (containing your commit with the fix, and pointing back to this one) and get his merged. Thank you for contributing the code, and for bringing the issue to our attention!

@mbfrahry mbfrahry mentioned this pull request Feb 2, 2017
@mbfrahry
Copy link
Member

mbfrahry commented Feb 2, 2017

Hey @bjorand, I wrote the test case and pushed it to a new PR #11645. Thanks for throwing this fix together!

@stack72
Copy link
Contributor

stack72 commented Feb 3, 2017

Closed via #11645

@stack72 stack72 closed this Feb 3, 2017
@ghost
Copy link

ghost commented Apr 17, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

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

Successfully merging this pull request may close these issues.

5 participants