-
Notifications
You must be signed in to change notification settings - Fork 1.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
[compute_instance] - Allow updating of network and subnetwork properties #4011
[compute_instance] - Allow updating of network and subnetwork properties #4011
Conversation
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148164" |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148166" |
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.
Here's some initial comments from a first pass. Also, watch out for the release note- we usually put just the product in the beginning (so "compute" in this case), and then the name of the resource somewhere in the body of the note.
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_compute_instance_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_compute_instance_test.go.erb
Outdated
Show resolved
Hide resolved
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.
Responding to comments
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148326" |
third_party/terraform/tests/resource_compute_instance_test.go.erb
Outdated
Show resolved
Hide resolved
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148326" |
1 similar comment
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148326" |
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.
A few more small comments, nothing too major. I'm curious about the answer to @drebes's question as well.
This change is also making me think that we should prioritize the new instances.update method a bit higher. The update function in this resource is suuuuper long, and I'm nervous there are combinations of updates that we're not catching.
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_compute_instance_test.go.erb
Outdated
Show resolved
Hide resolved
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148423" |
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.
Responding to comments.
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148424" |
Removing myself as a reviewer since Dana reviewed this- feel free to add me back if you'd like me to make a pass, though. |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148439" |
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.
Definitely didn't expect this to be so complex when you started out. My main concern is about adding some documentation about how we are approaching update behavior. The rest are just soft suggestions
third_party/terraform/tests/resource_compute_instance_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_compute_instance.go.erb
Outdated
Show resolved
Hide resolved
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148526" |
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.
Once Cameron's suggestions are resolved, I'm satisfied. I went ahead and retried the VCR tests since TeamCity ran out of memory on the last run- I assume you've run the new test and I don't expect this to affect existing tests, so if it has problems again I don't think it's worth blocking on.
Oh also I went ahead and edited the changelog note to match the guidelines- take a look at the edits I made so you can better understand what I mean by that :) |
typo correction Co-authored-by: Cameron Thornton <[email protected]>
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148526" |
typo correction Co-authored-by: Cameron Thornton <[email protected]>
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148526" |
1 similar comment
…m/ScottSuarez/magic-modules into scsuarez/update-network-interface
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148526" |
Allow updating of network and subnetwork properties on resource_compute_instance.
Some issues of 7118 are fixed. Still need to add support for updating network_ip during subnetwork change. This would require utilizing a customized diff.
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)