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.

Sign up for GitHub

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

Jump to bottom

Enable 'zone' to be specified at the provider level instead of per-resource. #816

Merged
merged 6 commits into from
Dec 6, 2017
Merged

Enable 'zone' to be specified at the provider level instead of per-resource. #816

merged 6 commits into from
Dec 6, 2017

Conversation

nat-henderson
Copy link
Contributor

Fixes #772. Tested:

22:44 $ make test
==> Checking that code complies with gofmt requirements...
go test -i $(go list ./... |grep -v 'vendor') || exit 1
echo $(go list ./... |grep -v 'vendor') | \
        xargs -t -n4 go test  -timeout=30s -parallel=4
go test -timeout=30s -parallel=4 github.com/terraform-providers/terraform-provider-google github.com/terraform-providers/terraform-provider-google/google github.com/terraform-providers/terraform-provider-google/scripts 
?       github.com/terraform-providers/terraform-provider-google        [no test files]
ok      github.com/terraform-providers/terraform-provider-google/google 0.065s
ok      github.com/terraform-providers/terraform-provider-google/scripts        0.012s

and confirmed I got all the places I needed to replace:

22:46 $ grep '.GetOk("zone")' google/*.go
google/utils.go:        res, ok := d.GetOk("zone")
22:46 $ grep '.Get("zone")' google/*.go
google/provider.go:             Zone:        d.Get("zone").(string),

(both of which are correct)

@nat-henderson
Copy link
Contributor Author

@rosbo This seems to have gone right, but as mentioned I'm not in a good state to run all the acctests. I'm running the ones I can now and I'll report back with any failures I suspect are related.

@nat-henderson
Copy link
Contributor Author

nat-henderson commented Dec 4, 2017 •
edited
Loading

Aha, there was a problem with the provider config. Hang on, I'll fix it and report back.

edit: Well, sort of a problem. More like a "choice I made without thinking about it". :) I don't think you should have to specify zone in the tests, so I want to change it from required to optional. Tests are running now and seem to be passing save for the quota issues I mentioned in #815. I'll push that commit once these tests are done.

rosbo
rosbo suggested changes Dec 5, 2017
View reviewed changes
google/utils.go
// given, an error is returned.
// given, checks for "zone" in either the given resource data or provider,
// and extracts region from zone. If "zone" is not provided, returns an
// error.
func getRegion(d *schema.ResourceData, config *Config) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth it to add a unit test for getRegion and getZone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure you're right - I'll get on that. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rosbo
Copy link
Contributor

rosbo commented Dec 5, 2017 •
edited
Loading

FYI: The CI is server is running all the acceptance tests on this branch.

rosbo
rosbo reviewed Dec 5, 2017
View reviewed changes
google/resource_compute_disk.go Outdated
if err != nil {
return handleNotFoundError(err, d, fmt.Sprintf("Disk %q", d.Get("name").(string)))
}
} else if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove this else if block. If no zone are specified, it is fine and the logic in the else block tries to guess the zone.

This comment also applies to group manager and autoscaler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this as well, sorry.

rosbo
rosbo reviewed Dec 5, 2017
View reviewed changes
google/resource_compute_disk.go Outdated
@@ -279,12 +287,14 @@ func resourceComputeDiskRead(d *schema.ResourceData, meta interface{}) error {
}

var disk *compute.Disk
if zone, ok := d.GetOk("zone"); ok {
if zone, err := getZone(d, config); zone != "" && err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the zone is empty, the err returned by getZone should not be nil and therefor, the zone != nil is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, shoot, I thought that was idiomatic because of the way that .Get() works in Schema. :) I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed that, also. Thanks!

@nat-henderson
Initial commit for provider-level zone attribute:
2c332da 
- Fetch Zone attribute any place where it *was* being fetched from the schema by
	combination schema / provider-level attribute.
-Allow region to be unspecified if zone is specified.
@nat-henderson
Switch one example to using provider-level zone as an example.
bb92780
@nat-henderson
Make provider-level zone optional.
cd2a9b9 
Individual resources will fail if they can't find a zone.
rosbo
rosbo approved these changes Dec 6, 2017
View reviewed changes
Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

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

The acceptance tests passed

@nat-henderson nat-henderson merged commit 114b646 into hashicorp:master Dec 6, 2017
@nat-henderson nat-henderson deleted the provider_level_zone branch December 6, 2017 22:30
@rosbo rosbo mentioned this pull request Jan 3, 2018
Merged
modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Sep 27, 2019
@modular-magician @chrisst
Retrieve project after detecting changes (hashicorp#816)
4a0731a 
Signed-off-by: Modular Magician <[email protected]>
@ghost
Copy link

ghost commented Mar 30, 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 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rosbo rosbo rosbo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add provider-level zone attribute
2 participants
@nat-henderson @rosbo