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 #1181 Add port property to installerOptions in the HTTPReceiver #1184

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Oct 29, 2021

Summary

This pull request resolves #1181 by adding custom port option in the App/HTTPReceiver constructors. Also, while working on this, the SocketModeReceiver starts its underlying HTTP server for the OAuth flow (before start() method call). As I believe that the timing to start the HTTP server should be consistent with other receivers, I've changed the behavior too.

Fixes #1181
Fixes #1179

Requirements (place an x in each [ ])

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality labels Oct 29, 2021
@seratch seratch added this to the 3.8.0 milestone Oct 29, 2021
@seratch seratch self-assigned this Oct 29, 2021
@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #1184 (431d890) into main (3077c40) will increase coverage by 0.09%.
The diff coverage is 54.54%.

❗ Current head 431d890 differs from pull request most recent head bd1f71f. Consider uploading reports for the commit bd1f71f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1184      +/-   ##
==========================================
+ Coverage   71.33%   71.42%   +0.09%     
==========================================
  Files          17       17              
  Lines        1399     1414      +15     
  Branches      415      424       +9     
==========================================
+ Hits          998     1010      +12     
- Misses        331      332       +1     
- Partials       70       72       +2     
Impacted Files Coverage Δ
src/receivers/HTTPReceiver.ts 37.08% <11.11%> (-0.96%) ⬇️
src/receivers/SocketModeReceiver.ts 88.63% <80.00%> (-0.65%) ⬇️
src/App.ts 85.05% <100.00%> (+1.41%) ⬆️

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 3077c40...bd1f71f. Read the comment docs.

@seratch seratch force-pushed the issue-1181-custom-port branch from f4c5323 to 871bf1c Compare October 29, 2021 07:33
@seratch seratch force-pushed the issue-1181-custom-port branch from 871bf1c to bd1f71f Compare October 29, 2021 08:06
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard work on this! Looks good 👍 :shipit:

@seratch
Copy link
Member Author

seratch commented Oct 29, 2021

Thank you for your quick review!

@seratch seratch merged commit 1b450f6 into slackapi:main Oct 29, 2021
@seratch seratch deleted the issue-1181-custom-port branch October 29, 2021 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add port property to installerOptions in the HTTPReceiver Custom Port Ignored when Using Socket Mode
2 participants