-
Notifications
You must be signed in to change notification settings - Fork 262
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
Install settings shutdown handler for TLS servers #915
Install settings shutdown handler for TLS servers #915
Conversation
Any shutdown handler passing in the settings to 'runTLS' or 'runTLSSocket' was not being installed in the same way that 'runSettingsSocket' and friends do for non-TLS servers. This updates 'runTLSSocket' to make it install the shutdown handler from the settings so that warp-tls behaves the same way as warp in this regard.
warp-tls/ChangeLog.md
Outdated
@@ -1,5 +1,10 @@ | |||
# ChangeLog | |||
|
|||
## 3.3.4.1 |
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.
This version number doesn't seem consistent with previous numbers. There are x.y.z.1
releases consistently follow a x.y.z
release.
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.
@ysangkok 3.3.4
was the version number in warp-tls.cabal
, so I incremented that for a bug fix. It looks like the master
branch now has change log notes for that version, but those notes were not present when I started my branch and I failed to notice the change log had been updated on master
when opening this PR. I'm happy to update the PR however is desired to get the right version number and notes in place.
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.
Please do merge in master
and reconcile the conflicts.
3.3.4.1
seems like it's fine when you finish merging in master
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.
@Vlix Done!
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.
@qxjit Sorry for the enormous delay, but if you could merge in master
again and bump it to 3.4.3
, then I'll try to get one of the maintainers to make a new release 😅
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.
@Vlix No worries. I've merged in master
and bumped the version to 3.4.3
now 😄
Nice catch. If you resolve the conflicts with the master branch, this can quickly be merged, I think. |
Looks good! @snoyberg Did I have permission to upload packages to hackage from the |
Sorry, didn't see that. I'm not sure if I gave you Hackage permissions. I'm happy either way. Let me know if you'd like me to add you, and if so, which packages you'd want to be a maintainer for and your Hackage username. |
I'm fine with having hackage permissions, so I can help with small things like version bounds revisions or small releases like this. I'm not too familiar with low level http to be of much use in that aspect, but |
Awesome, I've added you to those three repos. |
Any shutdown handler passing in the settings to 'runTLS' or 'runTLSSocket' was not being installed in the same way that 'runSettingsSocket' and friends do for non-TLS servers.
This updates 'runTLSSocket' to make it install the shutdown handler from the settings so that warp-tls behaves the same way as warp in this regard.
Before submitting your PR, check that you've:
After submitting your PR: