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

Use standard AWS environment variables #851

Merged
merged 5 commits into from
Jan 25, 2015
Merged

Conversation

sethvargo
Copy link
Contributor

Fixes #850, but maintains backwards compatibility.

/cc @mitchellh @svanharmelen

@svanharmelen
Copy link
Contributor

Following the preferred/standard AWS naming would (IMHO) indeed make sense. But personally I would not make the MultiEnvDefaultFunc part of helper/schema, but make it an AWS specific func only available in the AWS provider.

Allowing/supporting both variable names should be a temporary thing if you ask me. So maybe deprecate the old names for a few versions and with the 0.4.0 release (for example) only allow the new names and cleanup this temp fix/workaround?

My 2cts...

@sethvargo
Copy link
Contributor Author

@svanharmelen that was my original intention. You can't see it from the git history, but the original prototype was to add a private func to aws/provider.go that delegated to the existing EnvDefaultFunc, but supported multiple values. But then I looked at the code more...

As a general rule, it looks like Terraform follows the <provider>_<key_name> syntax for all of its environment variables (AWS_ACCESS_KEY, etc), and I wasn't sure about breaking that norm. Technically, either approach is fine, but I'm not sure what the UX should be... I thought it was weird that the envvars were not matching the "normal ones", but a different user could find it weird that the AWS environment variables do not map 1-1 with what is used inside a .tf file for example.

What do you think? I'm more than happy to move it back to a private func on the AWS provider 😄

@svanharmelen
Copy link
Contributor

Well... Personally I don't mind that much which name to use for the vars, as they both seem to have a valid thought behind them. The current one being inline with the others within TF, the latter one being inline with the AWS standards.

So I think maybe @armon or @mitchellh (who once choose the var names for this provider) are better candidates to have an opinion about what envvar names to use here.

But either way I do think that whatever the name should be, we should only one support 1 name for a given envvar and not multiple as it will only confuse things and make support cases more unclear.

So if the naming should change to the AWS standard names, then indeed I would prefer it to be a temp (for a few versions) private func inside the AWS provider...

@armon
Copy link
Member

armon commented Jan 23, 2015

I agree. Let's do it a temporary backwards compatible fix, but eventually deprecate. I do think MultiEnvDefaultFunc maybe useful in other places for similar transitions, so the helper.Schema seems fine to me.

sethvargo added a commit that referenced this pull request Jan 25, 2015
Use standard AWS environment variables
@sethvargo sethvargo merged commit 918ba4c into master Jan 25, 2015
@sethvargo
Copy link
Contributor Author

Created #866 to track the deprecation.

@sethvargo sethvargo deleted the sethvargo/aws_envvars branch January 25, 2015 18:29
@sethvargo sethvargo mentioned this pull request Jan 28, 2015
5 tasks
keymon added a commit to alphagov/paas-alpha-tsuru-terraform that referenced this pull request Jun 10, 2015
Terraform [now supports get the AWS credentials 
from the standard environment variables](hashicorp/terraform#851).
To make it work, you should not specify 'access_key' or
'secret_key' in the provider definition. 
We also need to remove the variables as we don't use them.
@ghost
Copy link

ghost commented May 4, 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 May 4, 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.

Does not respect "standard" AWS environment variables
3 participants