Define and document review expectations

[joshuagl]

We have various outstanding pull requests which have been approved by one or more of the TAP editors (#112, #122, #125, and #127).

From the TUF community meeting today, it appears that nobody knows quite what the expectations are for review and acceptance of specification changes.

Given the security sensitive nature of the project, it makes sense that pull requests would require a lengthy enough review period that the implications of a change can be reasoned about before the code is merged. However it is important for contributors to understand what the review process is and how proposed changes may eventually end up merged.

I propose we define a review standard review process which includes a number of reviewers and a contemplation period ensuring others have chance to comment. Then we should clearly document that process.

[lukpueh]

Absolutely agreed! Regardless, I think there is a rather practical reason why these PRs have not been merged (at least why I haven't), that is the slightly onerous follow-up work required after a merge, i.e. tag release, rebase draft branch and pending PRs on master resolving conflicts. Parts of this we intend to automate in #95. (cc @erickt how seems to have researched other specification processes and might have good recommendations)

[lukpueh]

It seems like we need to also better define the following items of the release management guide:

• For major- and minor-type changes, pull requests must be submitted against the 'draft' branch.
• Maintainers may, from time to time, decide that the 'draft' branch is ready for a new major or minor release, and submit a pull request from 'draft' against 'master'.

We currently have one merged PR (#57) in 'draft', which will require a major version bump, and one already approved but still pending PR (#122) with base 'draft', which will require a minor version bump.

I think we need something like a timeline, or clearer cues for when to do minor and major bumps and how or if to aggregate them.

Apologies if this diverges from the OP...

[mnm678]

I propose we define a review standard review process which includes a number of reviewers and a contemplation period ensuring others have chance to comment. Then we should clearly document that process.

This is a great idea. I would add a few comments:

• We should probably require at least one approving review from a TAP editor.
• Does the process change if there are some approving reviews and some requests for changes?
• We may want to allow very small changes (typo fixes, etc) more quickly if there is a safe/easy way to identify these.

[trishankatdatadog]

• We should probably require at least one approving review from a TAP editor.

I would go so far as 2 at least to set a high bar.

[joshuagl]

Absolutely agreed! Regardless, I think there is a rather practical reason why these PRs have not been merged (at least why I haven't), that is the slightly onerous follow-up work required after a merge, i.e. tag release, rebase draft branch and pending PRs on master resolving conflicts.

I know you h ave done this work a couple of times, but want to ask the question anyway. Do we really expect the maintainer merging a commit to rebase pending PRs on master? Usually the onus for updating a PR would fall to the contributor?

[joshuagl]

It seems like we need to also better define the following items of the release management guide:

• For major- and minor-type changes, pull requests must be submitted against the 'draft' branch.
• Maintainers may, from time to time, decide that the 'draft' branch is ready for a new major or minor release, and submit a pull request from 'draft' against 'master'.

We currently have one merged PR (#57) in 'draft', which will require a major version bump, and one already approved but still pending PR (#122) with base 'draft', which will require a minor version bump.

I think we need something like a timeline, or clearer cues for when to do minor and major bumps and how or if to aggregate them.

Agreed. That would be very useful. I infer from past conversations that we are defining major changes as those which would break backwards compatibility, and further that we would like to batch multiple changes into one major release (rather than quickly merging breaking changes and making releases).

I also think we are blocked from making a major release until TAP 14 is accepted.

Perhaps, in addition to better defining review process and more clearly defining minor vs major changes, we should create a roadmap for TUF 2.0. This roadmap could collect what changes we want in the next major version of the TUF specification and what things are blocking us from making that release (TAP 14, which may be blocked on the python-tuf refactor – so that we can implement a PoC).

Apologies if this diverges from the OP...

Not at all, I think this is very on-topic.

[joshuagl]

I propose we define a review standard review process which includes a number of reviewers and a contemplation period ensuring others have chance to comment. Then we should clearly document that process.

This is a great idea. I would add a few comments:

• We should probably require at least one approving review from a TAP editor.

Agreed. I was thinking TAP editors ~= reviewers, but we should absolutely be explicit here. At least two TAP editors seems prudent. We will need to have a list of TAP editors somewhere to ensure this process is transparent to contributors.

• Does the process change if there are some approving reviews and some requests for changes?

Great question. I think any request for changes should be discussed, and an outcome of the request agreed upon, before the changes land.

• We may want to allow very small changes (typo fixes, etc) more quickly if there is a safe/easy way to identify these.

Yes, that may be useful. Though adding escape hatches often results in unintended consequences. We should be clear in defining when the consideration period does not apply.

[erickt]

Do we have a formal list of people who are TAP editors?

One option we could adopt (or fork) rust-lang's https://github.com/rust-lang/rfcbot-rs, which automates their process for making changes to the rust language. For example, see rust-lang/rfcs#2963, where there's a formal process for calling for approval of a change, and the bot asks people that are part of the relevant team to vote to merge something in. I'm not sure how we would run it here though. Perhaps bots like this can be run as github actions?

Or we could just adopt a process like this by hand, since we really don't have that much churn on the spec.

[mnm678]

Do we have a formal list of people who are TAP editors?

I thought we did, but I don't see it on the repository. There's an outdated list of maintainers of the specification in the README, but a more formal MAINTAINERS.txt or something might be a better place to store this.

[joshuagl]

I took a first stab at documenting a process (and maintainers) in #132

While I like the idea of using something like Rust's rfcbot, or Kubernetes' prow, I thought it might be too much overhead for a small team. Please take a look at the PR and let me know what you think.

[joshuagl]

Marking as closed by #132. We have #95 to track some of our tooling requirements for automation.