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

Write a maintainer's guide #1209

Closed
waldyrious opened this issue Dec 21, 2016 · 27 comments · Fixed by #1897
Closed

Write a maintainer's guide #1209

waldyrious opened this issue Dec 21, 2016 · 27 comments · Fixed by #1897
Labels
community Issues/PRs dealing with role changes and community organization. decision A (possibly breaking) decision regarding tldr-pages content, structure, infrastructure, etc. documentation Issues/PRs modifying the documentation.

Comments

@waldyrious
Copy link
Member

We need to have a well-defined (public) procedure for handling PRs, since there's a chronic issue of a PR backlog we can't seem to get rid of entirely. It could be something like:

  • PRs will be merged once they (1) pass the automated tests (Travis CI and CLA signing) without issues, (2) have the review comments addressed (3) get the thumbs-up of two maintainers -- the second one can do the merging immediately after accepting.
  • If a week goes by after (1) and (2) are satisfied, but no second maintainer has manifested their approval, the first maintainer can merge.
  • If a month goes by without reaction from the submitter (e.g. to fix lint errors or review comments), the PR should be taken over by a maintainer who should ensure that at least the page conforms to the contribution guidelines and passes Travis CI before merging.
    • Here, "taking over" means making any necessary changes, including rebase, reword of commit messages, and change of content, while preserving the authorship metadata (e.g. by amending commits rather than copy-pasting the content)

This should ensure we remain responsive to contributions, and that contributors have a clear notion of what to expect (and when to call out maintainers if they fail to adhere :P)

Any thoughts? Did I miss something?

@waldyrious waldyrious added decision A (possibly breaking) decision regarding tldr-pages content, structure, infrastructure, etc. documentation Issues/PRs modifying the documentation. labels Dec 21, 2016
@agnivade
Copy link
Member

Agree on all points. 💯

I do have some concerns on the 3rd point though. I had spent quite some time merging old PRs. And it seems like the backlog is building up again. Now, I just don't feel like doing the whole exercise again. 😞 And imposing the task of making the necessary changes(which might include reading up the man page and studying the command) to the maintainers is a bit unfair I believe.

I absolutely want to remain responsive to contributions so that everyone feels like they can contribute. But, when a contributor does not respond even after pinging separately, it becomes a bit difficult to go through the task of merging the PR time and again.

@waldyrious
Copy link
Member Author

waldyrious commented Dec 31, 2016

We can leave it at the discretion of the maintainers: the explicit guidelines would make it clear to everyone when the maintainers could override a PR's author without stepping on any toes, but that would be a right, not a duty. This way, if merging the PR would entail too much work, it would be ok to simply close the PR with a note to that effect. Sounds good?

@agnivade
Copy link
Member

agnivade commented Jan 1, 2017

Again, "too much work" is a subjective thing. But yea, I am leaning towards the fact that its okay for us to close a PR, after pinging the contributor and waiting for one week after no response.

@waldyrious
Copy link
Member Author

Sure, the ideia is that either resolution would be acceptable (and predictable), so maintainers would have the option to choose whatever they deem appropriate given the specifics of the situation.

@agnivade
Copy link
Member

agnivade commented Jan 2, 2017

Great. Where do you think we should put this ?

@waldyrious
Copy link
Member Author

I'm thinking of adding this to a GOVERNANCE.md page (something quite simple and free-form -- just a list of the guidelines for maintainership and handling of issues and PRs that we've been following so far). This would also serve as a great place to implement #1117.

Before going ahead, I'd like to hear someone else's opinion -- at least @Ostera's.

@waldyrious
Copy link
Member Author

waldyrious commented Jan 4, 2017

In the meantime, @agnivade, do you think we could repurpose @tldr-bot to (also) perform the necessary pinging after the periods we define here expire? You've been doing that manually, but we should automate as much of this activity as possible, so as to free up contributor/maintainer time for the actual changes :)

The bot could even close PRs as you did in #975 -- the wording you used there seems like a great template we could adopt: friendly, informative, and with clear action paths for alternative resolution.

@agnivade
Copy link
Member

agnivade commented Jan 5, 2017

That sounds like a nice idea for a new project. I don't think there are any tools currently which do that(need to check though).

However, we still have to resolve the original problem of tldr-bot (the issue of it not getting the token for PRs coming from a different repo). Getting a separate server for ourselves seems to be the only way to go.

@waldyrious
Copy link
Member Author

Sure. I guess @Ostera also could have something to say here :)

I wonder if there's any host for server-side services that offers a free plan for FOSS projects, though, like Travis and others do for the CI side of things. Any ideas?

@agnivade
Copy link
Member

agnivade commented Jan 5, 2017

We could use heroku, but their free tier is too poor. Don't know about any other services.

@jeeftor
Copy link
Collaborator

jeeftor commented Feb 19, 2017

Could ya'll also add more maintainers? How many of you guys are actually active?

@agnivade
Copy link
Member

Could ya'll also add more maintainers?

Yea absolutely. I have been thinking of adding new ppl. More thoughts on that here - #1117.

How many of you guys are actually active?

Me and @waldyrious usually are. @Ostera also chimes in from time to time.

Will you be interested in joining as a member ? 😉 Don't hesitate to hop on to our gitter channel if you have more questions.

@agnivade
Copy link
Member

@waldyrious - Now that you are back, would you like to take care of this -

I'm thinking of adding this to a GOVERNANCE.md page (something quite simple and free-form -- just a list of the guidelines for maintainership and handling of issues and PRs that we've been following so far). This would also serve as a great place to implement #1117.

This has been quite some time pending, and pretty important now that we have a new collaborator(@jeeftor).

@waldyrious
Copy link
Member Author

@waldyrious - Now that you are back

Slowly doing so -- there's a big backlog to process, but I'll get it done :)

would you like to take care of this

just the other day I submitted such a document to another project I help maintain, and I'm planning on submitting something quite similar for tldr-pages. I will probably prioritize handling the open PRs first, but I've set a reminder to come back to this once that backlog is at least down to a manageable size (in terms of my involvement).

@waldyrious
Copy link
Member Author

waldyrious commented Mar 21, 2017

As a note to self: some of the things we'll want in that document are:

  • a guideline that all trivial fixes (typos, syntax fixes, etc.) are to be made by the maintainers
  • an explicit mention of our principles of enabling web-based contributions as much as possible (including offering to perform rebases or other complex git operations on behalf of contributors, or helping them if they want to do it themselves).

We'll also want to get useful recommendations from https://opensource.guide and maintainerati.

@waldyrious
Copy link
Member Author

waldyrious commented May 2, 2017

For future reference, here's a link to a summary I made for @sbrl in the project's Gitter chat room. Quoted below for convenience:

  • We generally prefer to have at least two maintainers review each PR, so for example if one reviews and approves it, the other can merge the PR once they agree as well (unless someone else raises objections).
    If other maintainers are too busy and a PR lingers around for too long without a second maintainer’s approval, they can be merged with a single maintainer’s approval, in the interests of providing a timely response to contributions
  • Generally we try to avoid merge commits, to allow a cleaner git history. If the commits in the PR are worth keeping separately (i.e. they represent semantic changes rather than typo fixes or syntax corrections revealed by TravisCI, have descriptive commit messages, etc.), then the "rebase and merge” option is the preferred one.
    Otherwise, if they’re just a bunch of commits named “edited command.md” or similar, then the "squash and merge” option is to be used instead.
  • For simple, uncontroversial fixes and typos to PR files, it’s best if the maintainer does them rather than ask the contributor, especially because the actual content-related discussion may end up requiring quite a bit of changes
    In such cases, it’s preferable to either use “rebase and merge” option on the github web interface, or an interactive rebase in the comand line, to clean up any commits that would need to be squashed, reworded, etc.

@waldyrious
Copy link
Member Author

waldyrious commented Jun 16, 2017

From #1407 (comment):

when merging PRs by squashing, it's generally recommended to edit the commit message body and remove any superfluous details, or those that only make sense within the sequence of changes made during the PR (in effect, ensuring the whole thing is written as if it was a single commit from the start).

For PRs that introduce new pages, it would suffice to use a single-line commit message (without a body) saying "<command>: add page", unless the submitter includes details in the PR thread such as the URL for the manpage from where the tldr-page was derived, or other similar metadata.

@waldyrious
Copy link
Member Author

waldyrious commented Aug 22, 2017

From #1429 (comment):

[whether to edit PRs by other users] is generally subjective, but the rule of thumb I've been using is a mix of three criteria:

  1. how many changes we've already requested from the author (not individual commits, but review rounds separated by new commits they make), and whether they're showing signs of fatigue (less enthusiastic responses, slower reactions)
  2. having all commits in a PR by the same author, ideally, so they can be squashed or rebaseed cleanly at the end of the review process.
  3. how subjective is the change, e.g. uncontroversial stuff like typos or formatting to match the project guidelines vs. rewordings, reorderings, etc.

I.e., on the one hand we don't want to be overly burdensome for contributors, and thus should do stuff ourselves when possible, but on the other hand we do want to give full credit to contributors (and have a clean git history) when they did pretty much all of the work. There isn't a single answer; it's definitely a subtle balance we must handle.

@waldyrious waldyrious changed the title Define PR merge guidelines Write a maintainer's guide Aug 22, 2017
@agnivade agnivade mentioned this issue Dec 17, 2017
5 tasks
@waldyrious
Copy link
Member Author

@agnivade and @sbrl: now that #1117 was fixed via #1839, which included a brand new maintainers-guide.md, my next goal is to complete it to close this issue.

Can you review that document and the comments above, and let me know if there's anything that isn't covered?

@agnivade
Copy link
Member

I believe these points were not covered ?

We generally prefer to have at least two maintainers review each PR, so for example if one reviews and approves it, the other can merge the PR once they agree as well (unless someone else raises objections).
If other maintainers are too busy and a PR lingers around for too long without a second maintainer’s approval, they can be merged with a single maintainer’s approval, in the interests of providing a timely response to contributions

@waldyrious
Copy link
Member Author

I believe these points were not covered ?

Sorry, I wasn't clear. I meant covered by the combination of the current document + the comments above. Meaning that I'll complete that document to include everything mentioned above, plus anything else you guys think is missing.

@sbrl
Copy link
Member

sbrl commented Jan 10, 2018

Right! Yeah, I think everything is either covered by the comments here or the new maintainers guide 😺

@agnivade
Copy link
Member

Oops 😅 Yep, everything is covered. I dont have anything to add.

@waldyrious
Copy link
Member Author

Cool! I'll get to work on this as soon as I can. For self-reference, Homebrew's invitation message for new maintainers has some useful notes at the end.

@waldyrious
Copy link
Member Author

Oh, one more thing that just occurred to me: maintainers are expected to hang around in the Gitter chat room on a regular basis, or at least show up every now and then.

@agnivade
Copy link
Member

Good point, let's add that.

@sbrl
Copy link
Member

sbrl commented Jan 12, 2018

Ah, yeah. I have it open as a pinned tab on my home laptop.

@waldyrious waldyrious added the community Issues/PRs dealing with role changes and community organization. label Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Issues/PRs dealing with role changes and community organization. decision A (possibly breaking) decision regarding tldr-pages content, structure, infrastructure, etc. documentation Issues/PRs modifying the documentation.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants