-
Notifications
You must be signed in to change notification settings - Fork 55
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
Initial conversion of the specification to bikeshed flavoured Markdown #143
Initial conversion of the specification to bikeshed flavoured Markdown #143
Conversation
Convert tuf-spec.md to use bikeshed flavoured markdown https://tabatkins.github.io/bikeshed/#markdown Co-authored-by: Joshua Lock <[email protected]>
Start introducing bikeshed specific markup: * definition lists for various key terms in the specification * add dfn elements to definitions of terms * add corresponding anchor links to link to definitions * add JSON syntax highlighting to the Markdown examples Co-authored-by: Erick Tryzelaar <[email protected]>
Add editors, mailing list, repository. Change the status. Omit conformance boilerplate, we already discuss RFC 2119 in the spec.
This is great, thanks @joshuagl! One thing I didn't get a chance to explore is that we could leverage a github action like https://github.com/koumoul-dev/gh-pages-multi to publish the main branch to a |
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 is a big improvement for readability. The links within the document and overall format make the rendered version much easier to reference. I support merging this as is, and adding automation later.
Amazing work, @joshuagl and @erickt! And good question, @trishankatdatadog. Based on a quick grep, we say "1.0.0" in the section about supported taps and in example metadata ( |
Nice find. This exactly the kind of workflow I was planning to create, I will try and experiment with gh-pages-multi this week. Thanks @erickt |
I agree with the rationale here, I've pushed a change to say "this major version (1.x.y) of the specification" in f4b15a9 |
40e009b
to
ecdb544
Compare
Rephrase many of the definitions to work better with the new format and layout. Add additional definitions and cross-references to definitions.
Remove one level of nesting for client workflow, as the only detailed workflow it doesn't make sense to have an additional heading/numbered section above. Rename some steps in detailed client workflow to remove redundancy between step title and details, and make step titles shorter.
Bikeshed does not recognise autolinks nor automatically render URLs as links, therefore turn all bare URLs in the document into clickable links by converting all bare URLs into Markdown links.
Consistently link to the copies of the papers hosted at our site theupdateframework.io
We expect major versions, per semver, to adhere to all of the TAPs which have been introduced into the specification.
Update some of the step references to be correct for the current numbering and add some links to parts of the specification that are cross-referenced from the detailed client workflow. Signed-off-by: Joshua Lock <[email protected]>
Switch from using Markdown auto-numbered lists to explicitly numbered steps in the detailed client workflow. As we currently cross-reference other steps in the workflow, having to build the document in order to see what number has been assigned is awkward. Signed-off-by: Joshua Lock <[email protected]>
We never want to check the generated tuf-spec.html into Git, thus add it to .gitignore Signed-off-by: Joshua Lock <[email protected]>
ecdb544
to
569dccb
Compare
I've pushed a few additional tweaks since the initial comments. I started working on a publishing pipeline using GitHub Actions, but didn't get it "done enough" to include here before heading out over the winter break. I will pick up the automation piece of this in the new year. |
Automating this with Github Actions pushing to GitHub Pages is a bit clunky, especially as I'm trying to avoid using 3rd-party actions. My current proposal is:
Questions:
Having written this all up, I think I should probably create a new issue for the automation and handle it separately to this PR. @JustinCappos, @trishankatdatadog, @mnm678, @lukpueh can we review this PR as is with a view to merging and handling automation later? @mnm678 indicated she was OK with that approach (but my adding to the PR dismissed her review) , any objections? |
Close/re-open to prod Travis. |
Apparently we've consumed all of our credits for theupdateframework organisation until 18th January, I assume these will be replenished some time today 🤷 |
This all sounds wonderful to me, and I'm looking forward to reviewing the GH Actions (and learn). Have you also thought about auto-syncing master with draft on a release? I suppose this will usually involve manual conflict resolving, so maybe the action can at least create a corresponding issue on each release so that maintainers don't forget?
If we release manually, then yes. If we release automatically, then probably yes, although that signature seems less significant.
I don't think it's worth the effort. Making
I don't think the build history for all PRs is of much interest, but I also don't think an automated cleanup workflow is worth much effort, at least for now. Or does the workflow already exist, then we should use it.
Yes, I am not very happy about them either, and I did make some suggestion to mitigate this threat (theupdateframework/python-tuf#1246 (comment)). But I think I'll let others decide here. On a related side-note: It would be really cool if there were multiple independent builders / release taggers and a software supply chain framework to encode authorization and threshold requirements... :P At any rate, if we decide to keep signed releases a manual task, then I'd also love to have a GitHub action that auto-creates an issue to remind maintainers of that task.
👍
I'm fine with this. But I haven't looked at the diff yet. I can do that tomorrow and green-light this PR. |
Yes, this stuff is beautiful, let's go ahead. Someone else should double-check that we haven't lost anything in translation. |
I dismissed the idea of auto-synching, because in my (limited) experience there's usually some manual conflict resolving. Filing an issue on merge is a great idea, though!
🤔
💯 this is a fine idea.
Brilliant, thank you @lukpueh ! |
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.
Now that I've taken a closer look, this seems even more impressive. Thanks again, @joshuagl!
So far I've rather superficially compared the old rendered markdown with the new rendered html, which looks really good. I'd like to give it another more thorough (word-diff) pass, especially in regards to renumbering, before I green-light.
Update line numbers and the date pattern we search for in check_release to match the newly bikeshed flavoured specification document. Signed-off-by: Joshua Lock <[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.
Here is the more thorough review as promised, which includes:
- a line by line review using
git diff --ignore-space-change --word-diff
(GitHub provides a similar view with thew=1
get param), - a review of those changes in the rendered html,
- a grep on all words for which definitions exist (I added a few suggestions for additional references of already defined words, plus new definitions)
- a check of all cross-references to client workflow section (given that the numbering has shifted)
Spectacular review, thanks Lukas! 🙇🏽♂️ |
Add additional cross-reference links for defined terms and sections. Co-authored-by: lukpueh <[email protected]>
Minor grammar and language changes to improve clarity Co-authored-by: lukpueh <[email protected]>
These don't aid clarity with the new format and clickable PLACEHOLDERS Signed-off-by: Joshua Lock <[email protected]>
Signed-off-by: Joshua Lock <[email protected]>
Signed-off-by: Joshua Lock <[email protected]>
* Resolve incorrect indentation causing bikeshed error * Disambiguate "paths" definition, to resolve a warning Signed-off-by: Joshua Lock <[email protected]>
Add a custom HTML header for use in the generated HTML version of the document. This allows us to: 1. not display a Status -- status isn't something we've typically used in TUF and the options available to us in bikeshed don't quite fit our use 2. display the specification version more prominently in the sub-heading, alongside the last modified date Signed-off-by: Joshua Lock <[email protected]>
Add a KEY definition to the part of the "File formats: general principles" section that describes the KEY format. Signed-off-by: Joshua Lock <[email protected]>
This was inconsistent; because we usually define the PLACEHOLDER, not the dictionary key. The description was incorrect; describing a list, when in fact we are expecting an OBJECT/dict of KEYID: KEY. Finally; the KEYID and KEY placeholders are already defined elsewhere in the document. Signed-off-by: Joshua Lock <[email protected]>
Signed-off-by: Joshua Lock <[email protected]>
Link the HEX_DIGEST placeholder to the explanation of HEX_DIGEST in the delegations object. Signed-off-by: Joshua Lock <[email protected]>
Add the preprocessor recognised keyword "Note" to have Bikeshed put this remark in a highlight box. Signed-off-by: Joshua Lock <[email protected]>
The "File formats: general principles" section had two separate places discussing KEYIDs, consolidate those in the same place -- where the definition is. Signed-off-by: Joshua Lock <[email protected]>
Thank you for the very detailed and high-quality review @lukpueh! I've addressed all of your suggestions for immediate work in follow-on commits, and added a few other minor changes. Most notably;
|
High-quality work deserves a thorough review! :) Thanks for addressing my comments. I'll give it another pass on Monday. |
A rendered version of the specification following the latest round of changes is viewable here: https://joshuagl.github.io/specification/1.0.17/ |
Absolutely amazing work, @joshuagl! Thanks a ton for taking care of this behemoth. Future readers of the spec should also thank you. Syncing changes into draft when we merge this might be a bit tricky, maybe we should do it in a PR so that someone can review it. |
I've prepared a branch for that here https://github.com/joshuagl/specification/tree/joshuagl/bikeshed-initial-draft |
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 PR lays the groundwork for #121 and is based on the excellent initial work by @erickt.
You can see a rendered version of the bikesheded specification document here: https://joshuagl.github.io/specification/
The major changes in this PR are:
I've been pleasantly surprised by how readable the raw document still is, and how nice the rendered version looks. The styling of code blocks, cross-linking of definitions, and hyper-linked contents are a major improvement to the readability of the rendered specification.
Because the raw version isn't significantly more complex to edit, but handling additional changes to the specification makes maintaining this in a branch awkward, I would like to propose that we consider merging this PR and continuing to work on next steps (see below) with additional PRs against the main branch.
cc @JustinCappos @trishankatdatadog @mnm678 @lukpueh
Next steps: