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 ca_path attribute to provider configuration #80

Merged

Conversation

remilapeyre
Copy link
Collaborator

Close #79

@remilapeyre remilapeyre force-pushed the add-ca-path-in-provider-configuration branch from 1e40488 to 4a1fd33 Compare December 21, 2018 13:10
@eedwards-sk
Copy link

Thanks for the PR for #79!

I noticed you added CONSUL_CAPATH env var support, as well.

I noticed that a few of the env vars supported in resource_provider.go do not match those supported by the consul cli, and some are missing entirely (e.g. CONSUL_HTTP_SSL_VERIFY).

Is that intentional?

Since the provider documentation references the above consul cli docs page, I assume there is some intent of parity.

@remilapeyre
Copy link
Collaborator Author

Thanks for the feedback!

I noticed that a few of the env vars supported in resource_provider.go do not match those supported by the consul cli, and some are missing entirely (e.g. CONSUL_HTTP_SSL_VERIFY).

I think many environment variables are already set when instantiating the client, for example CONSUL_HTTP_SSL_VERIFY is used here https://github.com/terraform-providers/terraform-provider-consul/blob/1a36214955ccd62b0750154aec46849a58af3df9/vendor/github.com/hashicorp/consul/api/api.go#L354-L362 wich is called by resource_provider.providerConfigure.

Did you try to set CONSUL_HTTP_SSL_VERIFY and it did not work?

Since the provider documentation references the above consul cli docs page, I assume there is some intent of parity.

I think so too, must would expect the behavior to be the same and it could be suprising if not for someone whose environment variables are set in bashrc for example. I you find some environment variables for which the behavior differ, please tell me and I will add support for them.

Copy link
Contributor

@pearkes pearkes left a comment

Choose a reason for hiding this comment

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

This looks good to me. I agree we should mirror the env vars as defaults but also allow them to be set like this -- this PR in that regard looks good.

Looking at some of our other options, it seems like we support different variables then the client package will "magically" support.

@remilapeyre remilapeyre merged commit f5d980b into hashicorp:master Mar 18, 2019
remilapeyre pushed a commit to remilapeyre/terraform-provider-consul that referenced this pull request Mar 19, 2019
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