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.

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

doc: additional note in README(.md) informing users that it is advise… #32655

Conversation

haqer1
Copy link
Contributor

@haqer1 haqer1 commented Apr 4, 2020

…d to import the full set of trusted release keys (rather than an individual key)

Additional README.md update

Fixes: #32559

Checklist
Note

This is part 2 (alternative) to simplify closure of part 1 in PR #32591 (this PR is mutually exclusive with PR #32654 (which the OP personally prefers to this PR))

…d to import the full set of trusted release keys (rather than an individual key)

Additional README.md update

Fixes: nodejs#32559
README.md Outdated Show resolved Hide resolved
This leaves a bit more between the lines, but makes the verbiage shorter.

Co-Authored-By: Rich Trott <rtrott@gmail.com>
@haqer1
Copy link
Contributor Author

haqer1 commented Apr 9, 2020

IMHO, perhaps a little too much emphasis on keeping the number of words to a minimum (could make one feel as if one was working on translating a holy book (if that's an appropriate thing to say)). But anyway, if the suggested shortened verbiage just committed is not explicit enough, somebody will make it known (before or after landing)...

README.md Outdated
Comment on lines 568 to 569
To avoid nuances involved in verification of a sub-key possibly used to sign a
release, import the full set of trusted release keys:
Copy link
Member

Choose a reason for hiding this comment

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

To import the full set of trusted release keys (including subkeys used to sign releases):

I think the current test has a bit too much editorialization. I think suggested text above accomplishes the intention while keeping things succinct.

LMK if you have alternative suggestions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To import the full set of trusted release keys (including subkeys used to sign releases):

IMHO, there is a need to mention something like "nuance"s or "pitfall"s, to make sure that users get an idea that if they choose to not import all the keys following (assuming #32654 is not going to land), they might (well) run into something they aren't expecting. My motivation was to prevent confusion & misuse of time (which happened to me).
So between this suggestion & the current version, i'd prefer the current version (because it's more informative, etc.).

P.S. It seems there is an agreement on a need for change, & hopefully there will be an agreement on the wording w/o spending too much time on it. For me, the current wording committed is better that the status quo.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I stand by my original review. I think the current text is too editorialized and prefer the proposed text I suggested. I'm open to reviewing alternative suggestions

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jul 3, 2020
@aduh95 aduh95 added stalled Issues and PRs that are stalled. and removed stalled Issues and PRs that are stalled. labels Oct 19, 2020
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@haqer1
Copy link
Contributor Author

haqer1 commented Oct 26, 2020

OK, not descriptive enough IMHO, but since there is that much effort to keep it very very short, would this be accepted?
To import the full set of trusted release keys (including subkeys possibly used to sign releases):

P.S. At least "possibly" makes it clear that subkeys aren't always used, but they might be...

@MylesBorins
Copy link
Member

That seems much better to me. If you update the PR I would sign off on that copy

…d to import the full set of trusted release keys (rather than an individual key) (reconciled with another suggestion from code review)

The OP finds his original suggestion more descriptive & more user-friendly, but prefers to move on since that suggestion is stalled in favor of much shorter verbiage

Co-Authored-By: Myles Borins <mylesborins@google.com>

Fixes: nodejs#32559
…d to import the full set of trusted release keys (rather than an individual key) (reconciled with another suggestion from code review)

The OP finds his original suggestion more descriptive & more user-friendly, but prefers to move on since that suggestion is stalled in favor of much shorter verbiage. This version also splits the line at 80 characters to comply with lint-md.

Co-Authored-By: Myles Borins <mylesborins@google.com>

Fixes: nodejs#32559
Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed stalled Issues and PRs that are stalled. labels Nov 8, 2020
aduh95 pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #32655
Fixes: #32559
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2020

Landed in 888900b

@aduh95 aduh95 closed this Nov 9, 2020
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #32655
Fixes: #32559
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #32655
Fixes: #32559
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #32655
Fixes: #32559
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #32655
Fixes: #32559
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
7 participants