docs: update based on suggestions from code review
Co-Authored-By: Eric Knibbe <3324775+EricFromCanada@users.noreply.github.com>
This commit is contained in:
Rylan Polster
parent
04f5433dcc
commit
de5fd4642d
@ -7,10 +7,13 @@ be aware of, but don't necessarily need to appear in documentation for most user
|
||||
|
||||
Using `gh pr checkout NUMBER` is a super easy way to check out a PR branch using the GitHub CLI.
|
||||
|
||||
When reviewing, choose the "comment" type for reviews when the PR isn't quite ready to be merged.
|
||||
Use the "approve" type when 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. Use the "request changes" type if you feel strongly
|
||||
that the PR is likely to cause a problem for users or have a genuine reason to oppose the PR.
|
||||
When reviewing a PR, use "comment", "approve", or "request changes" when submitting based on the following guidelines:
|
||||
|
||||
- Comment: if the PR isn't quite ready to be merged
|
||||
- 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.
|
||||
|
||||
## Merging PRs
|
||||
|
||||
@ -20,13 +23,13 @@ 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.
|
||||
- Passing CI. This is a _mandatory_ step. PRs with failing CI should _never_ be merged.
|
||||
- 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.
|
||||
|
||||
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)
|
||||
- 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).
|
||||
|
||||
## Automatic approvals
|
||||
|
||||
@ -56,27 +59,28 @@ There are many checks that run on every PR. The following is a quick list of the
|
||||
- `Vendor Gems / vendor-gems`: This is skipped except for dependabot PRs. It updates the RBI files to match
|
||||
any new/changed dependencies. See [Type Checking With Sorbet](Typechecking.md) for more information about RBI files
|
||||
and typechecking.
|
||||
- `Triage / review`: This controls whether the PR has been open for long enough or not.
|
||||
- `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
|
||||
- `CI / vendored gems (Linux)`: This checks whether there was a change to the venered gems on Linux that needs to be
|
||||
See [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` Linux to ensure it still works as expected.
|
||||
- `CI / test default formula (Linux)`: This runs `brew test-bot` on Linux to ensure it still works as expected.
|
||||
- `CI / syntax`: This is run first to check whether the PR passes `brew style` and `brew typecheck`. If this job fails the
|
||||
following jobs will not run.
|
||||
- `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 / test everything (macOS)`: This runs `brew tests` on macOS.
|
||||
- `CI / tests (no-compatibility mode)`, `CI / tests (generic OS)` and `CI / tests (Linux)`: These run
|
||||
`brew tests` with various options on Linux.
|
||||
|
||||
### `brew tests` and Codecov
|
||||
|
||||
A coverage report is generated by Codecov for every PR, and its results are shown as CI jobs.
|
||||
Additionally, annotations will appear in the "Files changed" where lines of code have been
|
||||
These reports are publicly viewable on [Homebrew/brew's Codecov page](https://app.codecov.io/gh/Homebrew/brew).
|
||||
Additionally, annotations will appear in the PR's "Files changed" tab where lines of code have been
|
||||
added that aren't being hit by `brew tests`. If the Codecov job fails, that's a sign that some
|
||||
more tests should be added to test the functionality being added in the PR.
|
||||
|
||||
|
||||
@ -12,7 +12,7 @@ This guide applies to all four of the cask repositories:
|
||||
|
||||
## Common Situations
|
||||
|
||||
Here is a list of the most common situations that arise in PRs and how to handle them:
|
||||
Here is a list of the most common situations that arise in cask PRs and how to handle them:
|
||||
|
||||
- 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.
|
||||
@ -25,18 +25,19 @@ Here is a list of the most common situations that arise in PRs and how to handle
|
||||
|
||||
If in doubt, ask another cask maintainer on GitHub or Slack.
|
||||
|
||||
Note that unlike in formulae, casks do not consider the `sha256` stanza as 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.
|
||||
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.
|
||||
|
||||
## Merging
|
||||
|
||||
### Approvals
|
||||
|
||||
PRs in the cask repositories should must have at least one approval
|
||||
from a user with write access before they can be merged.
|
||||
|
||||
Ideally, this approval will come from another maintainer. If necessary, however, 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
|
||||
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".
|
||||
|
||||
|
||||
@ -72,7 +72,7 @@ Only one maintainer is necessary to approve and merge the addition of a new or u
|
||||
|
||||
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.
|
||||
|
||||
| | PR modified a single formula | PR modifies multiple formulae |
|
||||
| | 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 |
|
||||
@ -117,13 +117,13 @@ Formulae that:
|
||||
should not be removed from Homebrew. The exception to this rule are [versioned formulae](Versions.md) for which there are higher standards of usage and a maximum number of versions for a given formula.
|
||||
|
||||
For more information about deprecating, disabling and removing formulae, see the
|
||||
[Deprecating, Disabling, and Removing Formulae page](Deprecating-Disabling-and-Removing-Formulae.md)
|
||||
[Deprecating, Disabling, and Removing Formulae page](Deprecating-Disabling-and-Removing-Formulae.md).
|
||||
|
||||
## Detailed merge checklist
|
||||
|
||||
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
|
||||
transparency for contributors in addition to the
|
||||
[Acceptable Formulae](Acceptable-Formulae.md) requirements.
|
||||
|
||||
- previously opened active PRs, as we would like to be fair to contributors who came first
|
||||
|
||||
@ -80,7 +80,7 @@ PRs that are an "enhancement" to existing functionality i.e. not a fix to an ope
|
||||
|
||||
If a maintainer is on holiday/vacation/sick during this time and leaves comments after they are back: please treat post-merge PR comments and feedback as you would left within the time period and follow-up with another PR to address their requests (if agreed).
|
||||
|
||||
The vast majority of `Homebrew/homebrew-core` PRs are bug fixes or version bumps so can be self-merged once CI has completed.
|
||||
The vast majority of `Homebrew/homebrew-core` PRs are bug fixes or version bumps which can be self-merged once CI has completed.
|
||||
|
||||
## Communication
|
||||
|
||||
|
||||
@ -61,10 +61,10 @@
|
||||
- [Maintainers: Avoiding Burnout](Maintainers-Avoiding-Burnout.md)
|
||||
|
||||
- [Maintainer Guidelines](Maintainer-Guidelines.md)
|
||||
- [`brew` Maintainer Guide](Brew-Maintainer-Guide.md)
|
||||
- [Core Maintainer Guide](Homebrew-homebrew-core-Merge-Checklist.md)
|
||||
- [Linux Maintainer Guide](Linux-Maintainer-Guide.md)
|
||||
- [Cask Maintainer Guide](Cask-Maintainer-Guide.md)
|
||||
- [Homebrew/brew Maintainer Guide](Homebrew-brew-Maintainer-Guide.md)
|
||||
- [Homebrew/homebrew-core Maintainer Guide](Homebrew-homebrew-core-Merge-Checklist.md)
|
||||
- [Homebrew/linuxbrew-core Maintainer Guide](Homebrew-linuxbrew-core-Maintainer-Guide.md)
|
||||
- [Homebrew/homebrew-cask Maintainer Guide](Homebrew-homebrew-cask-Maintainer-Guide.md)
|
||||
|
||||
- [Brew Test Bot For Maintainers](Brew-Test-Bot-For-Core-Contributors.md)
|
||||
- [Common Issues for Maintainers](Common-Issues-for-Core-Contributors.md)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user