Skip to content

Issue-268: Websocket login respects https_proxy environment variable #271

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

Merged
merged 2 commits into from
Nov 9, 2016

Conversation

liuweichu
Copy link

@liuweichu liuweichu commented Oct 4, 2016

  • [Y] I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • [Y] I've read and agree to the Code of Conduct.
  • [Y] I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
  • [Y] I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
  • I've written tests to cover the new code and functionality included in this PR.
  • [Y] I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

If https_proxy is set, slack websocket will use the proxy to connect to slack server.
This solves the issue that hubot-slack cannot login behind firewall.

Related Issues

Fixes #268

Test strategy

The code is running great in our production env, no test case is written though. (If test is required, please guide me how to write :octocat:

@UncannyBingo
Copy link

@aoberoi Could I convince you give this a review? I'm inclined to 👍 but I'd like your eyes on it too.

@UncannyBingo
Copy link

In the meantime, @liuweichu I see that the linter is rejecting your changes. In particular, we require string literals to use single-quotes, and all lines to be terminated with semi-colons. These issues must be resolved before we can accept this PR.

I apologize that this has taken me so long to come back around to, and I am eager to get this merged in!

Copy link

@UncannyBingo UncannyBingo left a comment

Choose a reason for hiding this comment

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

Unfortunately, it doesn't look like @liuweichu actually signed the CLA. We cannot approve without a signed CLA :(

@liuweichu
Copy link
Author

@DEGoodmanWilson Thanks for lintering

  • Rebased on current master
  • " -> '
  • ; added

@coveralls
Copy link

coveralls commented Nov 4, 2016

Coverage Status

Coverage decreased (-0.06%) to 85.603% when pulling e0ccfc7 on liuweichu:master into ce618b1 on slackapi:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 85.603% when pulling e0ccfc7 on liuweichu:master into ce618b1 on slackapi:master.

@coveralls
Copy link

coveralls commented Nov 9, 2016

Coverage Status

Coverage decreased (-0.06%) to 85.603% when pulling 4181511 on liuweichu:master into fdd3717 on slackapi:master.

@UncannyBingo UncannyBingo merged commit 1f241ac into slackapi:master Nov 9, 2016
@aoberoi
Copy link
Contributor

aoberoi commented Nov 9, 2016

Oops I missed this mention! I'll find a better way to deal with the notifications moving forward.

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.

5 participants