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

Initial conversion of the specification to bikeshed flavoured Markdown #143

Merged
merged 26 commits into from
Jan 27, 2021

Conversation

joshuagl
Copy link
Member

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:

  • Convert tuf-spec.md to Bikeshed Flavoured Markdown (bfm) in 2be515a
  • Start adopting some of the bfm features in 0713382 and c714c04. NOTE until this point the content of the spec is unchanged, all of the changes are stylistic
  • Changes the wording of some of the object field definitions, to better match the new layout and style, in 211ffbf
  • Misc. tweaks in remaining commmits

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:

  • Agree whether to proceed with switching to Bikeshed flavoured Markdown & merge this PR
  • Define a CI pipeline that will
    • automatically publish updated versions of the specification (master branch) to i.e theupdateframework.github.io/specification
    • preserve, in a versioned location, a published version for each tagged release of the specification to i.e. theupdateframework.github.io/specification/1.0.16/
    • automatically publish updated version of the draft specification (draft branch) to i.e. theupdateframework.github.io/specification/draft
  • Convert the detailed client workflow to call out to subsections (see Rewriting the workflow to call out to sub-sections #121)

erickt and others added 3 commits December 11, 2020 09:06
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.
@erickt
Copy link
Contributor

erickt commented Dec 11, 2020

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 /draft directory, then have a separate branch tracks the latest stable version publish to a separate directory.

mnm678
mnm678 previously approved these changes Dec 11, 2020
Copy link
Collaborator

@mnm678 mnm678 left a 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.

@trishankatdatadog
Copy link
Member

ZOMG, this is fantastic, thanks @joshuagl and @erickt 👏🏽

QQ: the main spec version is 1.0.17, but there are places throughout the spec where we say 1.0.0. Should they all say 1.0.17?

@lukpueh
Copy link
Member

lukpueh commented Dec 14, 2020

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 (spec_version field). I think we could change the former to say "1.y.z", i.e. "this major version" instead of "this version".
The latter I'd probably leave as is and not bump with every (patch) release, because they are "only" examples and adopters are free to interpret them as they wish, also they must be numeric, so I'd rather not use placeholders in the example.

@joshuagl
Copy link
Member Author

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 /draft directory, then have a separate branch tracks the latest stable version publish to a separate directory.

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

@joshuagl
Copy link
Member Author

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 (spec_version field). I think we could change the former to say "1.y.z", i.e. "this major version" instead of "this version".
The latter I'd probably leave as is and not bump with every (patch) release, because they are "only" examples and adopters are free to interpret them as they wish, also they must be numeric, so I'd rather not use placeholders in the example.

I agree with the rationale here, I've pushed a change to say "this major version (1.x.y) of the specification" in f4b15a9

@joshuagl joshuagl force-pushed the joshuagl/bikeshed-initial branch from 40e009b to ecdb544 Compare December 17, 2020 11:29
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]>
@joshuagl joshuagl force-pushed the joshuagl/bikeshed-initial branch from ecdb544 to 569dccb Compare December 18, 2020 15:27
@joshuagl
Copy link
Member Author

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.

@joshuagl
Copy link
Member Author

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:

  1. a CI workflow will build each PR and publish to an appropriately named directory in the gh-pages branch, i.e. this PR might be published to wip/pulls/143 (and then would be browsable at theupdateframework.github.io/specifcation/wip/pulls/143). The workflow would also update the PR with a link to the rendered document.
  2. a cleanup workflow would run periodically and compare the children of wip/pulls to open PRs, removing any children of wip/pulls for which there isn't a corresponding open PR
  3. a release workflow will be triggered on changes to master, and:
    1. make a release on GitHub with a corresponding versioned tag
    2. build the spec for that release and publish it to the latest directory of the gh-pages branch
    3. also publish the built spec to a versioned subdirectory of the gh-pages branch
    4. update a listing page (index.html?) to point to the versioned subdirectory (the listing will enable browsers to find: the latest spec, specific versioned spec releases, and the most recent draft)
  4. a draft workflow will be triggered on changes to draft, and:
    1. build the spec in the draft branch
    2. publish the spec to a draft subdirectory of the gh-pages branch

Questions:

  • Should we be signing spec releases (and therefore not doing automated releases on push to master)?
  • Do we want official (latest and versioned) releases of the spec to be published in a separate place to WIP (PRs) and draft releases? i.e. we could publish released versions of the spec on theupdateframwork.io via Netlify and only use GitHub Pages (theupdateframework.github.io/specification) to host WIP (draft & PR) specification versions.
  • Is the cleanup workflow necessary or, for the sake of a ~200K per PR, should we retain history for the PRs?
  • Does everyone distrust 3rd party actions, or is that just me? If we're only using actions to automate WIP changes, and opt to manually make signed spec releases (and push them to theupdateframework.io), I would personally be less anxious about using 3rd party actions.

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?

@joshuagl
Copy link
Member Author

Close/re-open to prod Travis.

@joshuagl joshuagl closed this Jan 18, 2021
@joshuagl joshuagl reopened this Jan 18, 2021
@joshuagl joshuagl changed the title RFC: Initial conversion of the specification to bikeshed flavoured Markdown Initial conversion of the specification to bikeshed flavoured Markdown Jan 18, 2021
@joshuagl
Copy link
Member Author

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 🤷

@lukpueh
Copy link
Member

lukpueh commented Jan 18, 2021

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:
...

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?

Questions:

  • Should we be signing spec releases (and therefore not doing automated releases on push to master)?

If we release manually, then yes. If we release automatically, then probably yes, although that signature seems less significant.

  • Do we want official (latest and versioned) releases of the spec to be published in a separate place to WIP (PRs) and draft releases? i.e. we could publish released versions of the spec on theupdateframwork.io via Netlify and only use GitHub Pages (theupdateframework.github.io/specification) to host WIP (draft & PR) specification versions.

I don't think it's worth the effort. Making theupdateframework.io point directly to latest and an index page that lists the versioned history should be just as good.

  • Is the cleanup workflow necessary or, for the sake of a ~200K per PR, should we retain history for the PRs?

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.

  • Does everyone distrust 3rd party actions, or is that just me? If we're only using actions to automate WIP changes, and opt to manually make signed spec releases (and push them to theupdateframework.io), I would personally be less anxious about using 3rd party actions.

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.

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?

I'm fine with this. But I haven't looked at the diff yet. I can do that tomorrow and green-light this PR.

@trishankatdatadog
Copy link
Member

@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?

Yes, this stuff is beautiful, let's go ahead. Someone else should double-check that we haven't lost anything in translation.

@joshuagl
Copy link
Member Author

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:
...

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?

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!

  • Does everyone distrust 3rd party actions, or is that just me? If we're only using actions to automate WIP changes, and opt to manually make signed spec releases (and push them to theupdateframework.io), I would personally be less anxious about using 3rd party actions.

Yes, I am not very happy about them either, and I did make some suggestion to mitigate this threat (theupdateframework/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.

💯 this is a fine idea.

@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?

I'm fine with this. But I haven't looked at the diff yet. I can do that tomorrow and green-light this PR.

Brilliant, thank you @lukpueh !

Copy link
Member

@lukpueh lukpueh left a 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.

tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Show resolved Hide resolved
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]>
Copy link
Member

@lukpueh lukpueh left a 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 the w=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)

tuf-spec.md Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
tuf-spec.md Outdated Show resolved Hide resolved
@trishankatdatadog
Copy link
Member

Here is the more thorough review as promised, which includes:

Spectacular review, thanks Lukas! 🙇🏽‍♂️

joshuagl and others added 13 commits January 21, 2021 12:29
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]>
* 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]>
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]>
@joshuagl
Copy link
Member Author

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;

  • added the extra cross-linking you suggested (7916026)
  • integrated your grammatical clarifications (6708a28), removed "where:" (6671fb4), and tried to ensure definitions are consistently capitalised (df33652)
  • fixed indentation in the examples (0c48395)
  • added a custom HTML header to change the document heading – adding version and removing status (3fa355b)
  • added a definition tag for the explanation of KEYs in the general principles section (3f5f5c0)
  • cleaned up some unnecessary discussion of the "keys" field in DELEGATION objects (9c8305f)
  • added a definition tag for HEX_DIGEST (2601e35)
  • consolidated remarks about KEYIDs in format principles (9d21a28)

@lukpueh
Copy link
Member

lukpueh commented Jan 22, 2021

High-quality work deserves a thorough review! :) Thanks for addressing my comments. I'll give it another pass on Monday.

@joshuagl
Copy link
Member Author

A rendered version of the specification following the latest round of changes is viewable here: https://joshuagl.github.io/specification/1.0.17/

@lukpueh
Copy link
Member

lukpueh commented Jan 25, 2021

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.

@joshuagl
Copy link
Member Author

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
Once this PR is merged I'll open another comparing that branch to the master branch so that we can review.

@joshuagl joshuagl requested a review from mnm678 January 25, 2021 15:07
Copy link
Collaborator

@mnm678 mnm678 left a comment

Choose a reason for hiding this comment

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

Thanks for the great work @joshuagl, and for the thorough review @lukpueh!

@joshuagl joshuagl mentioned this pull request Jan 27, 2021
6 tasks
@lukpueh lukpueh merged commit d3ee698 into theupdateframework:master Jan 27, 2021
@joshuagl joshuagl deleted the joshuagl/bikeshed-initial branch January 27, 2021 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants