-
Notifications
You must be signed in to change notification settings - Fork 20
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
[TAP 8] Update TAP 8 for clarity and changes in the spec #163
[TAP 8] Update TAP 8 for clarity and changes in the spec #163
Conversation
Notably this commit: * specifies that rotate files will be included in snapshot metadata * fixes/clarifies references to TAP 3 * Removes references to rejected TAPs * Simplifies the TAP 8 mechanism by not requiring role name changes Signed-off-by: Marina Moore <[email protected]>
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.
Looks fine to me, but it certainly is a mismatch of edits.
It's a collection of minor edits from re-reading this after 4 years of changes to TUF. If there's anything controversial (which I tried to avoid) I can move that into a different pr. |
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.
This looks great, thank you. I added a question/clarification but otherwise LGTM.
An additional minor comment: there's some strange line lengths in the edits i.e., line 49 is very long and line 191 is very short. The rest of the file wraps at ~80characters. Should we care about this? If we should, perhaps we could add a linter?
Signed-off-by: Marina Moore <[email protected]>
Signed-off-by: Marina Moore <[email protected]>
Thanks for the review!
I added a commit that cleans this up a bit for this TAP. In the long term it'd be nice to have a consistent stance backed by a linter. The ~80 character lines look nice, but lead to strange diffs when the text is edited, so I'd advocate for line breaks at the sentence or paragraph level. |
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, thanks!
I added a commit that cleans this up a bit for this TAP. In the long term it'd be nice to have a consistent stance backed by a linter. The ~80 character lines look nice, but lead to strange diffs when the text is edited, so I'd advocate for line breaks at the sentence or paragraph level.
Thank you for cleaning these up. Last time I looked (to use in SLSA slsa-framework/slsa#75) I couldn't find a linter which supports semantic line breaks. That was long enough ago that it would be worth another look. Let's file a separate issue for this?
Filed #164 |
@JustinCappos can you re-approve so this can be merged? |
Notably this pr: