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

AWS_PROFILE support #7

Closed
wants to merge 10 commits into from
Closed

AWS_PROFILE support #7

wants to merge 10 commits into from

Conversation

asafdav
Copy link

@asafdav asafdav commented Jul 13, 2017

First of all, thanks for this amazing terraform provider!
In our use case, we have different credstash instances spread over different accounts, I had to add profiles support and I think that other people can benefit from that.

It should solve #3

Copy link

@tmichel tmichel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It looks good.

Please fix the build. It seems that something got mixed up and the compilation fails.
Also please squash your commits into a single one with a descriptive commit message. Thanks!

"profile": {
Type: schema.TypeString,
Required: true,
DefaultFunc: func() (interface{}, error) {
Copy link

Choose a reason for hiding this comment

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

If this should come from AWS_PROFILE environment variable that please use the schema.MultiEnvDefaultFunc([]string{"AWS_PROFILE"}, "default") default value provider so that environment variable is picked up by default.

Copy link
Author

Choose a reason for hiding this comment

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

If you set AWS_PROFILE environment variable it drives Terraform itself nuts + I think it's just nicer to set the provider as:

provider "credstash" {
  table  = "some-table"
  region = "some-region"
  profile = "some-profile" 
}

@asafdav
Copy link
Author

asafdav commented Jul 16, 2017

@tmichel thanks for the quick workaround.

  1. I think that the build fails because of this import https://github.com/asafdav/terraform-provider-credstash/blob/e16edd4ae6d111053cb6ff749982d82a71052edd/datasource_secrets.go#L10
    instead of fetching my updated version of "credstash" it takes your original one, I forked the entire project and changed it to asafdav/* and it worked
  2. I can squash my commits but it's not needed you can squash the PR when you merge it using Github, isn't it cool ?
    screen shot 2017-07-16 at 14 32 26

@tmichel
Copy link

tmichel commented Jul 22, 2017

Cleaned up the code (gofmt) and pushed to master: 2433b14.

Thanks again!

@tmichel tmichel closed this Jul 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants