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

Move GCP projece attribute onto resources, inherit from provider #6112

Merged
merged 5 commits into from
Apr 11, 2016

Conversation

sethvargo
Copy link
Contributor

This commit uses the existing pattern in the provider for "region", but extends it to "project" as well. Currently the provider is heavily tied to a project. Some of the upcoming APIs (that we would like to leverage) do not require a project id. This PR decouples the provider configuration from the project configuration, but does so in a backwards-compatible way.

Highlight changes:

  • provider/google.project: required => optional
  • resource/google_*: +project attribute (with some exceptions)
  • Updated all docs with the new resource

/cc @lwander @jen20

The current implementation returns error as the first parameter, 
but it is usually the last parameter.
This is the first step in removing the config dependency on "project".
This change is backwards-compatible because the value for this new
attribute defaults to the value from the provider.
This commit also normalizes the format we display attributes.
@sethvargo
Copy link
Contributor Author

One important thing to note here is the plan vs. apply validation. Because the provider is not configured during validateWalk, a config like this will have a successful plan phase even though no value was provided for "project" in the provider configuration:

provider "google" {
  credentials = "${file("creds.json")}"
}

resource "google_compute_network" "default" {
  name       = "test"
  ipv4_range = "10.0.0.0/16"
}

This is because, due to the way providers are loaded, we don't have the provider data until we start running the function. As such, the apply will quickly error out, before actually making any API calls for the resource, when trying to read the value and finding it's nil.

I think this is "ok" until we can refactor Terraform's core to load provider configs in validateWalk, and I saw this pattern already exists in a few other places in Terraform. In reality, the probability for that type of error is quite minimal, and the gained-tradeoff is that we can now manage multiple projects in a single Terraform config 😄

// getRegion reads the "region" field from the given resource data and falls
// back to the provider's value if not given. If the provider's value is not
// given, an error is returned.
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.

Thanks for moving these in in here, resource_compute_address.go definitely wasn't the place for getRegion.

@lwander
Copy link
Contributor

lwander commented Apr 11, 2016

Hey just to make sure I didn't miss anything: You'll be adding the project resource in a separate PR? Also, you mention that some resources weren't updated with the new project attribute - which ones didn't need it?

Otherwise, LGTM! Thanks for updating the docs too.

@sethvargo
Copy link
Contributor Author

Hey @lwander

Yup! This PR was already rather large, so I didn't want to add any new resources yet. I'm going to do that in another PR once this lands in core 😄

@sethvargo sethvargo merged commit 183100a into master Apr 11, 2016
@sethvargo sethvargo deleted the sethvargo/gcp_project branch April 11, 2016 15:45
@sethvargo
Copy link
Contributor Author

@lwander whoops, missed the second part of your Q:

All the storage_bucket_* resources don't have a project attribute.

@ghost
Copy link

ghost commented Apr 26, 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 26, 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.

3 participants