Skip to content

Commit

Permalink
Improve some CI jobs
Browse files Browse the repository at this point in the history 
- make the CodeCov CI job informational. We don't want red PRs just
  because the coverage varies slightly. We still get comments inline
  saying where code coverage is met; this is more useful during review
  than a single number and failing status
- make the Triage CI job do less: instead of enforcing a time period for
  review window, make it only exist to self-approve PRs for merge and
  require a maintainer otherwise to review
  • Loading branch information
@MikeMcQuaid
MikeMcQuaid committed Mar 23, 2023
1 parent 65d858d commit 510c4dc
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 135 deletions.
1 change: 1 addition & 0 deletions .github/codecov.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ coverage:
status:
project:
default:
informational: true
threshold: 0.05%
patch:
default:
Expand Down
134 changes: 3 additions & 131 deletions .github/workflows/triage.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,8 @@ on:
- opened
- synchronize
- reopened
- closed
- labeled
- unlabeled
schedule:
- cron: "0 */3 * * *" # every 3 hours

permissions: {}

Expand All @@ -21,18 +18,10 @@ jobs:
runs-on: ubuntu-22.04
if: startsWith(github.repository, 'Homebrew/')
steps:
- name: Re-run this workflow
if: github.event_name == 'schedule' || github.event.action == 'closed'
uses: reitermarkus/rerun-workflow@7381e98aa2bc4464acef4b60ade8d1d1d90e6e65
with:
token: ${{ secrets.HOMEBREW_GITHUB_PUBLIC_REPO_TOKEN }}
continuous-label: waiting for feedback
trigger-labels: critical
workflow: triage.yml
- name: Review pull request
if: >
(github.event_name == 'pull_request' || github.event_name == 'pull_request_target') &&
github.event.action != 'closed' && github.event.pull_request.state != 'closed'
github.event.pull_request.state != 'closed'
uses: actions/github-script@v6
with:
github-token: ${{ secrets.HOMEBREW_BREW_TRIAGE_PULL_REQUESTS_TOKEN }}
Expand All @@ -51,36 +40,6 @@ jobs:
})
}
async function findComment(pullRequestNumber, id) {
const { data: comments } = await github.rest.issues.listComments({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pullRequestNumber,
})
const regex = new RegExp(`<!--\\s*#${id}\\s*-->`)
return comments.filter(comment => comment.body.match(regex))[0]
}
async function createOrUpdateComment(pullRequestNumber, id, message) {
const beginComment = await findComment(pullRequestNumber, id)
const body = `<!-- #${id} -->\n\n${message}`
if (beginComment) {
await github.rest.issues.updateComment({
...context.repo,
comment_id: beginComment.id,
body,
})
} else {
await github.rest.issues.createComment({
...context.repo,
issue_number: pullRequestNumber,
body,
})
}
}
async function approvalsByAuthenticatedUser(pullRequestNumber) {
const { data: user } = await github.rest.users.getAuthenticated()
Expand All @@ -93,42 +52,6 @@ jobs:
return approvals.filter(review => review.user.login == user.login)
}
async function dismissApprovals(pullRequestNumber, message) {
const reviews = await approvalsByAuthenticatedUser(pullRequestNumber)
for (const review of reviews) {
await github.rest.pulls.dismissReview({
...context.repo,
pull_number: pullRequestNumber,
review_id: review.id,
message: message
});
}
}
function offsetDate(start, offsetHours, skippedDays) {
let end = new Date(start)
end.setUTCHours(end.getUTCHours() + (offsetHours % 24))
while (skippedDays.includes(end.getUTCDay()) || offsetHours >= 24) {
if (!skippedDays.includes(end.getUTCDay())) {
offsetHours -= 24
}
end.setUTCDate(end.getUTCDate() + 1)
}
if (skippedDays.includes(start.getUTCDay())) {
end.setUTCHours(offsetHours, 0, 0)
}
return end
}
function formatDate(date) {
return date.toISOString().replace(/\.\d+Z$/, ' UTC').replace('T', ' at ')
}
async function reviewPullRequest(pullRequestNumber) {
const { data: pullRequest } = await github.rest.pulls.get({
owner: context.repo.owner,
Expand All @@ -147,65 +70,14 @@ jobs:
return
}
const reviewLabel = 'waiting for feedback'
const criticalLabel = 'critical'
const labels = pullRequest.labels.map(label => label.name)
const hasReviewLabel = labels.includes(reviewLabel)
const hasCriticalLabel = labels.includes(criticalLabel)
const offsetHours = 24
const skippedDays = [
6, // Saturday
0, // Sunday
]
const currentDate = new Date()
const reviewStartDate = new Date(pullRequest.created_at)
const reviewEndDate = offsetDate(reviewStartDate, offsetHours, skippedDays)
const reviewEnded = currentDate > reviewEndDate
if (reviewEnded || hasCriticalLabel) {
let message
if (hasCriticalLabel && !reviewEnded) {
message = `Review period skipped due to \`${criticalLabel}\` label.`
} else {
message = 'Review period ended.'
}
if (hasReviewLabel) {
await github.rest.issues.removeLabel({
...context.repo,
issue_number: pullRequestNumber,
name: reviewLabel,
})
}
if (hasCriticalLabel) {
const message = `Review granted due to \`${criticalLabel}\` label.`
core.info(message)
await createOrUpdateComment(pullRequestNumber, 'review-period-end', message)
await approvePullRequest(pullRequestNumber)
} else {
const message = `Review period will end on ${formatDate(reviewEndDate)}.`
core.info(message)
await dismissApprovals(pullRequestNumber, 'Review period has not ended yet.')
await createOrUpdateComment(pullRequestNumber, 'review-period-begin', message)
const endComment = await findComment(pullRequestNumber, 'review-period-end')
if (endComment) {
await github.rest.issues.deleteComment({
...context.repo,
comment_id: endComment.id,
})
}
await github.rest.issues.addLabels({
...context.repo,
issue_number: pullRequestNumber,
labels: [reviewLabel],
})
core.setFailed('Review period has not ended yet.')
}
}
Expand Down
6 changes: 2 additions & 4 deletions docs/Homebrew-brew-Maintainer-Guide.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@ If possible, PRs should also have GPG-signed commits (see the private `ops` repo

### Automatic approvals

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.
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, 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 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 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.
As a compromise between always needing a review and allowing maintainers to merge PRs they deem critical, the `Triage` CI job will ensure that if a PR is labelled `critical`, [@BrewTestBot](https://github.com/BrewTestBot) approves the PR, allowing it to be merged.

## CI

Expand Down

0 comments on commit 510c4dc

Please sign in to comment.