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.

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

bump-formula|cask-pr: do not allow to bump autobumped packages #16750

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Feb 26, 2024

Avoids:

  • Bumping stuff by mistake that will get autobumped anyway

  • Abusing the system to get "free" commits

  • Have you followed the guidelines in our Contributing document?

  • Have you checked to ensure there aren't other open Pull Requests for the same change?

  • Have you added an explanation of what your changes do and why you'd like us to include them?

  • Have you written new tests for your changes? Here's an example.

  • Have you successfully run brew style with your changes locally?

  • Have you successfully run brew typecheck with your changes locally?

  • Have you successfully run brew tests with your changes locally?


Library/Homebrew/dev-cmd/bump-cask-pr.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Great work @iMichka! Some Pathname and messaging improvements.

Library/Homebrew/dev-cmd/bump-cask-pr.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bump-cask-pr.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated Show resolved Hide resolved
@iMichka
Copy link
Member Author

iMichka commented Feb 27, 2024

One odd thing (and the reason why used a regex in the first place): cask.name returns an array of uppercase cask names.
I hade to change that to cask.name.first.downcase ...

I had no time to check what is the origin of this array. I got confused because formula.name was just a string as expected.

The current code is working, I'll defer to the reviewers if you want me to check the cask.name.first.downcase thing (I'll double check this tomorrow nevertheless, just had no time to dig through the whole code today).

Copy link
Member

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

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

Seems we also need to allow @BrewTestBot to bypass this restriction.

Library/Homebrew/dev-cmd/bump-cask-pr.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bump-cask-pr.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Suggestions look good to me!

@iMichka
Copy link
Member Author

iMichka commented Feb 28, 2024

This should be good now: thanks for all the suggestions and the help :)

Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks @iMichka!

Copy link
Member

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

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

@BrewTestBot is also using the bump-formula-pr/bump-cask-pr commands under the hood. Will this prevent it from bumping formulae normally?

@apainintheneck
Copy link
Contributor

Looks good to me!

$ brew bump-cask-pr --dry-run --version=4.4.4 tsh
Error: Whoops, the tsh cask has its version update
pull requests automatically opened by BrewTestBot!
We'd still love your contributions, though, so try another one
that's not in the autobump list:
  https://github.com/Homebrew/homebrew-cask/blob/master/.github/autobump.txt
$ brew bump-formula-pr --dry-run --version=4.4.4 agda
Error: Whoops, the agda formula has its version update
pull requests automatically opened by BrewTestBot!
We'd still love your contributions, though, so try another one
that's not in the autobump list:
  https://github.com/Homebrew/homebrew-core/blob/master/.github/autobump.txt

Thanks @iMichka!

@apainintheneck
Copy link
Contributor

@BrewTestBot is also using this bump-formula-pr command under the hood. Will this prevent it from bumping formulae normally?

Good question. I looked at brew test bot and it doesn't seem to use bump-cask-pr or bump-formula-pr directly outside of the stale PR workflow.

/u/l/H/L/T/h/homebrew-test-bot (master|✔) $ git grep bump-cask-pr
.github/workflows/triage-issues.yml:      - name: Mark/Close Stale `bump-formula-pr` and `bump-cask-pr` Pull Requests
.github/workflows/triage-issues.yml:          any-of-labels: "bump-formula-pr,bump-cask-pr"
/u/l/H/L/T/h/homebrew-test-bot (master|✔) $ git grep bump-formula-pr
.github/workflows/triage-issues.yml:      - name: Mark/Close Stale `bump-formula-pr` and `bump-cask-pr` Pull Requests
.github/workflows/triage-issues.yml:          any-of-labels: "bump-formula-pr,bump-cask-pr"

I looked at the core repos and it seems we use a separate action to bump packages that calls brew bump directly instead of using brew bump-cask-pr and brew bump-formula-pr.

There is a deprecated action for bumping formulae using brew bump-formula-pr but it's not used anymore in core.

@ZhongRuoyu
Copy link
Member

brew bump --open-pr invokes brew bump-formula-pr/brew bump-cask-pr under the hood, actually:

bump_cask_pr_args = [
"bump-#{version_info.type}-pr",
name,
*version_args,
"--no-browse",
"--message=Created by `brew bump`",
]
bump_cask_pr_args << "--no-fork" if args.no_fork?
system HOMEBREW_BREW_FILE, *bump_cask_pr_args

@apainintheneck
Copy link
Contributor

apainintheneck commented Feb 29, 2024

Ah, my bad. I had those backwards. I guess we could allow this only on CI somehow.

Edit: We can probably just use ENV["GITHUB_ACTIONS"] to check if we're on CI.

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

We need to find a way to not break CI scripts that run brew bump.

@ZhongRuoyu
Copy link
Member

We can probably just use ENV["GITHUB_ACTIONS"] to check if we're on CI.

Or maybe HOMEBREW_TEST_BOT_AUTOBUMP, added in #16664.

@MikeMcQuaid
Copy link
Member

Or maybe HOMEBREW_TEST_BOT_AUTOBUMP, added in #16664.

This feels like best fit to me 👍🏻

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good once there's a workaround for BrewTestBot to not 💥

apainintheneck added a commit to Homebrew/actions that referenced this pull request Mar 1, 2024
This will unblock Homebrew/brew#16750 by providing us with an environment variable that's only on when we're autobumping packages. In all other scenarios, we want people to get an error message if they try to bump one of the packages that gets regularly autobumped on CI.
@apainintheneck
Copy link
Contributor

I opened a PR to add the environment variable to the bump-packages action. After that gets merged in and the cleanup mentioned in #16750 (comment) is done, this should be good to go.

@apainintheneck apainintheneck dismissed their stale review March 1, 2024 04:25

Unblocking other reviewers since we've agreed on how to not break CI.

@apainintheneck
Copy link
Contributor

Just to over communicate a bit. Three PRs need to be merged in in a specific order and this one will be the last one to land.

See Homebrew/actions#506 (comment)

Avoids:
- Bumping stuff by mistake that will get autobumped anyway
- Abusing the system to get "free" commits
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @iMichka!

ZhongRuoyu added a commit to Homebrew/homebrew-core that referenced this pull request Mar 1, 2024
This is needed for Homebrew/brew#16750.

Note that #163023 is no longer relevant as the ability to skip duplicate
PR check in `brew bump` has been completely removed in
Homebrew/brew#16781.
Copy link
Member

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

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

Thanks everyone! Let's merge this after Homebrew/homebrew-core#164745 and Homebrew/homebrew-cask#167993.

chenrui333 pushed a commit to Homebrew/homebrew-cask that referenced this pull request Mar 1, 2024
Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

Awesome work everyone!

@ZhongRuoyu ZhongRuoyu merged commit 38b671d into Homebrew:master Mar 1, 2024
26 checks passed
@iMichka iMichka deleted the autobump branch March 1, 2024 21:48
@iMichka
Copy link
Member Author

iMichka commented Mar 1, 2024

Thanks all :)

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants