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

doc: remove reference to "credentials object" #26908

Closed
wants to merge 2 commits into from
Closed

doc: remove reference to "credentials object" #26908

wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Mar 25, 2019 •
edited
Loading

The reference is confusing because the object is actually of class
SecureContext. There is no object with class "credentials".

See: #20432 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3038/pipeline

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. labels Mar 25, 2019
@sam-github sam-github mentioned this pull request Mar 25, 2019
Closed
@sam-github
Copy link
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3039/

vsemozhetbyt
vsemozhetbyt reviewed Mar 25, 2019
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
@sam-github
doc: remove reference to "credentials object"
d4ce20b 
The reference is confusing because the object is actually of class
SecureContext. There is no object with class "credentials".

See: nodejs#20432 (comment)
@sam-github
Copy link
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3040/

Trott
Trott reviewed Mar 25, 2019
View reviewed changes
doc/api/tls.md Outdated
@@ -1406,7 +1406,9 @@ to `true`, other APIs that create secure contexts leave it unset.
from `process.argv` as the default value of the `sessionIdContext` option, other
APIs that create secure contexts have no default value.

The `tls.createSecureContext()` method creates a credentials object.
The `tls.createSecureContext()` method creates a `SecureContext` object. The
object has no public methods, but is accepted as an argument to several `tls`
Copy link
Member

Choose a reason for hiding this comment

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

Total micro-nit that you can ignore if you disagree, but I think the two sentences are clearer if you divide them up this way instead:

The `tls.createSecureContext()` method creates a `SecureContext` object with
no public methods. It is usable as an argument to several `tls` APIs such as
[`tls.createServer()`][] and [`server.addContext()`][].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, how about ba029f5?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 26, 2019
@sam-github
Copy link
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3058/

@sam-github
Copy link
Contributor Author

Landed in f11f180

@sam-github sam-github closed this Mar 28, 2019
@sam-github sam-github deleted the remove-credentials-object branch March 28, 2019 21:07
sam-github added a commit that referenced this pull request Mar 28, 2019
@sam-github
doc: remove reference to "credentials object"
f11f180 
The reference is confusing because the object is actually of class
SecureContext. There is no object with class "credentials".

See: #20432 (comment)

PR-URL: #26908
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Mar 28, 2019
@sam-github @targos
doc: remove reference to "credentials object"
bc52b6b 
The reference is confusing because the object is actually of class
SecureContext. There is no object with class "credentials".

See: #20432 (comment)

PR-URL: #26908
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Mar 29, 2019
@sam-github @targos
doc: remove reference to "credentials object"
256587d 
The reference is confusing because the object is actually of class
SecureContext. There is no object with class "credentials".

See: #20432 (comment)

PR-URL: #26908
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Mar 30, 2019
@sam-github @targos
doc: remove reference to "credentials object"
17bff5c 
The reference is confusing because the object is actually of class
SecureContext. There is no object with class "credentials".

See: #20432 (comment)

PR-URL: #26908
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cinderblock
Copy link

@sam-github Why does SecureContext not have any public methods?

Specifically, secureContext.context.addCACert(...) seems to be available and quite helpful. See #20432 (comment) and #20432 (comment) for more details.

@sam-github
Copy link
Contributor Author

@cinderblock I can't speak to the history, but I can say if there is a specific feature you would like, please post a feature request. It will help to have some concrete use cases. There are lots of things we could just make public, but it takes short term effort to add tests and docs (though feature requests with an acompanying PR don't have to attract a volunteer), and long-term, it can make it harder to improve the node API if too much of our internals are exposed in the API.

@cinderblock cinderblock mentioned this pull request Apr 3, 2019
Open
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
@sam-github @BethGriggs
doc: remove reference to "credentials object"
cf66e93 
The reference is confusing because the object is actually of class
SecureContext. There is no object with class "credentials".

See: #20432 (comment)

PR-URL: #26908
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@Trott Trott Trott left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@sagirk sagirk sagirk approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@bnoordhuis bnoordhuis bnoordhuis 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. doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@sam-github @nodejs-github-bot @cinderblock @bnoordhuis @Trott @lpinca @cjihrig @sagirk @BridgeAR @vsemozhetbyt