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.

Sign up for GitHub

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

Jump to bottom

Document the x509 error codes #37096

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Document the x509 error codes #37096

merged 1 commit into from
Feb 22, 2021

Conversation

dcermak
Copy link
Contributor

@dcermak dcermak commented Jan 27, 2021 •
edited by aduh95
Loading

This fixes: #29342 (was closed unresolved)

Fixes: #29342

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Jan 27, 2021
aduh95
aduh95 reviewed Jan 27, 2021
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
src/crypto/crypto_common.cc
Comment on lines +300 to +301
// if you modify anything in here, *please* update the respective section in
// doc/api/tls.md as well
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is useful to add a comment here. The chances for this list to be modified is very low – it hasn't changed in the last 8 years, since it was "introduced" in b9a0eb0 (and it's even older than that but you got the point).

Suggested change
// if you modify anything in here, *please* update the respective section in
// doc/api/tls.md as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, not sure that I agree with you here. Especially as this code is really old, while it is unlikely that it will be changed ever again, if it will, updating the docs will surely be forgotten. But I'm not insisting on keeping this commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, let's wait and see if anyone wants to share their thoughts on it. One thing to consider is that doc-only commits tends to be backported faster to LTS release lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm very much open to opinions from contributors as I'm not really familiar with node's development. I'm also not really in a rush to get this backported, so that would be fine by me.

Copy link
Member

Choose a reason for hiding this comment

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

The comment doesn't hurt I think :-)

doc/api/tls.md Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Jan 27, 2021
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Jan 27, 2021
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35833/

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2021
@dcermak @aduh95
doc: document x509 error codes
c909481 
Signed-off-by: Dan Čermák <dcermak@suse.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#37096
Fixes: nodejs#29342
Fixes: nodejs#29342
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented Feb 22, 2021

Landed in c909481

@aduh95 aduh95 merged commit c909481 into nodejs:master Feb 22, 2021
targos pushed a commit that referenced this pull request Feb 28, 2021
@dcermak @targos
doc: document x509 error codes
9ac8c74 
Signed-off-by: Dan Čermák <dcermak@suse.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>

PR-URL: #37096
Fixes: #29342
Fixes: #29342
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Mar 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Please document the list of possible errors for tls#TLSSocket#authorizationError
6 participants
@dcermak @nodejs-github-bot @aduh95 @jasnell @Trott @targos