From ebf5b670d4d5cbccad3e4c83105a2d53a297e8e2 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Sat, 23 Mar 2024 07:12:06 +1100 Subject: [PATCH 1/3] Update test script mention In the One ACK carve out just say "test scripts" instead of `test.sh` because we re-named the test scripts recently. --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 124bb284..f13a7ead 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -159,7 +159,7 @@ least two weeks with no comments, questions, or NACKs. We reserve the right to merge PRs with a single ACK [0], at any time, if they match any of the following conditions: -1. PR only touches CI i.e, only changes any of the `test.sh` scripts and/or +1. PR only touches CI i.e, only changes any of the test scripts and/or stuff in `.github/workflows`. 2. Non-content changing documentation fixes i.e., grammar/typos, spelling, full stops, capital letters. Any change with more substance must still get two From 42d02fbd66981fa20063986d0382e4857c8faf04 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Sat, 23 Mar 2024 07:17:52 +1100 Subject: [PATCH 2/3] Merge Refactor and One ACK carve outs The "One ACK carve-out" has 3 rules and then there is a separate "Refactor carve-out" that covers things that are not only refactoring - this makes it hard to reference the carve-outs in github because its a bit confusing. Merge the carve-outs into a single "one ACK carve-out" with multiple rules. Use rule 0 for the original refactor carve-out stuff because it makes the diff smaller and all good lists start with 0. Also remove mention of the refactor carve-out from rule 3. --- CONTRIBUTING.md | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f13a7ead..e1a0bc65 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -144,21 +144,18 @@ Current list of the project maintainers: - [Riccardo Casatta](https://github.com/RCasatta) - [Tobin Harding](https://github.com/tcharding) -#### Refactor carve-out +#### One ACK carve-out The repository is going through heavy refactoring and "trivial" API redesign (eg, rename `Foo::empty` to `Foo::new`) as we push towards API stabilization. As such reviewers are either bored or overloaded with notifications, hence we have created a carve out to the 2-ACK rule. -A PR may be considered for merge if it has a single ACK and has sat open for at -least two weeks with no comments, questions, or NACKs. - -#### One ACK carve-out - We reserve the right to merge PRs with a single ACK [0], at any time, if they match any of the following conditions: +0. PR has a single ACK and has sat open for at least two weeks with no comments, + questions, or NACKs. 1. PR only touches CI i.e, only changes any of the test scripts and/or stuff in `.github/workflows`. 2. Non-content changing documentation fixes i.e., grammar/typos, spelling, full @@ -166,8 +163,8 @@ any of the following conditions: ACKs. 3. Code moves that do not change the API e.g., moving error types to a private submodule and re-exporting them from the original module. Must not include - any code changes except to import paths. This rule is more restrictive than - the refactor carve-out. It requires absolutely no change to the public API. + any code changes except to import paths. Requires absolutely no change to the + public API. [0] - Obviously author and ACK'er must not be the same person. From 9b70c65f5d4d4c7ad9efe8ab6b834e941f9ac4bf Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Sat, 23 Mar 2024 07:27:42 +1100 Subject: [PATCH 3/3] Introduce new one-ack carve out rule Our merge process is being artificially slowed down because of a combination of: - Using merge-commit merging means PRs often have to be rebased with no changes but a different merge base (and force pushed). - Trivial changes, like fixing nits, are often force pushed also. - Force pushes invalidate ACKs - Our devs are spread around the world working at different times What this means is trivial force pushes often cause multi day delays in merging. To try and alleviate this problem introduce an additional rule to the One ACK carve-out so that Andrew can merge PRs that have previously been ack'ed by another dev and have only minimal changes. The definition of "trivial" is subjective which introduces a burden on Andrew to not merge stuff willy-nilly but also allows simple changes to the original PR (eg fixed nits that the original reviewer suggested). --- CONTRIBUTING.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e1a0bc65..722d22a9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -165,8 +165,17 @@ any of the following conditions: submodule and re-exporting them from the original module. Must not include any code changes except to import paths. Requires absolutely no change to the public API. +4. PR has previously had two ACKs, had minimal changes, and gets a single ACK + from Andrew. This call is subjective, gives extra privileges, but also + requires extra responsibility/accountability (including running a bunch + of local CI checks before merging) [1]. + + [0] - Obviously author and ACK'er must not be the same person. +[1] - The aim is to reduce the burden of re-ACK'ing trivial changes and also + alleviate the problem of devs spread around the world in different timezones. + ## Coding conventions