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

fix: Removing extraneous space inserted during refactoring commit 894484a #3523

Merged
merged 1 commit into from
Sep 19, 2022
Merged

fix: Removing extraneous space inserted during refactoring commit 894484a #3523

merged 1 commit into from
Sep 19, 2022

Conversation

mike-kingsbury
Copy link
Contributor

… Code was moved to a new function, and a space was added to the split. bz#2126155

All Pull Requests:

Check all that apply:

  • [ x ] Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • [ x ] Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

Reverting a change that introduced a space into a .split(','). The space broke the functionality of the Insights client when using NO_PROXY with multiple items.

@xiangce xiangce requested a review from ahitacat September 15, 2022 01:09
@xiangce
Copy link
Contributor

xiangce commented Sep 15, 2022

I'm just wondering if we could add a test case for this? It seems an issue that can be detected by a sufficient test.

@mike-kingsbury
Copy link
Contributor Author

I'm just wondering if we could add a test case for this? It seems an issue that can be detected by a sufficient test.

Give me a couple of minutes, you're correct it should be testable.

@xiangce
Copy link
Contributor

xiangce commented Sep 15, 2022

@mike-kingsbury - Thanks for the update. It looks good to me. @ahitacat, please have a look, if it is also good for you, I think we can merge and release it today.

@psachin psachin added the client These issues represent work to be done by the "client" team. label Sep 15, 2022
@ahitacat
Copy link
Contributor

In that case it would fail with no_proxy values separated by comma and space. Like the one is given in the bug report This no_proxy function get the the value from env or from rhsm. So let's try to support bot of them.

Copy link
Contributor

@ahitacat ahitacat left a comment

Choose a reason for hiding this comment

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

Also it could be useful to add a check under test_get_no_proxy_rhsm that tests when no_proxy has values separated by comma and space.

    with patch.dict(os_environ, {"HTTPS_PROXY": "env.proxy.example.com", "NO_PROXY": "example.com, redhat.com"}, clear=True):
        connection.get_proxies()
    assert connection.proxies == {'https': 'env.proxy.example.com'}

insights/client/connection.py Outdated Show resolved Hide resolved
… Code was moved to a new function, and a space was added to the split. bz#2126155

Signed-off-by: Mike Kingsbury <[email protected]>
Copy link
Contributor

@ahitacat ahitacat left a comment

Choose a reason for hiding this comment

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

This changes looks great and it can be merged. Thanks @mike-kingsbury for open and fix the bug.

@xiangce xiangce merged commit e942d79 into RedHatInsights:master Sep 19, 2022
@xiangce xiangce changed the title Removing extraneous space inserted during refactoring commit 894484a.… fix: Removing extraneous space inserted during refactoring commit 894484a Sep 19, 2022
xiangce pushed a commit that referenced this pull request Sep 19, 2022
- Removing extraneous space inserted during refactoring commit 894484a
- Code was moved to a new function, and a space was added to the split.  bz#2126155

Signed-off-by: Mike Kingsbury <[email protected]>
(cherry picked from commit e942d79)
xiangce pushed a commit that referenced this pull request Sep 6, 2024
- Removing extraneous space inserted during refactoring commit 894484a 
- Code was moved to a new function, and a space was added to the split.  bz#2126155 

Signed-off-by: Mike Kingsbury <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client These issues represent work to be done by the "client" team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants