-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: additional note in README(.md) informing users that it is advise… #32655
Conversation
…d to import the full set of trusted release keys (rather than an individual key) Additional README.md update Fixes: nodejs#32559
This leaves a bit more between the lines, but makes the verbiage shorter. Co-Authored-By: Rich Trott <rtrott@gmail.com>
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
To avoid nuances involved in verification of a sub-key possibly used to sign a | ||
release, import the full set of trusted release keys: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @Trott @MylesBorins
There was a problem hiding this comment.
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
8ae28ff
to
2935f72
Compare
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. |
OK, not descriptive enough IMHO, but since there is that much effort to keep it very very short, would this be accepted? P.S. At least "possibly" makes it clear that subkeys aren't always used, but they might be... |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed in 888900b |
…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))