Skip to content

Conversation

@seratch
Copy link
Contributor

@seratch seratch commented Sep 23, 2020

Summary

As reported at #820 #821 , the current implementation of WebClient/WebhookClient when enabling proxy option started failing to send requests to the Slack API server. The implementation introduced by #715 has been working with the server-side for three months but it doesn't work anymore.

Other HTTP clients don't fail and the changes on the server-side look valid to me. I found that the implementation can be corrected in the way this PR does.

This issue is affecting some users in production. So, I will quickly release a new patch version after verifying the CI builds and running all the integration tests.

Then, the affected users can deal with this issue by either of:

  • Upgrade to the next patch version (2.9.1)
  • Use HTTPS_PROXY environment variable over the proxy option in WebClient / WebhookClient

Category (place an x in each of the [ ])

  • slack.web.WebClient (Web API client)
  • slack.webhook.WebhookClient (Incoming Webhook, response_url sender)
  • slack.web.classes (UI component builders)
  • slack.rtm.RTMClient (RTM client)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python setup.py validate after making the changes.

@seratch seratch added Version: 2x bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented web-client labels Sep 23, 2020
@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #822 into main will decrease coverage by 0.03%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #822      +/-   ##
==========================================
- Coverage   87.31%   87.28%   -0.04%     
==========================================
  Files          30       30              
  Lines        3752     3758       +6     
  Branches      334      337       +3     
==========================================
+ Hits         3276     3280       +4     
  Misses        329      329              
- Partials      147      149       +2     
Impacted Files Coverage Δ
slack/web/base_client.py 80.35% <62.50%> (+0.17%) ⬆️
slack/webhook/client.py 88.70% <80.00%> (-2.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c723633...70157a2. Read the comment docs.

@seratch
Copy link
Contributor Author

seratch commented Sep 23, 2020

Also, I've verified all the integration tests pass with this change.

(env_3.8.5) $ python setup.py run_integration_tests
running run_integration_tests
Running pytest…
/Users/ksera/github/python-slackclient/env_3.8.5/bin/python -m pytest --cov-report=xml --cov=/Users/ksera/github/python-slackclient/slack integration_tests/
=========================================================================================== test session starts ============================================================================================
platform darwin -- Python 3.8.5, pytest-5.4.3, py-1.9.0, pluggy-0.13.1
rootdir: /Users/ksera/github/python-slackclient, inifile: pytest.ini
plugins: cov-2.10.1, asyncio-0.14.0
collected 88 items                                                                                                                                                                                         

integration_tests/rtm/test_issue_530.py ..                                                                                                                                                           [  2%]
integration_tests/rtm/test_issue_558.py s                                                                                                                                                            [  3%]
integration_tests/rtm/test_issue_569.py s.                                                                                                                                                           [  5%]
integration_tests/rtm/test_issue_605.py s                                                                                                                                                            [  6%]
integration_tests/rtm/test_issue_611.py s                                                                                                                                                            [  7%]
integration_tests/rtm/test_issue_631.py ss                                                                                                                                                           [ 10%]
integration_tests/rtm/test_issue_701.py ss                                                                                                                                                           [ 12%]
integration_tests/rtm/test_rtm_client.py ..                                                                                                                                                          [ 14%]
integration_tests/web/test_admin_conversations.py ..                                                                                                                                                 [ 17%]
integration_tests/web/test_admin_conversations_restrictAccess.py ..                                                                                                                                  [ 19%]
integration_tests/web/test_admin_usergroups.py ..                                                                                                                                                    [ 21%]
integration_tests/web/test_async_web_client.py ........                                                                                                                                              [ 30%]
integration_tests/web/test_calls.py ..                                                                                                                                                               [ 32%]
integration_tests/web/test_issue_378.py ..                                                                                                                                                           [ 35%]
integration_tests/web/test_issue_480.py ....                                                                                                                                                         [ 39%]
integration_tests/web/test_issue_560.py ....                                                                                                                                                         [ 44%]
integration_tests/web/test_issue_594.py ..                                                                                                                                                           [ 46%]
integration_tests/web/test_issue_654.py ..                                                                                                                                                           [ 48%]
integration_tests/web/test_issue_670.py ..                                                                                                                                                           [ 51%]
integration_tests/web/test_issue_672.py ....                                                                                                                                                         [ 55%]
integration_tests/web/test_issue_677.py ..                                                                                                                                                           [ 57%]
integration_tests/web/test_issue_714.py ..                                                                                                                                                           [ 60%]
integration_tests/web/test_issue_728.py ..                                                                                                                                                           [ 62%]
integration_tests/web/test_issue_770.py ..                                                                                                                                                           [ 64%]
integration_tests/web/test_issue_809.py ..                                                                                                                                                           [ 67%]
integration_tests/web/test_remote_file_replacement.py .                                                                                                                                              [ 68%]
integration_tests/web/test_web_client.py .........s........                                                                                                                                          [ 88%]
integration_tests/webhook/test_async_webhook.py .....                                                                                                                                                [ 94%]
integration_tests/webhook/test_webhook.py .....                                                                                                                                                      [100%]

============================================================================================= warnings summary =============================================================================================
integration_tests/web/test_issue_480.py::TestWebClient::test_issue_480_processes_async
  /Users/ksera/.pyenv/versions/3.8.5/lib/python3.8/asyncio/base_events.py:641: RuntimeWarning: coroutine 'RTMClient._dispatch_event' was never awaited
    self._ready.clear()

-- Docs: https://docs.pytest.org/en/latest/warnings.html

---------- coverage: platform darwin, python 3.8.5-final-0 -----------
Coverage XML written to file coverage.xml

=========================================================================== 79 passed, 9 skipped, 1 warning in 328.54s (0:05:28) ===========================================================================

In addition to that, I've verified the sample using proxy works now (I fails without this change).

(env_3.8.5) $ python setup.py run_integration_tests --test-targe=integration_tests/samples/readme/proxy.py 
running run_integration_tests
Running pytest…
/Users/ksera/github/python-slackclient/env_3.8.5/bin/python -m pytest --cov-report=xml --cov=/Users/ksera/github/python-slackclient/slack integration_tests/samples/readme/proxy.py
=========================================================================================== test session starts ============================================================================================
platform darwin -- Python 3.8.5, pytest-5.4.3, py-1.9.0, pluggy-0.13.1
rootdir: /Users/ksera/github/python-slackclient, inifile: pytest.ini
plugins: cov-2.10.1, asyncio-0.14.0
collected 0 items                                                                                                                                                                                          


---------- coverage: platform darwin, python 3.8.5-final-0 -----------
Coverage XML written to file coverage.xml

========================================================================================== no tests ran in 1.62s ===========================================================================================

@seratch seratch added this to the 2.9.1 milestone Sep 23, 2020
@seratch seratch merged commit eb7b33e into slackapi:main Sep 23, 2020
@seratch seratch deleted the fix-proxy-issue branch September 23, 2020 09:07
seratch added a commit that referenced this pull request Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 2x web-client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant