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

ADR: Add New repository library design #1693

Merged
merged 3 commits into from
Dec 8, 2021

Conversation

jku
Copy link
Member

@jku jku commented Nov 24, 2021

Document the decision to build a repository library on top of Metadata
API.

This ADR has "attachments":

Comments on the ADR, design doc and prototype are welcome here.

Fixes #1679

Document the decision to build a repository library on top of Metadata
API.

Signed-off-by: Jussi Kukkonen <[email protected]>
@coveralls
Copy link

coveralls commented Nov 24, 2021

Pull Request Test Coverage Report for Build 1549291197

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.581%

Totals Coverage Status
Change from base Build 1548864112: 0.0%
Covered Lines: 4032
Relevant Lines: 4116

💛 - Coveralls

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for the great work on this Jussi!

I made a few suggestions to make some of the recommendations more explicit. The design doc looks good, I think one thing that might make it easier to understand is a diagram.

The design has 3 layers:

  1. an API with interfaces provided by python-tuf
  2. implementations of the interfaces provided by the application or python-tuf
  3. the application-specific repository code

A diagram showing the three layers and who owns which pieces could help folks to understand.

Also, can we translate the design doc to markdown and include it in the repo before we merge? That feels more durable than linking to a Google doc.

Aside: the design feels a lot like the delegation pattern, it reminds me a lot of how in Objective-C/NextStep derived frameworks allow you to plug in a delegate object that implements defined protocols (rather than having to subclass complex objects).

docs/adr/0010-repository-library-design.md Outdated Show resolved Hide resolved
docs/adr/0010-repository-library-design.md Outdated Show resolved Hide resolved
docs/adr/0010-repository-library-design.md Outdated Show resolved Hide resolved
docs/adr/0010-repository-library-design.md Outdated Show resolved Hide resolved
@jku
Copy link
Member Author

jku commented Nov 30, 2021

Also, can we translate the design doc to markdown and include it in the repo before we merge? That feels more durable than linking to a Google doc.

I think I'll do this (google docs are indeed a bit too ephemeral)... The reason I didn't and am still hesitant is that the document will become outdated. But maybe if I date it and specify that it is the attachment to the design ADR it's still valuable.

Copy link
Collaborator

@kairoaraujo kairoaraujo left a comment

Choose a reason for hiding this comment

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

My comments are small (irrelevant) details :)

docs/adr/0010-repository-library-design.md Outdated Show resolved Hide resolved
* Also add some new diagrams in the design doc
* Fix some issues in ADR

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
Copy link
Member Author

jku commented Dec 1, 2021

Comments have been addressed. The design doc is now in git, and two diagrams have been added*.

*) the issue with diagrams is that they are images so uneditable without my google slides original of course... so we may need to re-invent this for actual API docs when that time comes. Maybe ascii art would work as well?

@jku
Copy link
Member Author

jku commented Dec 1, 2021

@lukpueh @sechkova comments?

@rdimitrov
Copy link
Contributor

I have a few minor formatting suggestions, but other than that everything's in line 👍

  • In docs/adr/0010-repository-library-design.md, it might be more explicit if code-related namings (such as repository_tool, repo.py , etc) are also formatted in the form of <text>.
  • There are a few file names and examples mentioned (like GitRepository). I see there's a table with references at one place, but it'd be also nice if the text is an actual link so when clicked it redirects to that source file/repo.
  • The diagrams are not so complicated, so as you pointed using something like https://asciiflow.com/ might make it easier to edit

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.

Thanks for providing these documents, @jku! They should really help us move forward. The entire ADR-document and the "Design principles" in the Design document look perfectly fine to me.

Regarding the "Design"-section in the design document I must admit that I had to go through it several times to better understand. The "Details"-section definitely helps, but it was also not 100% obvious to me how your ideas/decision from the POC map to the overall design, especially in regards to what can/can't or should/shouldn't implemented on which level and why.

I made a few small suggestions that might help to clarify, but overall I'm still missing the preferably exhaustive list of specific functionality a minimal repository abstraction includes/leaves to application, and the underlying rationale.

### No repository packages

Metadata API makes editing the repository content vastly simpler. There are
already repository implementations built with it[^1] so clearly a repository
Copy link
Member

Choose a reason for hiding this comment

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

Hooray for GitHub Markdown footnotes! 🎉

docs/adr/0010-repository-library-design.md Show resolved Hide resolved
docs/adr/0010-repository-library-design.md Show resolved Hide resolved
docs/repository-library-design.md Outdated Show resolved Hide resolved

## Design

![Design: Application and library components](repository-library-design-ownership.jpg)
Copy link
Member

Choose a reason for hiding this comment

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

I was a bit confused by the word "generic" in "Generic App Code". Isn't this actually the code specific to an application? Maybe it's me, but I briefly thought that it means that it is something provided by python-tuf (generic and thus eligible for all applications).

Would "App management code" be a good choice of words?

(Same goes for the second diagram)

Copy link
Member Author

@jku jku Dec 1, 2021

Choose a reason for hiding this comment

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

yeah, "generic" was meant in the sense that "this code does not need to be part of the class implementing Repository interface"...

at any level_: the application might add a `handle_new_target_files()` method
that adds a bunch of targets into the metadata, but one of the previous layers
could offer that as a helper function as well: code in both cases would look
similar as it would use the common editing interface.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the design document make the decision on what layer such functionality should be implemented? Or at least explain based on what factors the decision should be made?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm proposing we start with the absolute minimum in the library: possibly just snapshot and sign implemented in the library itself. So the application is left with it's own design choices for the rest (should MyDerivedRepository offer helper functions or should the application code just use the edit() contextmanager itself -- the proof of concept goes with the latter choice)...

Nothing prevents us from moving some functionality into the library if that seems to make sense for multiple users but I don't see a reason to do that without seeing some implementations.

Copy link
Member

@lukpueh lukpueh Dec 2, 2021

Choose a reason for hiding this comment

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

Again, with your explanation here the document makes more sense. (cf. my reply above)

docs/repository-library-design.md Outdated Show resolved Hide resolved
docs/repository-library-design.md Outdated Show resolved Hide resolved
docs/repository-library-design.md Show resolved Hide resolved

Currently the identified improvement areas are:
* Metadata initialization: this could potentially be improved by adding
default argument values to Metadata API constructors
Copy link
Member

Choose a reason for hiding this comment

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

I see potential problems for our current deserialization implementation when providing default arguments, but we can discuss this separately.

Copy link
Member Author

@jku jku Dec 2, 2021

Choose a reason for hiding this comment

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

let's do that in #1459 (see #1459 (comment))

Possibly some from_dict() implementations would have to be extra careful not to pass deserialized None/null values to a constructor.

of the repository API: they could be part of Metadata API as well

It would make sense for python-tuf to ship with at least one concrete
Repository implementation: possibly a repo.py look alike. This implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to have this example concrete implementation support a CLI for demos and such.

Copy link
Member Author

@jku jku Dec 2, 2021

Choose a reason for hiding this comment

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

Yes. I didn't want to bloat these docs but I see a few potential implementations:

  • some sort of CLI tool, to be able to generate and modify metadata for demo and testing purposes
  • an example repository server (a "mini-warehouse"): a focused and opinionated repository example that could maybe include a custom web api for uploading new targets and a tiny developer tool to do the upload. It should be possible to make this into an example that runs out-of-the-box

Both of these could live inside or outside the repo... I'm not against the first one at all but I kind of feel the second one is even more important. The focus on CLI tools in TUF community (where every project seems to be a CLI tool) is not just a good thing IMO: it creates this impression that production repos can be shell scripts calling CLI tools**... which I don't think is such a great idea.

** yes, I realize I've written exactly this myself: don't do as I do, do as I say 😬

lukpueh added a commit to lukpueh/tuf that referenced this pull request Dec 2, 2021
Add 1.0.0 announcment document and point to it in main README.

TODO:
- Commit message
- PR (blocks on theupdateframework#1693, theupdateframework#1675, maybe theupdateframework#1700)
@lukpueh lukpueh mentioned this pull request Dec 3, 2021
3 tasks
@jku
Copy link
Member Author

jku commented Dec 7, 2021

I believe I've addressed the comments so far.

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.

Thanks for addressing the comments, @jku! I think the added information is very helpful. I found some typos and have one general request.

docs/repository-library-design.md Outdated Show resolved Hide resolved
docs/repository-library-design.md Outdated Show resolved Hide resolved
docs/repository-library-design.md Outdated Show resolved Hide resolved
docs/repository-library-design.md Outdated Show resolved Hide resolved
In addition to multiple smaller review fixes:
* Explain how the proposed library is minimal: more specific
  functionality may be added as we get more experience
* Explain what a concrete Repository implementation must implement
  (details are obviously subject to change but this is what the
  current prototype requires)

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku
Copy link
Member Author

jku commented Dec 7, 2021

Thanks: all suggestions make a lot of sense. I've moved the "abstract methods that must be implemented" part down to the Repository details section.

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.

Thanks for bearing with me and my many comments, @jku!

@lukpueh lukpueh merged commit b2d8572 into theupdateframework:develop Dec 8, 2021
@lukpueh lukpueh mentioned this pull request Dec 13, 2021
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.

Write ADR for repository library design
8 participants