Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workflows: notify on autobump bypass #162592

Closed
wants to merge 1 commit into from
Closed

workflows: notify on autobump bypass #162592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .github/workflows/scripts/check-autobump.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why this isn't just implemented as a shell script that calls gh?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took inspiration from the label pull request action: https://github.com/Homebrew/actions/blob/master/label-pull-requests/main.js
I can try to rewrite as a shell script though, if we feel this is a good thing to do. I have no strong opinion on that right now.

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const fs = require('fs')
const path = require('path');

module.exports = async ({github, context, core}, autobump_list) => {
// Parse event
const json = fs.readFileSync(process.env.GITHUB_EVENT_PATH, { encoding: "utf-8" })
const event = JSON.parse(json)
const pull = event.pull_request

// Fetch PR files
const files = await github.rest.pulls.listFiles({
...context.repo,
pull_number: pull.number
})

const autobump_array = autobump_list.split(" ")

for (const file of files.data) {
if(autobump_array.includes(path.parse(file.filename).name)) {
github.rest.issues.createComment({
...context.repo,
issue_number: pull.number,
body: `
Hi! You have used \`brew bump-formula-pr\` on a formula that is on the autobump list.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do somewhat the opposite here and instead prevent brew bump-formula-pr from opening the PR in the first place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do that, do you have suggestions how we keep testbot out of that restriction?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought about this. I did not think the technical aspect through on how to filter it out for testbot (but there might be solutions).

I was more thinking about other implications of the change:
brew bump-formula-pr has been part of the success of Homebrew, we have advertised it as an "entry-level" tool for first time contributors. So I am worried to make drastic changes here.

I'm unsure if there will be situations where we still need be able to open these kind of pull requests. Maybe to bump a formula for an urgent CVE, though a manual pull request would workaround the limitation.

Also; we can still revisit the strategy later on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do gh api -H "Accept: application/vnd.github+json" /user and check what the login is. I think it's essential to avoid a "I know what I'm doing" flag, otherwise everyone will just use that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do that, do you have suggestions how we keep testbot out of that restriction?

an undocumented environment variable or similar.

I was more thinking about other implications of the change:
brew bump-formula-pr has been part of the success of Homebrew, we have advertised it as an "entry-level" tool for first time contributors. So I am worried to make drastic changes here.

yeh, I think we definitely should make drastic changes there 😂

though a manual pull request would workaround the limitation.

Exactly 👍🏻

I think it's essential to avoid a "I know what I'm doing" flag, otherwise everyone will just use that.

Agreed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have opened Homebrew/brew#16664 and #162841 for this.

Also on my list is to handle a maximum number of PRs that can be open for a given author.

No need to manually bump it, BrewTestBot will automatically create an equivalent pull request for you!

We prefer that you spend time fixing broken pull requests instead of manually bumping formulae.`
});
}
}
}
27 changes: 27 additions & 0 deletions .github/workflows/triage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ env:
GH_REPO: ${{ github.repository }}
GH_NO_UPDATE_NOTIFIER: 1
GH_PROMPT_DISABLED: 1
SCRIPTS_PATH: .github/workflows/scripts

concurrency:
group: "triage-${{ github.event.number }}"
Expand Down Expand Up @@ -60,6 +61,9 @@ jobs:
triage:
runs-on: ubuntu-22.04
steps:
- name: Checkout repository
uses: actions/checkout@v3
Comment on lines +64 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda wary about slowing down triage jobs with an additional clone.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too. I thought about moving it to the tests.yml file ... but it defeats the "triage" part


- name: Check commit format
if: >
github.actor != 'BrewTestBot' &&
Expand Down Expand Up @@ -227,3 +231,26 @@ jobs:

- label: pip-audit
pr_body_content: Created by `brew-pip-audit`

- name: Get list of autobumped formulae
id: autobump
run: echo "autobump_list=$(xargs < "$GITHUB_WORKSPACE"/.github/autobump.txt)" >> "$GITHUB_OUTPUT"

- name: Write comment on autobump bypass
if: >
github.actor != 'BrewTestBot' &&
contains(github.event.pull_request.labels.*.name, 'bump-formula-pr')
uses: actions/github-script@v7
env:
AUTOBUMPED_FORMULAE: ${{steps.autobump.outputs.autobump_list}}
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
script: |
const path = require('path')
const script = require(path.resolve(`${process.env.SCRIPTS_PATH}/check-autobump.js`))

try {
await script({github, context, core}, `${process.env.AUTOBUMPED_FORMULAE}`)
} catch (error) {
console.error(error);
}
Loading