-
Notifications
You must be signed in to change notification settings - Fork 149
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
204: Handle proxies when using Slack WebClient #205
Conversation
Thanks for the contribution! Before we can merge this, we need @raihle to sign the Salesforce Inc. Contributor License Agreement. |
Hi, @raihle! Thanks so much for opening this PR 🙌 This is looking good so far! I'm going to run the integration tests on my end to make sure all looks good, but it's a great sign that the unit tests are passing, especially with the updates to At first glance the two use cases you provided for the web client test make sense. I have to get access to a particular workspace to run the integration tests, so I'll drop in an update once I'm able to run these with the results as well as if I have any additional thoughts on the test cases! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raihle Great news - this is looking good on my end and I have confirmed through #206 that the integration tests pass!
This PR is currently failing the payload integration test due to a non-related issue that was uncovered where that test is having trouble parsing certain characters when they're in the body of a PR. I have a fix for that in #207. Waiting for approval for that one, and then once that has been merged in and the integration test is updated, I'll approve and merge this one in as well. Also - I have left one small comment on testing that I would like to get your thoughts on before approving and merging, so let me know if there are any questions there! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but let's also document the ability to use https_proxy
and HTTPS_PROXY
environment variables in the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filmaj Good call, I added a new section (sibling to the techniques) with an example using HTTPS_PROXY. |
Thank you so much for your contribution! |
Summary
Requirements (place an
x
in each[ ]
)The CLA mentioned in the Contributing Guidelines could not be found, but I'm (probably) willing to sign it if you can point me in the right direction.The bot took care of that promptly!