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

Clarify docs for how PRs should be completed by maintainers #7829

Merged
merged 1 commit into from Jun 25, 2020

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Jun 25, 2020

  • 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 tests with your changes locally?

Clarify some of the wording in the maintainer docs. The main thing that was missing was that pr-publish wasn't necessary for existing formulae because an approving review automatically triggers the bottles to be published.

Of course, suggestions to improve working are welcome 😄

@Rylan12 Rylan12 requested a review from SMillerDev June 25, 2020 14:06
@SMillerDev
Copy link
Member

@iMichka @jonchang since you wrote this flow a check would be appreciated

docs/Homebrew-homebrew-core-Merge-Checklist.md Outdated Show resolved Hide resolved
docs/New-Maintainer-Checklist.md Outdated Show resolved Hide resolved
@jonchang
Copy link
Contributor

jonchang commented Jun 25, 2020

If the goal is to describe what maintainers should do, then the docs in the Maintainers section of https://docs.brew.sh should be updated. The Bottles page is in the Users section and should be providing a general overview of how the process works for users, and not get into the weeds about the details of the maintainers' workflow. I would almost prefer something simpler like:

Bottles are created using the Brew Test Bot, usually when people submit pull requests to Homebrew. The bottle do block is updated by maintainers when they merge a pull request. For the Homebrew organisations’ taps they are uploaded to and downloaded from Bintray.

From the user's point of view, I don't see why making the distinction between a maintainer running brew pr-publish versus BrewTestBot doing the same is relevant, since their goal (getting the pull request merged) happens regardless.

@Rylan12
Copy link
Member Author

Rylan12 commented Jun 25, 2020

I think that makes sense. I'd still like to update the files that are in the Maintainers section. The reason I'm making this is that it wasn't clear to me so I think it's worthwhile to clarify for future maintainers.

I like your suggestion. I think saying "updated by maintainers" is totally reasonable even if it's automatic (it was automatically triggered by an approving review from a maintainer after all)

@jonchang
Copy link
Contributor

I think that makes sense. I'd still like to update the files that are in the Maintainers section. The reason I'm making this is that it wasn't clear to me so I think it's worthwhile to clarify for future maintainers.

Oops, didn't see that the other files were updated. 👍 on any updates to those docs since you have a better perspective as new maintainer on what's lacking.

@SMillerDev
Copy link
Member

LGTM 👍

@Rylan12
Copy link
Member Author

Rylan12 commented Jun 25, 2020

The only real misconception I had was that pr-publish was always necessary. Otherwise, it was more a matter of connecting the dots.

In general, I'd say just making sure to talk through the procedure with a new maintainer (like @SMillerDev did with me) should be enough to clarify for them.

@Rylan12
Copy link
Member Author

Rylan12 commented Jun 25, 2020

@SMillerDev okay to merge this without an official approval?

@Rylan12 Rylan12 merged commit 12b4912 into Homebrew:master Jun 25, 2020
@Rylan12 Rylan12 deleted the clarify-maintainer-docs branch June 25, 2020 15:52
@MikeMcQuaid
Copy link
Member

Thanks @Rylan12!

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 27, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 27, 2020
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