From ee23c492dee9ed13d04ccf6369c643d659cb3973 Mon Sep 17 00:00:00 2001 From: Igor Kapkov Date: Wed, 11 Sep 2019 20:24:41 +1000 Subject: [PATCH 1/3] Initial draft of the checklist for PR merge decisions --- docs/Formula-Merge-Checklist.md | 55 +++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 docs/Formula-Merge-Checklist.md diff --git a/docs/Formula-Merge-Checklist.md b/docs/Formula-Merge-Checklist.md new file mode 100644 index 0000000000000..702527d477ca5 --- /dev/null +++ b/docs/Formula-Merge-Checklist.md @@ -0,0 +1,55 @@ +# Formula Merge Checklist + +The following checklist is intended to help maintainers to decide on +whether to merge, request changes or close a PR. It also brings more +transparency for contributors in addition to +[Acceptable Formulae](Acceptable-Formulae.md) requirements. + +This is a guiding principle. As a maintainer, you can make a call on either +request changes from a contributor or help them out based on their comfort and +previous contributions. Remember, as a team we +[Prioritise Maintainers Over Users](Maintainers-Avoiding-Burnout.md) to avoid +burnout. + +This is a more practical checklist, it should be used after you get familiar with +[Maintainer Guidelines](Maintainer-Guidelines.md). + +## Checklist + +Check for: + +- previously opened active PRs, we would like to be fair to contributors who came first; +- patches/`inreplace` that been applied to upstream and can be removed; +- comments in formula around `url`, we do skip some version (for example [vim](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/vim.rb#L4) or [v8](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/v8.rb#L4)); +- vendored resources that need updates (for example [emscripten](https://github.com/Homebrew/homebrew-core/commit/57126ac765c3ac5201ce53bcdebf7a0e19071eba)); +- vendored dependencies (for example [certbot](https://github.com/Homebrew/homebrew-core/pull/42966/files)) +- stable/announced release + - some teams use odd minor release number for tests and even for stable releases; + - other teams drop new version with minor release 0 but promote it to stable only after few minor releases; + - if the software uses only hosted version control (such as github, gitlab or bitbucket), the release should be tagged and if upstream marks latest/pre-releases, PR must use latest; +- does changelog mention addition/removal of dependency and it's addressed in the PR; + - does formula depend on versioned formula (for example `python@2`, `go@1.10`, `erlang@17`) that can be upgraded; +- commits + - contain one formula change per commit; + - version update follows preferred message format for simple version updates `foobar 7.3` + - other fixes format is `foobar: fix flibble matrix` + - you can use `--bump` flag for `brew pull` in case PR have single commit but the wrong message. +- bottle block is not removed; + Suggested reply: + ``` + Please keep bottle block in place, [@BrewTestBot](https://github.com/BrewTestBot) takes care of it. + ``` +- is there are test block for other than checking version or printing help? Consider asking to add one; +- if CI failed + - due to test block - paste relevant lines and add `test failure` label; + - due to build errors - paste relevant lines and add `build failure` label; + - due to other formulas need revision bumps - suggest to use the following command: + ``` + # in this example PR is for `libuv` formula and `urbit` needs revision bump + brew bump-revision --message 'for libuv' urbit + ``` +- if CI is green and formula `bottle :unneeded` you can merge it through GitHub UI; +- if CI is green and bottles need to be pulled, use: `brew pull --bottle $PR_ID` +- don't forget to thank the contributor + - celebrate the first-time contributors +- suggest to use `brew bump-formula-pr` next time if this was not a case; From 2ec526609c321883e0b2349e1a9e51586e6d4345 Mon Sep 17 00:00:00 2001 From: Igor Kapkov Date: Thu, 12 Sep 2019 07:36:17 +1000 Subject: [PATCH 2/3] Add link, rename file and remove semicolons --- ...Homebrew-homebrew-core-Merge-Checklist.md} | 36 +++++++++---------- docs/README.md | 1 + 2 files changed, 19 insertions(+), 18 deletions(-) rename docs/{Formula-Merge-Checklist.md => Homebrew-homebrew-core-Merge-Checklist.md} (84%) diff --git a/docs/Formula-Merge-Checklist.md b/docs/Homebrew-homebrew-core-Merge-Checklist.md similarity index 84% rename from docs/Formula-Merge-Checklist.md rename to docs/Homebrew-homebrew-core-Merge-Checklist.md index 702527d477ca5..7921277a83be1 100644 --- a/docs/Formula-Merge-Checklist.md +++ b/docs/Homebrew-homebrew-core-Merge-Checklist.md @@ -1,4 +1,4 @@ -# Formula Merge Checklist +# Homebrew/homebrew-core Merge Checklist The following checklist is intended to help maintainers to decide on whether to merge, request changes or close a PR. It also brings more @@ -18,38 +18,38 @@ This is a more practical checklist, it should be used after you get familiar wit Check for: -- previously opened active PRs, we would like to be fair to contributors who came first; -- patches/`inreplace` that been applied to upstream and can be removed; -- comments in formula around `url`, we do skip some version (for example [vim](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/vim.rb#L4) or [v8](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/v8.rb#L4)); -- vendored resources that need updates (for example [emscripten](https://github.com/Homebrew/homebrew-core/commit/57126ac765c3ac5201ce53bcdebf7a0e19071eba)); +- previously opened active PRs, we would like to be fair to contributors who came first +- patches/`inreplace` that been applied to upstream and can be removed +- comments in formula around `url`, we do skip some version (for example [vim](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/vim.rb#L4) or [v8](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/v8.rb#L4)) +- vendored resources that need updates (for example [emscripten](https://github.com/Homebrew/homebrew-core/commit/57126ac765c3ac5201ce53bcdebf7a0e19071eba)) - vendored dependencies (for example [certbot](https://github.com/Homebrew/homebrew-core/pull/42966/files)) - stable/announced release - - some teams use odd minor release number for tests and even for stable releases; - - other teams drop new version with minor release 0 but promote it to stable only after few minor releases; - - if the software uses only hosted version control (such as github, gitlab or bitbucket), the release should be tagged and if upstream marks latest/pre-releases, PR must use latest; -- does changelog mention addition/removal of dependency and it's addressed in the PR; - - does formula depend on versioned formula (for example `python@2`, `go@1.10`, `erlang@17`) that can be upgraded; + - some teams use odd minor release number for tests and even for stable releases + - other teams drop new version with minor release 0 but promote it to stable only after few minor releases + - if the software uses only hosted version control (such as github, gitlab or bitbucket), the release should be tagged and if upstream marks latest/pre-releases, PR must use latest +- does changelog mention addition/removal of dependency and it's addressed in the PR + - does formula depend on versioned formula (for example `python@2`, `go@1.10`, `erlang@17`) that can be upgraded - commits - - contain one formula change per commit; + - contain one formula change per commit - version update follows preferred message format for simple version updates `foobar 7.3` - other fixes format is `foobar: fix flibble matrix` - - you can use `--bump` flag for `brew pull` in case PR have single commit but the wrong message. -- bottle block is not removed; + - you can use `--bump` flag for `brew pull` in case PR have single commit but the wrong message +- bottle block is not removed Suggested reply: ``` Please keep bottle block in place, [@BrewTestBot](https://github.com/BrewTestBot) takes care of it. ``` -- is there are test block for other than checking version or printing help? Consider asking to add one; +- is there are test block for other than checking version or printing help? Consider asking to add one - if CI failed - - due to test block - paste relevant lines and add `test failure` label; - - due to build errors - paste relevant lines and add `build failure` label; + - due to test block - paste relevant lines and add `test failure` label + - due to build errors - paste relevant lines and add `build failure` label - due to other formulas need revision bumps - suggest to use the following command: ``` # in this example PR is for `libuv` formula and `urbit` needs revision bump brew bump-revision --message 'for libuv' urbit ``` -- if CI is green and formula `bottle :unneeded` you can merge it through GitHub UI; +- if CI is green and formula `bottle :unneeded` you can merge it through GitHub UI - if CI is green and bottles need to be pulled, use: `brew pull --bottle $PR_ID` - don't forget to thank the contributor - celebrate the first-time contributors -- suggest to use `brew bump-formula-pr` next time if this was not a case; +- suggest to use `brew bump-formula-pr` next time if this was not a case diff --git a/docs/README.md b/docs/README.md index fcda63826dc60..a346a962f72c2 100644 --- a/docs/README.md +++ b/docs/README.md @@ -51,6 +51,7 @@ - [Common Issues for Maintainers](Common-Issues-for-Core-Contributors.md) - [Releases](Releases.md) - [Developer/Internal API Documentation](https://rubydoc.brew.sh) +- [Homebrew/homebrew-core merge checklist](Homebrew-homebrew-core-Merge-Checklist.md) ## Governance From 1cd5fdf3e16c4f24bf6f812a459e48ca4101f2fa Mon Sep 17 00:00:00 2001 From: Igor Kapkov Date: Thu, 12 Sep 2019 19:50:01 +1000 Subject: [PATCH 3/3] Address comments --- .../Homebrew-homebrew-core-Merge-Checklist.md | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/docs/Homebrew-homebrew-core-Merge-Checklist.md b/docs/Homebrew-homebrew-core-Merge-Checklist.md index 7921277a83be1..76416865e75ca 100644 --- a/docs/Homebrew-homebrew-core-Merge-Checklist.md +++ b/docs/Homebrew-homebrew-core-Merge-Checklist.md @@ -1,55 +1,59 @@ # Homebrew/homebrew-core Merge Checklist -The following checklist is intended to help maintainers to decide on +The following checklist is intended to help maintainers decide on whether to merge, request changes or close a PR. It also brings more transparency for contributors in addition to [Acceptable Formulae](Acceptable-Formulae.md) requirements. -This is a guiding principle. As a maintainer, you can make a call on either +This is a guiding principle. As a maintainer, you can make a call to either request changes from a contributor or help them out based on their comfort and previous contributions. Remember, as a team we [Prioritise Maintainers Over Users](Maintainers-Avoiding-Burnout.md) to avoid burnout. -This is a more practical checklist, it should be used after you get familiar with +This is a more practical checklist; it should be used after you get familiar with [Maintainer Guidelines](Maintainer-Guidelines.md). ## Checklist Check for: -- previously opened active PRs, we would like to be fair to contributors who came first -- patches/`inreplace` that been applied to upstream and can be removed -- comments in formula around `url`, we do skip some version (for example [vim](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/vim.rb#L4) or [v8](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/v8.rb#L4)) +- previously opened active PRs, as we would like to be fair to contributors who came first +- patches/`inreplace` that have been applied to upstream and can be removed +- comments in formula around `url`, as we do skip some versions (for example [vim](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/vim.rb#L4) or [v8](https://github.com/Homebrew/homebrew-core/blob/359dbb190bb3776c4d6a1f603a271dd8f186f077/Formula/v8.rb#L4)) - vendored resources that need updates (for example [emscripten](https://github.com/Homebrew/homebrew-core/commit/57126ac765c3ac5201ce53bcdebf7a0e19071eba)) - vendored dependencies (for example [certbot](https://github.com/Homebrew/homebrew-core/pull/42966/files)) - stable/announced release - some teams use odd minor release number for tests and even for stable releases - - other teams drop new version with minor release 0 but promote it to stable only after few minor releases - - if the software uses only hosted version control (such as github, gitlab or bitbucket), the release should be tagged and if upstream marks latest/pre-releases, PR must use latest -- does changelog mention addition/removal of dependency and it's addressed in the PR + - other teams drop new version with minor release 0 but promote it to stable only after a few minor releases + - if the software uses only hosted version control (such as GitHub, GitLab or Bitbucket), the release should be tagged and if upstream marks latest/pre-releases, PR must use latest +- does changelog mention addition/removal of dependency and is it addressed in the PR - does formula depend on versioned formula (for example `python@2`, `go@1.10`, `erlang@17`) that can be upgraded - commits - contain one formula change per commit - - version update follows preferred message format for simple version updates `foobar 7.3` + - ask author to squash + - rebase during merge + - version update follows preferred message format for simple version updates: `foobar 7.3` - other fixes format is `foobar: fix flibble matrix` - - you can use `--bump` flag for `brew pull` in case PR have single commit but the wrong message + - you can use `--bump` flag for `brew pull` in case the PR have a single commit but the wrong message - bottle block is not removed + Suggested reply: ``` Please keep bottle block in place, [@BrewTestBot](https://github.com/BrewTestBot) takes care of it. ``` -- is there are test block for other than checking version or printing help? Consider asking to add one +- is there a test block for other than checking version or printing help? Consider asking to add one - if CI failed - due to test block - paste relevant lines and add `test failure` label - due to build errors - paste relevant lines and add `build failure` label - - due to other formulas need revision bumps - suggest to use the following command: + - due to other formulae needing revision bumps - suggest to use the following command: ``` # in this example PR is for `libuv` formula and `urbit` needs revision bump brew bump-revision --message 'for libuv' urbit ``` + - make sure it is one commit per revision bump - if CI is green and formula `bottle :unneeded` you can merge it through GitHub UI - if CI is green and bottles need to be pulled, use: `brew pull --bottle $PR_ID` - don't forget to thank the contributor - celebrate the first-time contributors -- suggest to use `brew bump-formula-pr` next time if this was not a case +- suggest to use `brew bump-formula-pr` next time if this was not the case