Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: Display a warning (instead of exiting early) when unable to find recent issues on a tap #17182

Conversation

boblail
Copy link
Contributor

@boblail boblail commented Apr 29, 2024

Occasionally, when failing to brew install or brew upgrade a formula, users will get an error message and the last line of it will be:

Error: Validation Failed: [{"message"=>"The listed users and repositories cannot be searched either because the resources do not exist or you do not have permission to view them.", "resource"=>"Search", "field"=>"q", "code"=>"invalid"}]

Here's an old public example: https://gitlab.com/tabos/rogerrouter/-/issues/21

At Square, this error can be caused by a transient issue with a user's GitHub credentials.

My proposal is that any error which prevents brew from showing related issues should:

  1. Not cause brew to exit early (vs. printing diagnostic checks and doing any other cleanup)
  2. Be clearly differentiated from the original failure (so it doesn't become a red herring for users troubleshooting their broken formula)

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

…on and unable to find related issues

Occasionally, when failing to `brew install` or `brew upgrade` a formula, users will get an error message and the last line of it will be:
```
Error: Validation Failed: [{"message"=>"The listed users and repositories cannot be searched either because the resources do not exist or you do not have permission to view them.", "resource"=>"Search", "field"=>"q", "code"=>"invalid"}]
```

Here's an old public example: https://gitlab.com/tabos/rogerrouter/-/issues/21

At Square, this error can be caused by a transient issue with a user's GitHub credentials.

My proposal is that any error which prevents `brew` from showing related issues should:
1. Not cause `brew` to exit early (vs. printing diagnostic checks and doing any other cleanup)
2. Be clearly differentiated from the original failure (so it doesn't become a red herring for users troubleshooting their broken formula)
rescue GitHub::API::RateLimitExceededError => e
opoo e.message
rescue GitHub::API::Error => e
opoo "Unable to query GitHub for recent issues on the tap\n#{e.message}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added text to the beginning of the error message to explain that the error isn't related to the original exception

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.

This makes perfect sense to me. There is no reason we should be failing inside an error while querying Github so catching all Github API errors is reasonable. It's only used to print some extra debug information anyway so skipping it isn't the end of the world if the request fails for some reason.

It seems to only be used here.

if issues.present?
puts "These open issues may also help:"
puts issues.map { |i| "#{i["title"]} #{i["html_url"]}" }.join("\n")
end

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks!

@carlocab carlocab merged commit 7690eb3 into Homebrew:master Apr 30, 2024
25 checks passed
@MikeMcQuaid
Copy link
Member

Thanks again @boblail!

@boblail
Copy link
Contributor Author

boblail commented Apr 30, 2024

Thanks all!

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

Successfully merging this pull request may close these issues.

4 participants