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

.github/ISSUE_TEMPLATE/bug.yml: add instructions for mirror sites bug reporting #11797

Merged
merged 8 commits into from Aug 17, 2021

Conversation

skyzh
Copy link

@skyzh skyzh commented Jul 31, 2021

… reporting

  • 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?
  • (Not applicable) 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?

Regarding previous discussions on mirror sites Homebrew/discussions#1906, we have created a discussion Homebrew/discussions#1917 for mirror maintainers to update mirror information. And then, in this PR, we modified the issue templates to remind our users to see if the issue is related to the mirror sites. Now users could post issues to the best-fit place when they find Homebrew mirrors are not working.

cc @MikeMcQuaid for review, thanks!

- type: markdown
attributes:
value: If you are using `HOMEBREW_BOTTLE_DOMAIN` or `HOMEBREW_ARTIFACT_DOMAIN` to configure Homebrew mirrors, please first try unsetting those variables. If this bug is related to mirror sites themselves, please report the bug to the respective mirror sites. See https://github.com/Homebrew/discussions/discussions/1917 for more information.
- type: textarea
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be better to request unsetting HOMEBREW_{BOTTLE,ARTIFACT}_DOMAIN and HOMEBREW_{BREW,CORE}_GIT_REMOTE as part of the checkboxes below instead.

Copy link
Author

Choose a reason for hiding this comment

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

Agree

Copy link
Member

Choose a reason for hiding this comment

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

It might also be nice for the error message from brew to refer users to the right place for network failures that occur when they have these variables set.

Not sure if there's a way to do this without also carrying around a hash of remotes and associated issue trackers in brew, which I don't think we'll want to do (we don't know how to keep such a list up-to-date), but just mentioning this in case you have ideas here.

Copy link
Author

@skyzh skyzh Jul 31, 2021

Choose a reason for hiding this comment

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

I have added a checkbox and moved instructions for reporting after the checkbox section.

In fact, the failure of mirror sites does not necessarily lead to network issues. There might be

  • HTTP redirections, users are simply redirected to the upstream and eventually succeed.
  • Slow download speed, and users may cancel installation without seeing the error message.

These are all possible failures of mirror sites without a error reported in the console, and I believe we should let them know the right place to go when they want to report issues to Homebrew.

Copy link
Member

Choose a reason for hiding this comment

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

It might also be nice for the error message from brew to refer users to the right place for network failures that occur when they have these variables set.

Not sure if there's a way to do this without also carrying around a hash of remotes and associated issue trackers in brew, which I don't think we'll want to do (we don't know how to keep such a list up-to-date), but just mentioning this in case you have ideas here.

We already have a message that says this for third-party tap issues:

Please report this issue to the rylan12/personal tap (not Homebrew/brew or Homebrew/core), or even better, submit a PR to fix it:
  /usr/local/Homebrew/Library/Taps/rylan12/homebrew-personal/Formula/foo.rb:16

I could definitely see us catching certain errors and saying something like this if one of the env vars is set:

Please report this issue to the maintainers of <MIRROR> mirror. For help using mirrors, see https://github.com/Homebrew/discussions/discussions/1917

That way, no need for a hash for each mirror. Just redirect them to that discussion (which we can structure to try to lead folks to the correct mirror repo (or but reporting location).

And good to have this in here (i.e. this PR), too, in case people ignore that.

Copy link
Author

@skyzh skyzh Aug 1, 2021

Choose a reason for hiding this comment

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

I've taken a quick look at the brew source code (I'm new to Ruby 😂😂), and I don't know where to add the error message for mirrors. I was thinking that I could add a new if branch around where Please report this issue to... is printed out, but this message only shows in check_conflict in formula_installer.rb and odeprecated in utils.rb.

I'm expecting to find some places to handle download errors and add our error messages. I'm also thinking of adding some warnings in brew doctor command. I would do this in another PR. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

brew doctor warnings are probably a good idea. No worries if you can't get the error message thing. It might be very difficult to detect which errors are related to mirrors versus the main brew code, anyway (tbh I'm not sure what types of errors we would want to catch anyway). If you do get it, a separate PR is totally fine.

Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
skyzh and others added 3 commits August 1, 2021 10:31
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
Copy link
Member

@Rylan12 Rylan12 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. Thanks, @skyzh!

.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
@skyzh skyzh requested a review from carlocab August 1, 2021 14:16
@@ -25,6 +25,11 @@ body:
required: true
- label: I have resolved all warnings from `brew doctor` and that did not fix my problem.
required: true
- label: I have not set `HOMEBREW_BOTTLE_DOMAIN`, `HOMEBREW_ARTIFACT_DOMAIN`, `HOMEBREW_BREW_GIT_REMOTE`, or `HOMEBREW_CORE_GIT_REMOTE` and am still able to reproduce this issue.
Copy link
Member

Choose a reason for hiding this comment

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

@Homebrew/maintainers I'm debating whether we should just warn on these being set in brew doctor. That seems in keeping with the "you can do this but we don't really support it and want you to try without before you report it"?

Copy link
Member

Choose a reason for hiding this comment

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

I’d approve of the idea. The fewer checkboxes there are in an issue, the likelier users will read them and not just tick blindly.

Copy link
Author

Choose a reason for hiding this comment

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

+1 for this. So let's just add an entry on new issue page about how to report to public mirror sites, and remove this checkbox in bug report template?

@skyzh
Copy link
Author

skyzh commented Aug 17, 2021

I've updated this PR. So the only thing we need is to show "how to report to mirror" in the entry page of submitting an issue.

cc @vitorgalvao @MikeMcQuaid PTAL, many thanks for reviewing!

@skyzh
Copy link
Author

skyzh commented Aug 17, 2021

Shall we get this merged?

@MikeMcQuaid
Copy link
Member

@skyzh yup, just rerunning CI 👍🏻

@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @skyzh!

@MikeMcQuaid MikeMcQuaid merged commit 7656ddd into Homebrew:master Aug 17, 2021
@skyzh skyzh deleted the patch-1 branch August 18, 2021 03:41
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2021
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.

None yet

5 participants