Backport of #5706 to v4.x #5983
Conversation
MylesBorins
force-pushed
the
v4.x-staging
branch
from
df3a543 to
7384140
Compare
|
@svozza ... can you rebase your branch to the current v4.x-staging to clear out the extraneous commits here? |
|
Oh this is weird, I ddn't see all those extra commits. I'm not actually sure how this happened. I didn't have the v4.x-staging branch before I did this change. All I did was this: git checkout -b v4.x-staging upstream/v4.x-staging
git cherry-pick 4f6ad5c
git push -u origin v4.x-stagingMy branch said it was only 1 commit ahead when I made the PR and when I do |
|
OK. I think I've fixed it. |
| @@ -50,29 +50,41 @@ openssl pkcs12 -export -in agent5-cert.pem -inkey agent5-key.pem \ | |||
| - `certfile`: all CA certs concatenated in one file like | |||
| `cat ca1-cert.pem ca2-cert.pem > ca-cert.pem` | |||
|
|
|||
| ## ALPN, NPN and SNI | |||
There was a problem hiding this comment.
This should not have made it into the backport
ALPN support is only on v5.x +
Please move this section
There was a problem hiding this comment.
Ah yes, I wasn't sure about that ALPN stuff. I'll remove the references.
|
@svozza when you cherry picked did you have conflicts? It appears to me like you merged all of the latest changes as opposed to manually handling the conflicts, as such we have a bunch of documentation in here about ALPN which is not a feature in v4.x I would suggest you might find it easier in the sections with conflicts to opt for the version present in v4, not in your commit and simply reaudit those sections to make sure they are correct |
|
Done. |
|
|
||
| NOTE: the change is effective only for the future server connections. Existing | ||
| or currently pending server connections will use previous keys. | ||
| ### server.maxConnections |
There was a problem hiding this comment.
where did this come from?
|
@svozza I'm still seeing quite a bit of information in here of v5 specific documentation. Would you be open to redoing this fix from scratch using the v4 doc? Sorry for being tight on this, but I really don't want to accidentally merge incorrect docs |
|
Yeah, no problem. I think I made a mess of the rebase. |
|
@svozza thanks for all the hard work! |
|
@svozza let me know if I can help with anything |
|
Really sorry about the delay, will be able to get to it tonight though! |
|
So I just blew away the branch and started over. No rebasing or any messing around and I think it worked out much better. |
|
I apologize, I was commenting on a backport after the original commit was already accepted. I've removed my nits and will instead address these in a future PR |
|
Oh right, I was in the process of fixing them! :) I agree that 'This event is emitted...` reads better btw. |
|
@whitlockjc the review is appreciated. This backport ended up having to be a complete rewrite due to the differences (to a certain extent). |
|
Well, if I wasn't out of line, let me add them back. Thanks for stepping in @thealphanerd and @svozza for being so willing to help. <3. |
| @@ -202,63 +202,64 @@ established it will be forwarded here. | |||
|
|
|||
| `function (sessionId, sessionData, callback) { }` | |||
|
|
|||
| Emitted on creation of TLS session. May be used to store sessions in external | |||
| Emitted on creation of a TLS session. May be used to store sessions in external | |||
There was a problem hiding this comment.
For consistency, we might use "This event is emitted" like we did for the 'secure' event.
| `callback(null, sessionData)` once finished. If session can't be resumed | ||
| (i.e. doesn't exist in storage) one may call `callback(null, null)`. Calling | ||
| `callback(err)` will terminate incoming connection and destroy socket. | ||
| Emitted when the client wants to resume the previous TLS session. The event |
There was a problem hiding this comment.
Same as above for 'clientError' event.
Brings tls.markdown into alignment with the node.js styleguide, specifically regarding the use of personal pronouns. Also, fixes various typos, punctuation errors, missing definite/indefinite articles and other minor grammatical issues. PR-URL: nodejs#5706 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Think I got them all! |
|
I'll take another peek. I also see a number of places where we're using a trailing |
|
@svozza Thanks for addressing the If no one else has an opinion on the argument/option documentation inconsistencies, LGTM. |
|
Yeah, I really wasn't sure what to do there so I left it originally. I was leaning towards putting fullstops everywhere but thought it might be best not to go down that rabbithole. |
|
Let's see what @thealphanerd has to say. Others welcome of course as well but he was last involved before myself. Great job regardless. |
|
If something is consistent in the docs and not specific to this change I think it should come in another update |
|
@thealphanerd That's what I was thinking. Seems you've assigned to yourself so I'll step out and let you merge when it's ready. Thanks for stepping in. |
|
cool.. can I get an official LGTM from you @whitlockjc |
MylesBorins
force-pushed
the
v4.x-staging
branch
from
6291de2 to
a8312f2
Compare
|
ping @whitlockjc |
|
ping @whitlockjc I'm going to close for now, please feel free to reopen |
This is a backport of #5706 (doc: align doc/api/tls.markdown with style guide) to the v4.x-staging branch as requested @thealphanerd.