[bisq-network/proposals] Add additional fields to PRs and Commits to streamline reviews, merges, and releases (#145)

[Julian Knutsen]

I wanted to share some feedback and potential solutions for some deficiencies in the current PR and commit workflow that I've seen in my time with Bisq. I think adopting more standard practices will help increase transparency and reduce overhead for reviewers, maintainers, and authors.  I also think it will help with scaling the development team until review capacity increases with the onboarding of new developers.

I don't want to pollute `Proposals` with each one individually so hopefully, some consensus can be reached if other people would find any of these changes valuable.

My recommendations can be found [here](#recommendations).

## Motivation
1. Looking at a given PR, the priority of a change is not clear. This makes it hard for reviewers and maintainers to prioritize their time and can lead to important changes getting delayed. This is especially harmful if the PR fixes a recent regression in `master` and it not prioritized.

2. Looking at a given PR, it isn't clear if it needs to make it into a release branch causing additional back-and-forth around reviews.

3. Looking at a given PR with one or more utAck or Ack, it isn't clear if/when it will be merged causing unnecessary delays and ownership confusion.

4. Looking at a given merge-commit, it is hard to understand the change because the text is created from the author-defined branch name, not the PR or commit text.

5. Looking at a given commit, it isn't clear who signed off on the change.

6. Looking at a given closed issue, it isn't clear if the fix has been validated by someone other than the author to verify it is fixed.

## How does Bitcoind handle these issues?
**Priority**
Bitcoind uses a `Bug` label that can be added by the author. This allows reviewers and maintainers to use GitHub filtering to identify high-risk bugs.

**Branch Target**
Bitcoind uses the Milestone feature that can be added by an author to identify issues that are targetted for a known release. This allows release tracking for the maintainers as well as an additional way to filter for reviewers who should prioritize soon-to-be-released code first.

**Current PR Responsibility**
Bitcoind uses a special label `Waiting for author` to specify that the current PR is blocked waiting
for some information/updates from the author. This allows reviewers to ignore the PR until it is updated and also allows authors to know that reviewers are waiting on them.

**Merge Commit Text**
Maintainers rename the merge commits to be more descriptive of the change. [Example](https://github.com/bitcoin/bitcoin/commit/d8a66626d63135fd245d5afc524b88b9a94d208b). This is similar to what would be required with a squash-merge, but still allows the two-parent commit which is great for bisecting.

Specifically, they require ACK `<commit-hash>` so that an automated script can fill in the commit text.

**Commit Message Information**
Bitcoind has specific fields on merge-commits that list the ACKs for a given merge. [ACKs for top commit example](https://github.com/bitcoin/bitcoin/commit/d8a66626d63135fd245d5afc524b88b9a94d208b)

This allows developers who are bisecting failures to inform the right people when a bad commit is found.

**Bug Closing**
All ACKs are required to have what testing was performed during the review to help validate that the bugs are actually fixed. That way, when issues are auto-closed by `fixes #<issue>` the testing has already been performed so there is no gap in quality.

This also helps when bisecting failures since it is easy to audit what testing was done prior to the merge.

## <a id="recommendation"/>Proposal ##
* Add and require milestone tags for PRs that need to make it into a release branch. No milestone means `master`. Maintainers know exactly what needs to be cherry-picked into release branches without unnecessary back-and-forth.

* Add and require a `bug` tag for all PRs that fix bugs so they can be audited quickly and prioritized appropriately.

* Any PRs that fix regressions caused by recent work in `master` need to be specifically called out in `#dev` so other developers and maintainers can see important fixes and not waste time on known-buggy code.

* Require maintainers to comment on PRs quickly (within 2 business days?) of a pull request with the expected timeline for review. This will set expectations around releases and contribution request times around what can/can't make it in.

* Use `Waiting for author` or equivalent on PRs.
  * `Waiting for Author` means the responsibility is on the PR author to push it forward
  * If a PR has one utAck, but multiple are required, the first utAck reviewer should specifically state the party responsible for pushing the PR forward

* Add `signed-off-by` text in the merge commit messages and require ACKs to list the testing performed prior to signing off. This will make it more clear who is responsible for changes and confirm that bugs are fixed prior to integration. I also expect the testing requirement to catch more bad code prior to merging.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/proposals/issues/145
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191127/8a81a739/attachment-0001.html>