search

ring-clojure / ring

Clojure HTTP server abstraction
MIT License
3.75k stars 519 forks source link

Add KeyStoreScanner bean to the jetty server #417

Closed sirmspencer closed 3 years ago

sirmspencer commented 3 years ago

Closes #416

atomist[bot] commented 3 years ago

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

sirmspencer commented 3 years ago

@weavejester Do you have time to review the updates from your change requests?

weavejester commented 3 years ago

This looks fine. It's worth mentioning that Github doesn't notify me on new commits, only on new comments, so it's often a good idea to put a comment in after you've finished making changes, or else I'll never learn about the new commits.

Can you squash down your commits, ready to be merged? Also while it's not necessary, it might be useful to separate out the alignment change into a commit that just adds whitespace, so people can more easily see which lines are relevant.

sirmspencer commented 3 years ago

This looks fine. It's worth mentioning that Github doesn't notify me on new commits, only on new comments, so it's often a good idea to put a comment in after you've finished making changes, or else I'll never learn about the new commits.

That is good to know, thank you!

Can you squash down your commits, ready to be merged? Also while it's not necessary, it might be useful to separate out the alignment change into a commit that just adds whitespace, so people can more easily see which lines are relevant.

My original submission had the two sets of changes in separate commits. Would you like me to keep the pattern of the first two commits, but flatten in your review changes to the first? I see that my original two comments did a bad job of keeping them separate. VScode didnt stage the changes correctly. I think I got it correct the second time.

weavejester commented 3 years ago

Thanks! Almost perfect. Can you change the formatting on the first commit message to be:

Add optional KeyStoreScanner to jetty server

When a :keystore-scan-interval is provided, a KeyStoreScanner will be
added to monitor the keystore. When the keystore is updated, Jetty will
gracefully refresh its SSL session.

Then I'll merge it in.

sirmspencer commented 3 years ago

@weavejester I made the change to the commit message you asked for.