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: Updates the OpenSSL 'list-message-digest-algorithms' to the newer 'list digest-algorithms' #20400

Closed
wants to merge 5 commits into from
Closed

doc: Updates the OpenSSL 'list-message-digest-algorithms' to the newer 'list digest-algorithms' #20400

wants to merge 5 commits into from

Conversation

shobhitchittora
Copy link
Contributor

Closes: #20385

  • documentation is changed or added
  • commit message follows [commit guidelines]

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Apr 29, 2018
@shobhitchittora shobhitchittora changed the title docs: Updates the OpenSSL list-message-digest-algorithms to the newer list digest-algorithms docs: Updates the OpenSSL 'list-message-digest-algorithms' to the newer 'list digest-algorithms' Apr 29, 2018
@shobhitchittora shobhitchittora changed the title docs: Updates the OpenSSL 'list-message-digest-algorithms' to the newer 'list digest-algorithms' doc: Updates the OpenSSL 'list-message-digest-algorithms' to the newer 'list digest-algorithms' Apr 29, 2018
vsemozhetbyt
vsemozhetbyt reviewed Apr 29, 2018
View reviewed changes
doc/api/crypto.md Outdated
@@ -1585,7 +1585,7 @@ behavior.

The `algorithm` is dependent on the available algorithms supported by the
version of OpenSSL on the platform. Examples are `'sha256'`, `'sha512'`, etc.
On recent releases of OpenSSL, `openssl list-message-digest-algorithms` will
On recent releases of OpenSSL, `openssl list digest-algorithms` ( `openssl list-message-digest-algorithms` for older versions of OpenSSL ) will
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we have a linter rule that requires wrapping long lines at 80 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment. Running make lint didn't show any errors for me. 👍🏻

Made the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. If I recall correctly, make lint only lints code files and code fragments in the docs, not the doc formatting.

@shobhitchittora
Copy link
Contributor Author

@RichAyotte thanks for pointing it out. Somehow I missed that in the doc. 🤦‍♂️

@vsemozhetbyt
Copy link
Contributor

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

@shobhitchittora
Copy link
Contributor Author

🎉🎉🎉🎉 CI completed. Any other changes or can we close this ?

@vsemozhetbyt
Copy link
Contributor

As per our rules, we need to wait a bit)

@vsemozhetbyt
Copy link
Contributor

Node.js Collaborators, please, add 👍 here to approve fast-tracking.

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 29, 2018
@shobhitchittora
Copy link
Contributor Author

@vsemozhetbyt didn't know about that. This the first time I've read something like this for an OSS project. I can see the benefits of this, in not hurrying things up and really pushing for quality. Kudos! 👍🏻

@vsemozhetbyt
Copy link
Contributor

If you have some time to read, there is our contributing guide start point :)

BridgeAR
BridgeAR reviewed Apr 29, 2018
View reviewed changes
doc/api/crypto.md Outdated
@@ -1585,7 +1585,8 @@ behavior.

The `algorithm` is dependent on the available algorithms supported by the
version of OpenSSL on the platform. Examples are `'sha256'`, `'sha512'`, etc.
On recent releases of OpenSSL, `openssl list-message-digest-algorithms` will
On recent releases of OpenSSL, `openssl list -digest-algorithms` (
`openssl list-message-digest-algorithms` for older versions of OpenSSL ) will
Copy link
Member

Choose a reason for hiding this comment

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

Would you be so kind and remove the whitespace in the beginning and end of the bracket? And move the opening bracket to the second line, so it is nicer to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @BridgeAR.

@shobhitchittora
Copy link
Contributor Author

@BridgeAR @vsemozhetbyt Anything more to do for this ?

@vsemozhetbyt
Copy link
Contributor

cc @nodejs/crypto Can we have one LGTM for this change?

shigeki
shigeki approved these changes May 3, 2018
View reviewed changes
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

LGTM.
openssl list-cipher-algorithms also needs to be changed to openssl list -cipher-algorithms.
Please submit an new PR.

@vsemozhetbyt
Copy link
Contributor

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

vsemozhetbyt pushed a commit that referenced this pull request May 3, 2018
@shobhitchittora @vsemozhetbyt
doc: update cli flag in crypto.md
6a24c0c 
PR-URL: #20400
Fixes: #20385
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented May 3, 2018 •
edited
Loading

Sorry I have landed before the last commit: 6a24c0c

Can you please open another PR for the last commit?

@shobhitchittora
Copy link
Contributor Author

@vsemozhetbyt I'll do that.
Also I'll open a new issue for tracking @shigeki suggestion.

@vsemozhetbyt
Copy link
Contributor

Thank you!

@shobhitchittora shobhitchittora deleted the update-doc-openssl-list-digest-algos branch May 3, 2018 15:00
MylesBorins pushed a commit that referenced this pull request May 4, 2018
@shobhitchittora @MylesBorins
doc: update cli flag in crypto.md
2d26f55 
PR-URL: #20400
Fixes: #20385
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
vsemozhetbyt pushed a commit that referenced this pull request May 7, 2018
@shobhitchittora @vsemozhetbyt
doc: update one more command in crypto.md
5efbe4c 
PR-URL: #20500
Refs: #20400
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@shobhitchittora @MylesBorins
doc: update one more command in crypto.md
aef4f80 
PR-URL: #20500
Refs: #20400
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@shobhitchittora @MylesBorins
doc: update cli flag in crypto.md
8b17e7a 
PR-URL: #20400
Fixes: #20385
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@shobhitchittora @MylesBorins
doc: update one more command in crypto.md
38323bc 
PR-URL: #20500
Refs: #20400
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
@shobhitchittora @MylesBorins
doc: update one more command in crypto.md
bea4ffc 
PR-URL: #20500
Refs: #20400
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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

@BridgeAR BridgeAR BridgeAR left review comments

@trivikr trivikr trivikr approved these changes

@tniessen tniessen tniessen approved these changes

@shigeki shigeki shigeki approved these changes

Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@shobhitchittora @vsemozhetbyt @RichAyotte @shigeki @tniessen @BridgeAR @trivikr @nodejs-github-bot