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

Add Region to Provider #25

Merged

Conversation

jtopjian
Copy link
Contributor

@jtopjian jtopjian commented Jun 14, 2017

This commit adds the region argument to the OpenStack provider.
This enables a user to specify the region once at the provider-level
and have all resources within that provider be built in that region.

If a resource-level region was specified, it is still recognized
and will override the provider-level region.

Fixes #17

This commit adds the region argument to the OpenStack provider.
This enables a user to specify the region once at the provider-level
and have all resources within that provider be built in that region.

If a resource-level region was specified, it is still recognized
and will override the provider-level region.
@jtopjian jtopjian requested a review from fatmcgav June 14, 2017 03:21
@jtopjian jtopjian changed the title Add Region to Provider [WIP] Add Region to Provider Jun 14, 2017
@jtopjian jtopjian changed the title [WIP] Add Region to Provider Add Region to Provider Jun 14, 2017
@jtopjian
Copy link
Contributor Author

jtopjian commented Jun 14, 2017

I've finally had a chance to test this out in an environment with a multi-region catalog and I'm happy with how it works. It's quite safe and I was unable to accidentally destroy resources.

It's still possible there are edge cases here, so there will be a warning in the CHANGELOG to do a plan before applying.

@fatmcgav Let me know if you see any problems with this.

@mcanevet I found some resources that weren't setting the region in their state, notably the openstack_compute_instance_v2 resource. If you have a moment, could you test this with the Terraform 0.10.0-dev build?

If neither of you see any issues with this, I'll go ahead and merge it. It's a big change, so I want to get this one in first in order to avoid a lot of rebasing / merge conflicts :)

@mcanevet
Copy link
Contributor

@jtopjian I'll not have time to port all my custom providers to 0.10.0-dev in the nexts few days, but IIRC I add to add region in lifecycle's ignore_changes with your code because of the lack of region in the state. But I think it's a bug with the way it was done before, not the way you're doing it now so I can personally deal with it.
Thanks for your great work!

// GetRegion returns the region from either d.Get("region") or OS_REGION_NAME
func GetRegion(d *schema.ResourceData) string {
// GetRegion returns the region from either d.Get("region") or config.Region
// which is either specified in the config or pulled from OS_REGION_NAME.
Copy link
Contributor

Choose a reason for hiding this comment

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

@jtopjian Is the reference to OS_REGION_NAME still accurate here?

Is which is either specified in the resource config or pulled from provider config. more accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll clean this up.

@@ -135,44 +137,55 @@ func (c *Config) loadAndValidate() error {
return nil
}

func (c *Config) determineRegion(region string) string {
// If a resource-level region was not specified, and a provider-level region was set,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to get an 'empty' region in here?

Do we need to check for that condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still possible to not set a region at all. I've tested this and it works as expected. I'll update the docs to mention no region is possible for single-region catalogs.

@fatmcgav
Copy link
Contributor

@jtopjian Had a quick run through the code and a couple of comments...

I'll try and get a 0.10-dev build done later to day and test in anger :)

Copy link
Contributor

@fatmcgav fatmcgav left a comment

Choose a reason for hiding this comment

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

New docs look good. I haven't yet had chance to test in anger though...

@jtopjian
Copy link
Contributor Author

jtopjian commented Jun 14, 2017

No worries - take your time.

Building the new release isn't difficult, just a few new steps. Just do a make dev on hashicorp/terraform and make build on this branch. Then you'll have to do a terraform init before running plan/apply.

@fatmcgav
Copy link
Contributor

@jtopjian I'm not sure if I'm missing something, but I've tried to run the 0.10.0-dev, and I can't get it to pick up the provider... :(

I can see the terraform-provider-openstack bin in $GOPATH/bin, but terraform init doesn't seem to be looking there...
I've also tried putting it in my .terraformrc and that doesn't help either...

Any pointers?

Cheers

@jtopjian
Copy link
Contributor Author

@fatmcgav are you sure you're running ~/go/bin/terraform init and not terraform located in another part of path? (which terraform)

@fatmcgav
Copy link
Contributor

@jtopjian Ah, good shout... I was running from ~/golang/src/github.com/hashicorp/terraform/bin/terraform rather than $GOROOT/bin/terraform.

So I'm guessing it's looking for provider plugins in the same dir as the terraform bin, which kinda makes sense now :)

@fatmcgav
Copy link
Contributor

So I've had a quick test with various combinations, and they all seem to work for me :)

I've tried:

  • Region specified on resource
  • Region specified on provider
  • Region specified using OS_REGION_NAME

Haven't yet had chance to try a multi-region plan, but that'll take me a bit of time to get some test configs written up...

Happy for this to be merged though... :)

@jtopjian
Copy link
Contributor Author

👍

Awesome.

I've also done a good amount of testing with pre-0.10.0 state files and things worked well.

Merging now.

@mcanevet @fatmcgav Thank you for your help reviewing this :)

@jtopjian jtopjian merged commit cc222c2 into terraform-provider-openstack:master Jun 15, 2017
gator1 referenced this pull request in gator1/terraform-provider-opentelekomcloud Nov 27, 2017
@jtopjian jtopjian deleted the provider-region branch October 22, 2018 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants