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

Added support for VAULT_PROXY_ADDR + Updated docs #15377

Merged
merged 11 commits into from
May 24, 2022
Merged

Added support for VAULT_PROXY_ADDR + Updated docs #15377

merged 11 commits into from
May 24, 2022

Conversation

peteski22
Copy link
Contributor

Updated documentation to describe the behavior when supplying VAULT_HTTP_PROXY.
Also added support for VAULT_PROXY_ADDR as a 'better name' for VAULT_HTTP_PROXY.

Updated documentation to describe the behavior when supplying `VAULT_HTTP_PROXY`. Also added support for `VAULT_PROXY_ADDR` as a 'better name' for `VAULT_HTTP_PROXY`.
Change is in addition to existing change
Related to GH issue raised regarding behavior for Vault proxy env var.
@peteski22 peteski22 marked this pull request as ready for review May 11, 2022 20:29
Copy link
Contributor

@taoism4504 taoism4504 left a comment

Choose a reason for hiding this comment

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

minor nits; but LGTM. I'm not sure if you require other reviewers since there seems to be new code introduced.

peteski22 and others added 2 commits May 16, 2022 09:03
Co-authored-by: Loann Le <84412881+taoism4504@users.noreply.github.com>
Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

I'm not 100% convinced that the better name is worth having two different ways to do something and the potential confusion that introduces. That said, I like the tests and the docs, and my minor misgivings aren't enough for me to reject this. Perhaps in time we could retire the old name.

api/client_test.go Outdated Show resolved Hide resolved
api/client_test.go Outdated Show resolved Hide resolved
As raised in a PR comment, we don't need to call ReadEnvironment() as the DefaultConfig() does that for us. We can just check the config.Error for the error.
api/client_test.go Outdated Show resolved Hide resolved
api/client_test.go Outdated Show resolved Hide resolved
api/client_test.go Outdated Show resolved Hide resolved
@peteski22
Copy link
Contributor Author

I'm not 100% convinced that the better name is worth having two different ways to do something and the potential confusion that introduces. That said, I like the tests and the docs, and my minor misgivings aren't enough for me to reject this. Perhaps in time we could retire the old name.

Yes, ideally we could get rid of the VAULT_HTTP_PROXY at some point. 👍🏼

@ncabatoff ncabatoff merged commit 338fbea into hashicorp:main May 24, 2022
@peteski22 peteski22 deleted the fix/vault-http-proxy-docs branch May 24, 2022 20:34
Gabrielopesantos pushed a commit to Gabrielopesantos/vault that referenced this pull request Jun 6, 2022
Updated documentation to describe the behavior when supplying `VAULT_HTTP_PROXY`. Also added support for `VAULT_PROXY_ADDR` as a 'better name' for `VAULT_HTTP_PROXY`.
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.

3 participants