publish-commit-bottles: optionally push to pull request branch. #125556

MikeMcQuaid · 2023-03-13T22:05:25Z

Rather than pulling bottles and pushing to master, pull bottles, push the bottle commit to the PR branch for automatic merging by GitHub and add a label to avoid rerunning brew test-bot on the various formulae again.

This also allows us to use the GitHub merge queue so run on merge_group events too.

CC @carlocab as we'd talked about this

carlocab

One thing we might want to consider is to teach test-bot to ignore formulae which already have a separate bottle commit on the PR branch, but build ones for those still missing them. This might get us to something like incremental rebuilds on reruns.

.github/workflows/publish-commit-bottles.yml

ZhongRuoyu · 2023-03-14T01:45:58Z

Sounds worth trying out! Question: wouldn't the bottle commit dismiss existing approvals?

Update: or, does this mean another maintainer (or possibly the maintainer(s) who previously approved it) is required to approve the PR again?

ZhongRuoyu

Worth trying out!

.github/workflows/publish-commit-bottles.yml

MikeMcQuaid · 2023-03-14T10:46:29Z

Question: wouldn't the bottle commit dismiss existing approvals?

@ZhongRuoyu If so: I'll adjust the branch protection rules to avoid this 👍🏻

.github/workflows/publish-commit-bottles.yml

.github/workflows/tests.yml

Needed for Homebrew/homebrew-core#125556.

ZhongRuoyu

These too. (⌘F says that's all.)

.github/workflows/publish-commit-bottles.yml

.github/workflows/tests.yml

ZhongRuoyu · 2023-03-15T19:13:53Z

~~Two~~ Three more things (for now):

Maybe, we want to consider changing the wording of comments posted here? Currently they read either ":shipit: @BrewTestBot has triggered a merge" or ":robot: A scheduled task has triggered a merge".
(Technically this is not wrong, since a merge will follow this soon after; but I'd like to raise a discussion to see if "@BrewTestBot has triggered a bottle publish" or something similar would better describe what this workflow does.)
Will this (L83-L91; couldn't comment there) still push to master...

      - name: Push commits
        uses: Homebrew/actions/git-try-push@master
        with:
          token: ${{secrets.HOMEBREW_GITHUB_PUBLIC_REPO_TOKEN}}
          directory: ${{steps.set-up-homebrew.outputs.repository-path}}
        env:
          GIT_COMMITTER_NAME: BrewTestBot
          GIT_COMMITTER_EMAIL: 1589480+BrewTestBot@users.noreply.github.com
          HOMEBREW_GPG_PASSPHRASE: ${{ secrets.BREWTESTBOT_GPG_SIGNING_SUBKEY_PASSPHRASE }}

... because of this? https://github.com/Homebrew/actions/blob/master/git-try-push/action.yml#L18-L21

  branch:
    description: Git branch
    required: false
    default: master

I think so, based on what I read from the action's main.js. But even if that's not the case, perhaps we'd better be more explicit.

Regarding this question:

wouldn't the bottle commit dismiss existing approvals?

@MikeMcQuaid said:

If so: I'll adjust the branch protection rules to avoid this 👍🏻

If that's to untick "Dismiss stale pull request approvals when new commits are pushed": assuming a naughty PR author is lucky enough to successfully push a malicious commit after the bottle commit is pushed, but right before automerge is switched on, the malicious commit might be able to sneak into master (CMIIW).

I refer to this which reads:

After you enable auto-merge for a pull request, if someone who does not have write permissions to the repository pushes new changes to the head branch or switches the base branch of the pull request, auto-merge will be disabled.

Do we have an appropriate solution to tackle that? We could enable automerge before pushing the bottle commit, but that could also happen before automerge is enabled, but after this workflow starts. There may still be a window for bad things to happen.

MikeMcQuaid · 2023-03-15T20:44:30Z

We could enable automerge before pushing the bottle commit, but that could also happen before automerge is enabled, but after this workflow starts

That's a good idea 👍🏻

carlocab · 2023-03-16T00:20:34Z

.github/workflows/tests.yml

+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}


Looks like this worked, but you need to cd to homebrew/core.

carlocab · 2023-03-16T00:22:28Z

We could enable automerge before pushing the bottle commit, but that could also happen before automerge is enabled, but after this workflow starts

That's a good idea 👍🏻

Wouldn't this result in a PR getting merged before the bottle commit is pushed if it was previously approved? Or is that fixed by merge_group?

MikeMcQuaid · 2023-03-16T02:43:22Z

Wouldn't this result in a PR getting merged before the bottle commit is pushed if it was previously approved?

Yes, that's true. Need to enable auto-merge immediately after pushing I guess!

carlocab

Happy with where this is going, but I think it would be useful to gate these changes behind a PR label so that we can test it out on a few PRs before rolling it out fully.

Then, once we start merging most PRs this way, we can also use a label for the old merge flow for PRs that don't have Allow edits from maintainers enabled (e.g. org forks).

Bo98

Been thinking about it a bit and wondering how we could use merge queues to handle this. Turns out merge groups are a bit more limited than I thought.

Will take a proper look at this at the weekend but here's a couple questions from a quick think through.

Bo98 · 2023-03-16T13:58:16Z

.github/workflows/publish-commit-bottles.yml

@@ -30,6 +30,12 @@ jobs:
    container:
      image: ghcr.io/homebrew/ubuntu22.04:master
    steps:
+      - name: Enable PR automerge
+        run: gh pr merge --auto ${{github.event.inputs.pull_request}}


What strategy does this use?

We don't allow merge commits currently and squash commits is not what we want. Rebase sounds right but sadly still destroys signing.

@Bo98 In this case as-is: the default. We should start using merge commits in this repository (and homebrew-cask) along with sharing Formula/Casks to make the Git operations on these repositories more performance so by the time I merge this I expect: merge commits.

It's not clear what "the default" meant considering merge commits isn't currently enabled and the default on the web interface is just the last one you used.

I think gh requires specification of a merge strategy to use gh pr merge. i.e. we must pass one of --squash, --rebase, or --merge.

Bo98 · 2023-03-16T13:59:28Z

.github/workflows/publish-commit-bottles.yml

      - name: Pull and upload bottles to GitHub Packages
        env:
          BREWTESTBOT_NAME_EMAIL: "BrewTestBot <1589480+BrewTestBot@users.noreply.github.com>"
          HOMEBREW_GPG_PASSPHRASE: ${{ secrets.BREWTESTBOT_GPG_SIGNING_SUBKEY_PASSPHRASE }}
          HOMEBREW_GITHUB_API_TOKEN: ${{secrets.HOMEBREW_CORE_PUBLIC_REPO_EMAIL_TOKEN}}
          HOMEBREW_GITHUB_PACKAGES_USER: brewtestbot
          HOMEBREW_GITHUB_PACKAGES_TOKEN: ${{secrets.HOMEBREW_CORE_GITHUB_PACKAGES_TOKEN}}
-        run: brew pr-pull --debug --workflows=tests.yml --committer="$BREWTESTBOT_NAME_EMAIL" --root-url="https://ghcr.io/v2/homebrew/core" ${{github.event.inputs.args}} ${{github.event.inputs.pull_request}}
+        run: brew pr-pull --debug --workflows=tests.yml --committer="$BREWTESTBOT_NAME_EMAIL" --root-url="https://ghcr.io/v2/homebrew/core" --no-autosquash --clean ${{github.event.inputs.args}} ${{github.event.inputs.pull_request}}


This is a significant change and needs to be communicated accordingly or have stronger commit checks that disallow merges that need their commits adjusted.

@Bo98 Alternatively, we can (and should) care less about commit message/format pedantry, particularly if/when we're moving to merge commits where it matters much less about narrowing things down to a single commit.

That would be disappointing - git blame is actually really useful here, compared to sometimes being horrid to use on Homebrew/brew to the point where I have to click 20 times to get past one PR. But I'll consider the rest of the concept first.

I'm also not entirely sure how brew extract/FormulaVersions behaves when a version bump is split into several commits but that can be adjusted if need be.

I'm also not entirely sure how brew extract/FormulaVersions behaves when a version bump is split into several commits but that can be adjusted if need be.

Yea, I think we may break brew extract and FormulaVersions. Though I think the former will be broken because it relies on the latter. Breaking FormulaVersions would not be good -- we use it for some audits.

MikeMcQuaid · 2023-03-16T14:34:35Z

Been thinking about it a bit and wondering how we could use merge queues to handle this. Turns out merge groups are a bit more limited than I thought.

@Bo98 Yes, this gets us on the way to be able to use merge queues for some flows but you cannot push or modify commits as part of the merge queue. We should not expect that to change.

Expanding on the above: the homebrew/core CI flow is (and I say this as the person who is primarily to blame for this 😅) very unusual, makes it hard to rely on GitHub's security and workflow features (shout out to all our PRs with bottles being "closed" rather than "merged", something GitHub is never likely to let us fix), makes the repository network size balloon and stops us from being able to repeatedly rerun builds and reuse existing work.

To be very clear: I don't expect to just merge this PR as is in the next few days and I expect there will be follow-up changes required to fix and improve these workflows.

carlocab · 2023-03-16T14:39:34Z

To be very clear: I don't expect to just merge this PR as is in the next few days and I expect there will be follow-up changes required to fix and improve these workflows.

I hate to sound like a broken record, but: that's the advantage of my suggestion to gate this change behind a PR flag. It'll give us the chance to test and iterate on these changes more quickly, much the same way we introduce significant new features via a HOMEBREW_* environment variable, then to HOMEBREW_DEVELOPERs, etc.

MikeMcQuaid · 2023-03-16T15:17:04Z

I hate to sound like a broken record, but: that's the advantage of my suggestion to gate this change behind a PR flag.

I feel like you are a working record because this sounds like the first time I heard this, sorry 😂

Definitely game to land this and #125595 sooner rather than later for experimentation.

How do you see the PR flag working? A label? Something different?

We'll have to avoid some things e.g. probably using merge commits then but that shouldn't make much of a difference.

Feel free to commit directly to this PR (and #125595) if that's easier than leaving comments here.

carlocab

Modulo token issues and cleanup, the only change that this will really have that isn't opt-in is merge_group. Happy to land this for experimentation once token issues and cleanup is done.

MikeMcQuaid · 2023-03-17T19:19:23Z

It wasn't that long ago! 😅 #125556 (review)

Whoops, sorry, missed that!

Modulo token issues and cleanup, the only change that this will really have that isn't opt-in is merge_group. Happy to land this for experimentation once token issues and cleanup is done.

Awesome, thanks @carlocab, yeh, please feel free to land when you're happy with it. The merge_group change is essentially a no-op until we enable the merge queue branch protection so should be safe as-is.

carlocab · 2023-03-17T20:14:27Z

I can clean this up, but I was hoping you could handle the token problem since I have no idea what to do with it 😅 In particular, I don't know which tokens have which scopes, etc.

MikeMcQuaid · 2023-03-17T20:21:45Z

I can clean this up, but I was hoping you could handle the token problem since I have no idea what to do with it 😅 In particular, I don't know which tokens have which scopes, etc.

@carlocab sure thing will take a look!

carlocab · 2023-03-17T20:31:17Z

.github/workflows/publish-commit-bottles.yml

+        run: gh pr checkout ${{github.event.inputs.pull_request}}
+        if: inputs.commit_bottles_to_pr_branch
+        env:
+          GH_TOKEN: ${{secrets.GITHUB_TOKEN}}


Yep, I've tried GITHUB_TOKEN, but it seems to not have the right permissions either.

@carlocab Working now. Needed to add permissions.

Rather than pulling bottles and pushing to master, pull bottles, push the bottle commit to the PR branch for automatic merging by GitHub and add a label to avoid rerunning `brew test-bot` on the various formulae again. This also allows us to use the GitHub merge queue so run on `merge_group` events too. Co-authored-by: Ruoyu Zhong <zhongruoyu@outlook.com> Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>

carlocab · 2023-03-17T20:52:48Z

Needs @Homebrew/tsc to merge.

Rylan12

This looks awesome, I'm very excited to see this in action!

If someone wants to get this merged tonight: I'll be around for several hours in case a TSC approval is needed for a revert. I'll try to periodically check but if it's urgent ping me on Slack so it sends me a notification.

#126011) Revert "publish-commit-bottles: optionally push to pull request branch. (#125556)" This reverts commit 72680fb.

Needed for Homebrew/homebrew-core#125556. Without this, `pr-pull` attempts to cherry-pick commits from the PR branch onto the PR branch, and then gets upset that nothing happened. See https://github.com/Homebrew/homebrew-core/actions/runs/4461335852/jobs/7835095294#step:10:40

Needed for Homebrew#125556. Closes Homebrew/actions#338.

…126125) Needed for #125556. Closes Homebrew/actions#338.

MikeMcQuaid requested a review from a team March 13, 2023 22:05

MikeMcQuaid requested a review from a team as a code owner March 13, 2023 22:05

BrewTestBot added automerge-skip `brew pr-automerge` will skip this pull request workflows PR modifies GitHub Actions workflow files labels Mar 13, 2023

carlocab previously approved these changes Mar 14, 2023

View reviewed changes

.github/workflows/publish-commit-bottles.yml Show resolved Hide resolved

.github/workflows/publish-commit-bottles.yml Outdated Show resolved Hide resolved

ZhongRuoyu reviewed Mar 14, 2023

View reviewed changes

.github/workflows/publish-commit-bottles.yml Outdated Show resolved Hide resolved

MikeMcQuaid dismissed carlocab’s stale review via ffa99d1 March 14, 2023 10:27

MikeMcQuaid force-pushed the publish_commit_bottles_to_pr branch from ffa99d1 to c3bf91b Compare March 14, 2023 10:35

carlocab reviewed Mar 14, 2023

View reviewed changes

.github/workflows/publish-commit-bottles.yml Outdated Show resolved Hide resolved

MikeMcQuaid mentioned this pull request Mar 14, 2023

brew bump* commands should enable auto-merge Homebrew/brew#14538

Closed

1 task

MikeMcQuaid force-pushed the publish_commit_bottles_to_pr branch from 9ff1491 to 1261fb3 Compare March 14, 2023 17:15

carlocab reviewed Mar 15, 2023

View reviewed changes

.github/workflows/tests.yml Outdated Show resolved Hide resolved

carlocab mentioned this pull request Mar 15, 2023

Dockerfile: fix installation of gh, et al. Homebrew/brew#14982

Merged

7 tasks

carlocab added a commit to carlocab/brew that referenced this pull request Mar 15, 2023

Dockerfile: fix installation of gh, et al.

9ad7f91

Needed for Homebrew/homebrew-core#125556.

ZhongRuoyu reviewed Mar 15, 2023

View reviewed changes

.github/workflows/publish-commit-bottles.yml Outdated Show resolved Hide resolved

.github/workflows/tests.yml Outdated Show resolved Hide resolved

MikeMcQuaid force-pushed the publish_commit_bottles_to_pr branch 2 times, most recently from 6625e5c to d0498d9 Compare March 15, 2023 21:13

carlocab reviewed Mar 16, 2023

View reviewed changes

Bo98 reviewed Mar 16, 2023

View reviewed changes

MikeMcQuaid mentioned this pull request Mar 16, 2023

Re-evaluate OpenSSL's keg-only status #104784

Closed

carlocab previously approved these changes Mar 17, 2023

View reviewed changes

MikeMcQuaid dismissed carlocab’s stale review via af49ee3 March 17, 2023 20:27

MikeMcQuaid force-pushed the publish_commit_bottles_to_pr branch from ea2ce6b to af49ee3 Compare March 17, 2023 20:27

MikeMcQuaid changed the title ~~publish-commit-bottles: push to pull request branch.~~ publish-commit-bottles: optionally push to pull request branch. Mar 17, 2023

carlocab reviewed Mar 17, 2023

View reviewed changes

MikeMcQuaid force-pushed the publish_commit_bottles_to_pr branch from af49ee3 to 8bf0f6d Compare March 17, 2023 20:38

github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Mar 17, 2023

MikeMcQuaid force-pushed the publish_commit_bottles_to_pr branch from 8bf0f6d to 3c84e2a Compare March 17, 2023 20:40

carlocab approved these changes Mar 17, 2023

View reviewed changes

carlocab requested a review from a team March 17, 2023 20:52

carlocab enabled auto-merge (squash) March 17, 2023 20:52

Rylan12 approved these changes Mar 18, 2023

View reviewed changes

carlocab merged commit 72680fb into master Mar 18, 2023

carlocab deleted the publish_commit_bottles_to_pr branch March 18, 2023 01:58

p-linnane mentioned this pull request Mar 18, 2023

Revert "publish-commit-bottles: optionally push to pull request branc… #126011

Merged

p-linnane added a commit that referenced this pull request Mar 18, 2023

Revert "publish-commit-bottles: optionally push to pull request branc… (

f6816db

#126011) Revert "publish-commit-bottles: optionally push to pull request branch. (#125556)" This reverts commit 72680fb.

carlocab mentioned this pull request Mar 19, 2023

pr-pull: add --no-cherry-pick flag Homebrew/brew#15011

Merged

7 tasks

carlocab mentioned this pull request Mar 19, 2023

Allow git-try-push to push to PR branches. Homebrew/actions#338

Closed

carlocab mentioned this pull request Mar 20, 2023

publish-commit-bottles: specify branch and remote in git-try-push #126125

Merged

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Mar 20, 2023

publish-commit-bottles: specify branch and remote in git-try-push

c6d7bb3

Needed for Homebrew#125556. Closes Homebrew/actions#338.

carlocab added a commit that referenced this pull request Mar 21, 2023

publish-commit-bottles: specify branch and remote in git-try-push (#…

b7e5253

…126125) Needed for #125556. Closes Homebrew/actions#338.

carlocab mentioned this pull request Mar 28, 2023

dev-cmd/pr-*: support merge workflow changes Homebrew/brew#15077

Merged

7 tasks

BugenZhao mentioned this pull request May 30, 2023

avoid brew pr-pull directly push to the main branch risingwavelabs/homebrew-risingwave#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

publish-commit-bottles: optionally push to pull request branch. #125556

publish-commit-bottles: optionally push to pull request branch. #125556

MikeMcQuaid commented Mar 13, 2023

carlocab left a comment

ZhongRuoyu commented Mar 14, 2023 •

edited

Loading

ZhongRuoyu left a comment

MikeMcQuaid commented Mar 14, 2023

ZhongRuoyu left a comment

ZhongRuoyu commented Mar 15, 2023 •

edited

Loading

MikeMcQuaid commented Mar 15, 2023

carlocab Mar 16, 2023

carlocab commented Mar 16, 2023

MikeMcQuaid commented Mar 16, 2023

carlocab left a comment

Bo98 left a comment •

edited

Loading

Bo98 Mar 16, 2023 •

edited

Loading

MikeMcQuaid Mar 16, 2023

Bo98 Mar 16, 2023

carlocab Mar 17, 2023

Bo98 Mar 16, 2023

MikeMcQuaid Mar 16, 2023

Bo98 Mar 16, 2023 •

edited

Loading

carlocab Mar 17, 2023

MikeMcQuaid commented Mar 16, 2023

carlocab commented Mar 16, 2023

MikeMcQuaid commented Mar 16, 2023

carlocab left a comment

MikeMcQuaid commented Mar 17, 2023

carlocab commented Mar 17, 2023

MikeMcQuaid commented Mar 17, 2023

carlocab Mar 17, 2023

MikeMcQuaid Mar 17, 2023

carlocab commented Mar 17, 2023

Rylan12 left a comment

publish-commit-bottles: optionally push to pull request branch. #125556

publish-commit-bottles: optionally push to pull request branch. #125556

Conversation

MikeMcQuaid commented Mar 13, 2023

carlocab left a comment

Choose a reason for hiding this comment

ZhongRuoyu commented Mar 14, 2023 • edited Loading

ZhongRuoyu left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented Mar 14, 2023

ZhongRuoyu left a comment

Choose a reason for hiding this comment

ZhongRuoyu commented Mar 15, 2023 • edited Loading

MikeMcQuaid commented Mar 15, 2023

Choose a reason for hiding this comment

carlocab commented Mar 16, 2023

MikeMcQuaid commented Mar 16, 2023

carlocab left a comment

Choose a reason for hiding this comment

Bo98 left a comment • edited Loading

Choose a reason for hiding this comment

Bo98 Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bo98 Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Mar 16, 2023

carlocab commented Mar 16, 2023

MikeMcQuaid commented Mar 16, 2023

carlocab left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented Mar 17, 2023

carlocab commented Mar 17, 2023

MikeMcQuaid commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlocab commented Mar 17, 2023

Rylan12 left a comment

Choose a reason for hiding this comment

ZhongRuoyu commented Mar 14, 2023 •

edited

Loading

ZhongRuoyu commented Mar 15, 2023 •

edited

Loading

Bo98 left a comment •

edited

Loading

Bo98 Mar 16, 2023 •

edited

Loading

Bo98 Mar 16, 2023 •

edited

Loading