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

Jump to bottom

bump-*-pr: handle HOMEBREW_NO_GITHUB_API=1 being set #16925

Merged
merged 1 commit into from
Mar 25, 2024
Merged

bump-*-pr: handle HOMEBREW_NO_GITHUB_API=1 being set #16925

merged 1 commit into from
Mar 25, 2024

Conversation

gibfahn
Copy link
Contributor

@gibfahn gibfahn commented Mar 20, 2024 •
edited
Loading

  • 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?

Commits (oldest to newest)

e11f797 bump-*-pr: handle HOMEBREW_NO_GITHUB_API=1 being set

We early return here
https://github.com/homebrew/brew/blob/92a4311868322188478d7a90511ec0e8e6b0d7df/Library/Homebrew/utils/github/api.rb#L220
, but don't then handle that through the stack.

Repro:

❯ HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_GITHUB_API=1 brew bump-formula-pr --write-only --version 1.2.3 --no-audit jq
Error: undefined method `[]' for nil:NilClass
Do not report this issue until you've run `brew update` and tried again.
Warning: Removed Sorbet lines from backtrace!
/opt/homebrew/Library/Homebrew/utils/github.rb:565:in `block in fetch_pull_requests'
/opt/homebrew/Library/Homebrew/utils/github/api.rb:334:in `paginate_graphql'
/opt/homebrew/Library/Homebrew/utils/github.rb:564:in `fetch_pull_requests'
/opt/homebrew/Library/Homebrew/utils/github.rb:628:in `check_for_duplicate_pull_requests'
/opt/homebrew/Library/Homebrew/dev-cmd/bump-formula-pr.rb:456:in `check_open_pull_requests'
/opt/homebrew/Library/Homebrew/dev-cmd/bump-formula-pr.rb:135:in `run'
/opt/homebrew/Library/Homebrew/brew.rb:89:in `<main>'
Rerun with `--verbose` to see the original backtrace

@MikeMcQuaid
Copy link
Member

@gibfahn Can you explain the motivation for this rather than what it does?

@gibfahn
Copy link
Contributor Author

gibfahn commented Mar 20, 2024

We have automation using these commands in taps other than homebrew/core and homebrew/cask.

Also I have wanted to be able to run this locally to do version replacement without needing public internet access.

@gibfahn gibfahn force-pushed the write_only_github branch from ed97ea3 to b3f7cc2 Compare March 20, 2024 22:45
@MikeMcQuaid
Copy link
Member

@gibfahn I still don't understand the motivation here, sorry. You have automation: why/how is that effected by the existing behaviour?

@gibfahn gibfahn force-pushed the write_only_github branch 2 times, most recently from f491dd4 to ab98bfd Compare March 25, 2024 08:56
@gibfahn
Copy link
Contributor Author

gibfahn commented Mar 25, 2024

Sorry, I didn't dig into this enough. Have now used pry to do more digging, and it seems to be a missing nil check caused by HOMEBREW_NO_GITHUB_API=1 being set. We already handle it deeper in the stack, but then don't catch that properly. I updated the PR/commit message with more details.

@gibfahn gibfahn changed the title bump-*-pr: don't check pull requests if --write-only passed bump-*-pr: handle HOMEBREW_NO_GITHUB_API=1 being set Mar 25, 2024
MikeMcQuaid
MikeMcQuaid reviewed Mar 25, 2024
View reviewed changes
Library/Homebrew/utils/github.rb Outdated
@@ -562,6 +562,7 @@ def self.fetch_pull_requests(name, tap_remote_repo, state: nil, version: nil)

pull_requests = []
API.paginate_graphql(graphql_query, variables:) do |result|
return [] if result.nil?
Copy link
Member

@MikeMcQuaid MikeMcQuaid Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can this be moved to the beginning of the fetch_pull_requests method instead and check the specific variable used? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to check the env var directly, let me know if this is what you had in mind.

@gibfahn gibfahn force-pushed the write_only_github branch from ab98bfd to 909b597 Compare March 25, 2024 09:14
MikeMcQuaid
MikeMcQuaid approved these changes Mar 25, 2024
View reviewed changes
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.

Thanks again @gibfahn!

@MikeMcQuaid
Copy link
Member

brew style --fix is required: https://github.com/Homebrew/brew/actions/runs/8417714289/job/23046704929?pr=16925

@gibfahn
bump-*-pr: handle HOMEBREW_NO_GITHUB_API=1 being set
e11f797 
We early return here
https://github.com/homebrew/brew/blob/92a4311868322188478d7a90511ec0e8e6b0d7df/Library/Homebrew/utils/github/api.rb#L220
, but don't then handle that through the stack.

Repro:

```console
❯ HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_GITHUB_API=1 brew bump-formula-pr --write-only --version 1.2.3 --no-audit jq
Error: undefined method `[]' for nil:NilClass
Do not report this issue until you've run `brew update` and tried again.
Warning: Removed Sorbet lines from backtrace!
/opt/homebrew/Library/Homebrew/utils/github.rb:565:in `block in fetch_pull_requests'
/opt/homebrew/Library/Homebrew/utils/github/api.rb:334:in `paginate_graphql'
/opt/homebrew/Library/Homebrew/utils/github.rb:564:in `fetch_pull_requests'
/opt/homebrew/Library/Homebrew/utils/github.rb:628:in `check_for_duplicate_pull_requests'
/opt/homebrew/Library/Homebrew/dev-cmd/bump-formula-pr.rb:456:in `check_open_pull_requests'
/opt/homebrew/Library/Homebrew/dev-cmd/bump-formula-pr.rb:135:in `run'
/opt/homebrew/Library/Homebrew/brew.rb:89:in `<main>'
Rerun with `--verbose` to see the original backtrace
```
@gibfahn gibfahn force-pushed the write_only_github branch from 909b597 to e11f797 Compare March 25, 2024 11:11
@carlocab carlocab merged commit 966f819 into Homebrew:master Mar 25, 2024
23 checks passed
@gibfahn gibfahn deleted the write_only_github branch March 25, 2024 12:02
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 25, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gibfahn @MikeMcQuaid @carlocab