-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Read more default envvars for GCP #6114
Conversation
sethvargo
commented
Apr 10, 2016
- Closes Change to defaults for gce #5874
- Fixes Align Terraform environment variables with GCE defaults #5872
e59d4a5
to
ac46535
Compare
} | ||
|
||
if v := os.Getenv("GOOGLE_REGION"); v != "us-central1" { | ||
t.Fatal("GOOGLE_REGION must be set to us-central1 for acceptance tests") | ||
projs := []string{ |
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.
what is the difference between projs
& creds
? can't we set a global constant for this?
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.
Hey @stack72
- creds - places to look for credentials json
- projs - places to look for project
- regs - places to look for regions
We could make this a constant and share it between the provider and provider_test, but that wasn't a pattern I saw elsewhere in the codebase and didn't want to introduce a new paradigm. This is just the test file searching for the place to pull in creds from, and it arguably could remain unchanged.
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.
Makes sense not to break the formulae :)
ac46535
to
337895b
Compare
can also be specified with the `GOOGLE_CREDENTIALS` or `GOOGLE_CLOUD_KEYFILE_JSON` | ||
shell environment variable, containing the contents of the credentials file. | ||
can also be specified using any of the following environment variables | ||
(listed in order of precedence): |
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.
Could you point to the docs where you found these requirements here? It will make it clearer why Terraform allows you to configure your credentials so many different ways.
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.
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.
Ah, darn, oh well. I guess adding those would just make it more confusing.
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.
Yea, (other cloud) had this problem and eventually someone made an authoritative decision and said "these are the envvars, everything else is wrong!" 😄
LGTM after comment |
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. |