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

Support setting proxy url in client opts #453

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

sboschman
Copy link
Contributor

prerequisite to support rancher/terraform-provider-rancher2#982

@kkaempf
Copy link

kkaempf commented Dec 11, 2023

Hmm, this disables ProxyFromEnvironment

@sboschman what's wrong with passing the proxy via the environment ?

@sboschman
Copy link
Contributor Author

sboschman commented Dec 11, 2023

Hmm, this disables ProxyFromEnvironment

The idea is to use ProxyFromEnvironment as default else block, unless a proxy url is explicitly set in the ClientOpts.

The problem with passing the proxy via the env is described in the linked issue. It sets the same proxy for every Terraform provider as the env is picked up by golang internally. If the proxy is only for the Rancher provider, but not for any other provider used in the same terraform run you are stuck.

@snasovich snasovich requested a review from a team December 11, 2023 23:53
@snasovich
Copy link
Contributor

Since the change is in Norman, assigning team responsible for this component instead. FYI @cbron @samjustus

@snasovich snasovich removed their request for review December 11, 2023 23:53
Copy link

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

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

One small nitpick, but outside of that LGTM. Keep in mind that we will hold off on merging this until we have a second approval as well.

clientbase/common.go Outdated Show resolved Hide resolved
clientbase/common.go Outdated Show resolved Hide resolved
@sboschman sboschman requested a review from KevinJoiner January 24, 2024 15:53
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.

6 participants