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

Respect the PROXY_ENABLED environment variable #146

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

mssola
Copy link
Contributor

@mssola mssola commented Sep 5, 2022

Description

This is a special environment variable set in SUSE systems and that is managed by tools like YAST. The usage of this environment variable is not standard and hence the net/http/httputil library does not support it.

Therefore, this commit adds an extra check for this environment variable when setting up the HTTP client.

See SUSE/connect@b2758ad

How to test it

  1. Build with this patch applied.
  2. Check that the environment variable PROXY_ENABLED is respected. That is, even if HTTP_PROXY has been set, setting PROXY_ENABLED to false will make SUSEConnect to ignore the setup.
  3. Note that not setting it at all (e.g. leave a blank value or not setting it at all) will tell SUSEConnect to go on as usual.

@mssola mssola requested review from skazi0 and djoreilly and removed request for skazi0 September 5, 2022 08:36
@mssola
Copy link
Contributor Author

mssola commented Sep 5, 2022

skazi0
skazi0 previously approved these changes Sep 5, 2022
Copy link
Contributor

@skazi0 skazi0 left a comment

Choose a reason for hiding this comment

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

lgtm

This is a special environment variable set in SUSE systems and that is
managed by tools like YAST. The usage of this environment variable is
not standard and hence the net/http/httputil library does not support
it.

Therefore, this commit adds an extra check for this environment variable
when setting up the HTTP client.

See SUSE/connect@b2758ad

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
@mssola
Copy link
Contributor Author

mssola commented Sep 5, 2022

@skazi0 @djoreilly note that I have basically reverted to the original code from this PR. That is, we don't use ParseBool and instead just do some basic comparisons. I've done this because ParseBool is not being too convenient on this case. Notice that the /etc/sysconfig/proxy files use a yesno notation, so this falls out of the ParseBool scope. I have also tried to use it eitherway as a final check, but the code was becoming more cumbersome than really needed. Thus, I have kept it simple and resorted to a much simpler implementation.

Copy link
Contributor

@skazi0 skazi0 left a comment

Choose a reason for hiding this comment

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

lgtm

@mssola mssola merged commit de887da into main Sep 5, 2022
@mssola mssola deleted the add-proxy-enabled-support branch September 5, 2022 12:02
if !proxyEnabled() {
return nil, nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO something like this would be easier to understand

       if strings.ToLower(strings.TrimSpace(os.Getenv("PROXY_ENABLED"))) == "no" {
               return nil, nil
       }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that it can also be "n". And users might have mistakenly set "false" or "f", and I don't think is unreasonable to support it (plus, in SUSEConnect ruby we also allow all this as you can see on the referenced commit). With this in mind, the comparison is not that easy and putting it in as a separate function reads well while allowing to hide how exactly we determine all this from the caller perspective.

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.

None yet

3 participants