docs: update based on suggestions from code review
Co-Authored-By: Mike McQuaid <mike@mikemcquaid.com>
This commit is contained in:
parent
ba22ec6447
commit
46af59fe3a
@ -13,41 +13,41 @@ When reviewing a PR, use "comment", "approve", or "request changes" when submitt
|
||||
- Approve: if you feel that the PR is in a good state to be merged, even if there are
|
||||
non-blocking changes you'd like to be made
|
||||
- Request changes: if you feel strongly that the PR is likely to cause a problem for users or
|
||||
have a genuine reason to oppose the PR.
|
||||
have another reason to oppose the PR.
|
||||
|
||||
## Merging PRs
|
||||
|
||||
Merging should be done using the standard Merge button in the `Homebrew/brew` repository to preserve history and GPG commit signing. The Squash and Merge and Rebase and Merge buttons are disabled.
|
||||
Merging is be done using the standard Merge button in the `Homebrew/brew` repository to preserve
|
||||
history and GPG commit signing. The Squash and Merge and Rebase and Merge buttons are disabled.
|
||||
|
||||
PRs must meet the following conditions to be merged:
|
||||
|
||||
- Have at least one maintainer (or `BrewTestBot`) approval. See [below](#automatic-approvals)
|
||||
for more details about how `BrewTestBot` approves PRs.
|
||||
- Have at least one maintainer (or [@BrewTestBot](https://github.com/BrewTestBot)) approval.
|
||||
See the ["Automatic approvals" section below](#automatic-approvals).
|
||||
for more details about how [@BrewTestBot](https://github.com/BrewTestBot) approves PRs.
|
||||
- Have passing CI. This is a _mandatory_ step. PRs with failing CI should _never_ be merged.
|
||||
See [below](#ci) for more information about `Homebrew/brew` CI.
|
||||
See the ["CI" section below](#ci) for more information about `Homebrew/brew` CI.
|
||||
|
||||
If possible, PRs should also have:
|
||||
|
||||
- Linear commit history (i.e. no merge commits in PR branches).
|
||||
- GPG-signed commits (see the private `ops` repository for instructions on setting this up).
|
||||
If possible, PRs should also have GPG-signed commits (see the private `ops` repository for
|
||||
instructions on setting this up).
|
||||
|
||||
## Automatic approvals
|
||||
|
||||
To ensure that each PR has the opportunity to be seen and reviewed by any other maintainers who wish
|
||||
To ensure that non-urgent PRs have the opportunity to be seen and reviewed by any other maintainers who wish
|
||||
to take a look, all PRs require an approval before they can be merged. However, not every PR is
|
||||
reviewed by another maintainer, and some PRs are urgent enough that they need to be merged without
|
||||
an approval by another maintainer.
|
||||
|
||||
As a compromise between always needing a review and allowing maintainers to merge PRs they deem ready,
|
||||
the `Triage` CI job will ensure that PRs cannot be merged until they've been open for 24 hours
|
||||
(only counting hours that occur during the business week). After the triage period has expired, the
|
||||
CI job will show up as "passed" and `BrewTestBot` will approve the PR, allowing it to be merged.
|
||||
This gives all maintainers a reasonable opportunity to review every PR, but won't block any PR for lack
|
||||
of reviews.
|
||||
(only counting hours that occur Monday to Friday). After the triage period has expired, the
|
||||
CI job will show up as "passed" and [@BrewTestBot](https://github.com/BrewTestBot) will approve the PR,
|
||||
allowing it to be merged. This gives all maintainers a reasonable opportunity to review every PR,
|
||||
but won't block any PR for lack of reviews.
|
||||
|
||||
If the PR is urgent enough that it is necessary to bypass that 24 hour window, the `critical` label
|
||||
can be applied to the PR. When this label is applied, the `Triage` CI job will immediately be
|
||||
successful and `BrewTestBot` will approve the PR.
|
||||
should be applied to the PR. When this label is applied, the `Triage` CI job will immediately be
|
||||
successful and [@BrewTestBot](https://github.com/BrewTestBot) will approve the PR.
|
||||
|
||||
## CI
|
||||
|
||||
@ -62,7 +62,7 @@ There are many checks that run on every PR. The following is a quick list of the
|
||||
- `Triage / review`: This verifies that the PR has been open for long enough.
|
||||
See [above](#automatic-approvals) for more information about automatic approvals.
|
||||
- `codecov/patch` and `codecov/project`: These show the Codecov report for the PR.
|
||||
See [below](#brew-tests-and-codecov) for more info about Codecov.
|
||||
See the ["`brew tests` and Codecov" section below](#brew-tests-and-codecov) for more info about Codecov.
|
||||
- `CI / vendored gems (Linux)`: This checks whether there was a change to the vendored gems on Linux that needs to be
|
||||
committed to the PR branch.
|
||||
- `CI / test default formula (Linux)`: This runs `brew test-bot` on Linux to ensure it still works as expected.
|
||||
@ -71,11 +71,14 @@ There are many checks that run on every PR. The following is a quick list of the
|
||||
- `CI / tap syntax (Linux)`: This runs `brew style` and `brew audit` on all official taps
|
||||
(note that although this has Linux in its name, it does check `Homebrew/homebrew-core`,
|
||||
`Homebrew/linuxbrew-core` and all cask repos).
|
||||
- `CI / docker`: This builds and deploys a new Homebrew docker image.
|
||||
- `CI / test everything (macOS)`: This runs `brew tests` on macOS.
|
||||
- `CI / docker`: This builds and deploys a new Homebrew Docker image.
|
||||
- `CI / test everything (macOS)`: This runs several checks on macOS including `brew tests`, `brew update-tests`,
|
||||
`brew test-bot --only-formulae --test-default-formula`, `brew readall` and `brew doctor`.
|
||||
- `CI / tests (no-compatibility mode)`, `CI / tests (generic OS)` and `CI / tests (Linux)`: These run
|
||||
`brew tests` with various options on Linux.
|
||||
|
||||
_Note that this list is non-exhaustive and can change over time._
|
||||
|
||||
### `brew tests` and Codecov
|
||||
|
||||
A coverage report is generated by Codecov for every PR, and its results are shown as CI jobs.
|
||||
@ -93,8 +96,8 @@ but this should be avoided if possible.
|
||||
|
||||
Homebrew's manpages and shell completions are generated automatically by the `brew generate-man-completions` command.
|
||||
Contributors are welcome to run this command and commit the changes in a PR, but they don't have to. If they don't,
|
||||
a follow-up PR to make the necessary changes will be opened automatically by `BrewTestBot` once the original PR is
|
||||
merged. These follow-up PRs can be merged immediately if the changes seem correct.
|
||||
a follow-up PR to make the necessary changes will be opened automatically by [@BrewTestBot](https://github.com/BrewTestBot)
|
||||
once the original PR is merged. These follow-up PRs can be merged immediately if the changes seem correct.
|
||||
|
||||
An update can be requested manually by triggering the workflow from the
|
||||
[Update maintainers, manpage and completions](https://github.com/Homebrew/brew/actions/workflows/update-man-completions.yml)
|
||||
|
||||
@ -17,6 +17,7 @@ Here is a list of the most common situations that arise in cask PRs and how to h
|
||||
- The `version` and `sha256` both change (keeping the same format): Merge.
|
||||
- Only the `sha256` changes: Merge unless the version needs to be updated as well.
|
||||
It’s not uncommon for upstream vendors to update versions in-place.
|
||||
However, be wary for times when e.g. upstream could have been hacked.
|
||||
- `livecheck` is updated: Use your best judgement and try to make sure that the changes
|
||||
follow the [`livecheck` guidelines](Brew-Livecheck.md).
|
||||
- Only the `version` changes or the `version` format changes: Use your best judgement and
|
||||
@ -27,22 +28,10 @@ If in doubt, ask another cask maintainer on GitHub or Slack.
|
||||
|
||||
Note that unlike formulae, casks do not consider the `sha256` stanza to be a meaningful security measure
|
||||
as maintainers cannot realistically check them for authenticity. Casks download from upstream; if a malicious
|
||||
actor compromised a URL, they could just as well compromise a version and make it look like an update.
|
||||
actor compromised a URL, they could potentially compromise a version and make it look like an update.
|
||||
|
||||
## Merging
|
||||
|
||||
### Approvals
|
||||
|
||||
Most PRs in the cask repositories are simple version bumps that don't necessarily need
|
||||
another maintainer's approval. However, GitHub will not allow a PR to be merged without
|
||||
at least one approving review. To bypass this requirement if necessary, a maintainer
|
||||
can self-approve one of their PRs using the `self-approve` GitHub Actions workflow to
|
||||
satisfy this requirement. To trigger a self-approval, navigate to the
|
||||
["Self-approve a Pull Request" section of the Actions tab](https://github.com/Homebrew/homebrew-cask/actions/workflows/self-approve.yml),
|
||||
click on "Run workflow", enter the PR number and click "Run workflow".
|
||||
|
||||
### Merge Types
|
||||
|
||||
In general, using GitHub's Squash and Merge button is the best way to merge a PR. This can be used when
|
||||
the PR modifies only one cask, regardless of the number of commits or whether the commit message
|
||||
format is correct. When merging using this method, the commit message can be modified if needed.
|
||||
|
||||
@ -13,7 +13,8 @@ A detailed checklist can be found [below](#detailed-merge-checklist). This is al
|
||||
[pip](https://pip.pypa.io/en/stable/).
|
||||
- Ensure that any dependencies are accurate and minimal. We don't need to
|
||||
support every possible optional feature for the software.
|
||||
- When bottles aren't required or affected, use the GitHub squash & merge workflow for a single-formula PR or rebase & merge workflow for a multiple-formulae PR. See [below](#how-to-merge-without-bottles) for more details.
|
||||
- When bottles aren't required or affected, use the GitHub squash & merge workflow for a single-formula PR or rebase & merge workflow for a multiple-formulae PR. See the ["How to merge without bottles" section below](#how-to-merge-without-bottles)
|
||||
for more details.
|
||||
- Use `brew pr-publish` or `brew pr-pull` otherwise, which adds messages to auto-close pull requests and pull bottles built by the Brew Test Bot.
|
||||
- Thank people for contributing.
|
||||
|
||||
@ -26,26 +27,12 @@ For example, we build Wireshark, but not the heavy GUI.
|
||||
Homebrew is about Unix software. Stuff that builds to an `.app` should
|
||||
be in Homebrew Cask instead.
|
||||
|
||||
## Naming
|
||||
|
||||
The name is the strictest item, because avoiding a later name change is
|
||||
desirable.
|
||||
|
||||
Choose a name that’s the most common name for the project.
|
||||
For example, we initially chose `objective-caml` but we should have chosen `ocaml`.
|
||||
Choose what people say to each other when talking about the project.
|
||||
|
||||
Formulae that are also packaged by other package managers (e.g. Debian, Ubuntu) should be
|
||||
named consistently (subject to minor differences due to Homebrew formula naming conventions).
|
||||
|
||||
Add other names as aliases as symlinks in `Aliases` in the tap root. Ensure the
|
||||
name referenced on the homepage is one of these, as it may be different and have
|
||||
underscores and hyphens and so on.
|
||||
|
||||
We now accept versioned formulae as long as they [meet the requirements](Versions.md).
|
||||
|
||||
## Merging, rebasing, cherry-picking
|
||||
|
||||
In most cases, you can simply approve a PR and an automatic merge (with bottles)
|
||||
will be performed by [@BrewTestBot](https://github.com/BrewTestBot).
|
||||
See [Brew Test Bot For Core Contributors](Brew-Test-Bot-For-Core-Contributors.md) for more information.
|
||||
|
||||
PRs modifying formulae that don't need bottles or making changes that don't
|
||||
require new bottles to be pulled should use GitHub's squash & merge or rebase & merge workflows.
|
||||
See the [table below](#how-to-merge-without-bottles) for more details.
|
||||
@ -70,13 +57,31 @@ Only one maintainer is necessary to approve and merge the addition of a new or u
|
||||
|
||||
### How to merge without bottles
|
||||
|
||||
Here are guidelines about when to use squash & merge versus rebase & merge. These options should only be used with PRs where bottles are not needed or affected.
|
||||
Here are guidelines about when to use squash & merge versus rebase & merge. These options should only be used with PRs where bottles are not affected.
|
||||
|
||||
| | PR modifies a single formula | PR modifies multiple formulae |
|
||||
|---|---|---|
|
||||
| **Commits look good** | rebase & merge _or_ squash & merge | rebase & merge |
|
||||
| **Commits need work** | squash & merge | manually merge using the command line |
|
||||
|
||||
## Naming
|
||||
|
||||
The name is the strictest item, because avoiding a later name change is
|
||||
desirable.
|
||||
|
||||
Choose a name that’s the most common name for the project.
|
||||
For example, we initially chose `objective-caml` but we should have chosen `ocaml`.
|
||||
Choose what people say to each other when talking about the project.
|
||||
|
||||
Formulae that are also packaged by other package managers (e.g. Debian, Ubuntu) should be
|
||||
named consistently (subject to minor differences due to Homebrew formula naming conventions).
|
||||
|
||||
Add other names as aliases as symlinks in `Aliases` in the tap root. Ensure the
|
||||
name referenced on the homepage is one of these, as it may be different and have
|
||||
underscores and hyphens and so on.
|
||||
|
||||
We now accept versioned formulae as long as they [meet the requirements](Versions.md).
|
||||
|
||||
## Testing
|
||||
|
||||
We need to at least check that it builds. Use the [Brew Test Bot](Brew-Test-Bot.md) for this.
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user