nonspec: Make requirements for draft reviews more explicit #1201
Conversation
Currently CONTRIBUTING.md says that PRs that affect draft files have 'relaxed' approval requirements but does not specify what those relaxed requirements are. This PR makes the default requirements for draft PRs explicit (only 1 reviewer, no waiting period) in an attempt to speed progress on draft changes. Approving reviewers will have the opportunity to indicate if they want additional reviewers prior to submitting these draft PRs. We will still have strict requirements (3 approvers, 72h waiting period) when moving content from draft to rc or final status. This should allow thorough review of changes in their finished state without hampering progress on new tracks. Signed-off-by: Tom Hennen <tomhennen@google.com>
TomHennen
requested review from
MarkLodato,
lehors,
arewm,
trishankatdatadog,
zachariahcox,
marcelamelara and
a team
✅ Deploy Preview for slsa ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
No strong opinions. I can see where this PR is coming from, but maybe two instead of three? This is so that the big reviews later going from draft to stable is not too surprising, but two would still be slower than one. |
Yeah that's definitely the tradeoff. I'd hope that the first approver could recognize situations like that that may require additional guidance and indicate it? It think that's generally the guidance at the moment. "up to Maintainer discretion" isn't quite specific enough about who that maintainer is. Should the PR author be allowed to make this distinction? That might also help with the goal without being as prescriptive. E.g. We could say "up to Maintainer's discretion (including the PR author if they're a maintainer)" Then the author could decide if they want to take the 'risk' of merging something that might generate a lot more feedback later. |
Yes, pls, thx! |
Signed-off-by: Tom Hennen <tomhennen@google.com>
Done! |
There was a problem hiding this comment.
Thanks @TomHennen ! I'm sure we'll run into some special cases, but this seems like it covers the most general ones.
|
For what it's worth, I read the original text as meaning that for Drafts there are no requirements per se and "up to Maintainer discretion", in which case there is no need to define the requirements any further. But I'm happy with the suggested change. |
hah! I'd been reading it as somewhat more restrictive, like I (as the PR author) would need to get explicit agreement with other Maintainer's to submit without all the documented approvals. |
It isn't checked automatically. I could add that 'draft' tag as a requirement. Not sure how to check it automatically though. Would we want just 'draft:' or like I've been doing "[whatever]: draft: [description]" |
There was a problem hiding this comment.
Are there any requirements that would be imposed if we added draft as an option here? https://github.com/slsa-framework/slsa/blob/main/.github/workflows/conventional-commits.yml#L22-L29
Do we have any process documented for moving content out of draft to ensure that appropriate reviews are given?
Signed-off-by: Tom Hennen <tomhennen@google.com>
I think that just checks that the right prefix was added but doesn't do anything else AFAIK. Maybe someone else does?
We do! It's discussed here. |
|
Ok, I think we have enough approvals and waited long enough, so I'm going to merge. Thanks all! |
Currently CONTRIBUTING.md says that PRs that affect draft files have 'relaxed' approval requirements but does not specify what those relaxed requirements are.
This PR makes the default requirements for draft PRs explicit (only 1 reviewer, no waiting period) in an attempt to speed progress on draft changes. Approving reviewers will have the opportunity to indicate if they want additional reviewers prior to submitting these draft PRs.
We will still have strict requirements (3 approvers, 72h waiting period) when moving content from draft to rc or final status. This should allow thorough review of changes in their finished state without hampering progress on new tracks.