Updating the contributing.md by LHinson · Pull Request #302 · patternfly/patternfly-react

LHinson · 2018-04-10T15:24:45Z

I have updated the contributing.md to include information about how to become a maintainer.

…me a maintainer.

coveralls · 2018-04-10T15:29:40Z

Pull Request Test Coverage Report for Build 1167

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.5%) to 73.966%

Totals
Change from base Build 1127:	1.5%
Covered Lines:	1445
Relevant Lines:	1782

💛 - Coveralls

ohadlevy · 2018-04-10T15:34:53Z

  - [Code Consistency](#code-consistency)
  - [Code Contribution Guidelines](#code-contribution-guidelines)
+- [Becoming a Maintainer](#becoming-a-maintainer)
+  - [How do I become a maintainer?](how-do-i-become-a-maintainer?)


maintainer or committer? there might be a slight difference between the two

@ohadlevy yeah I noticed this discrepancy between Foreman and PF. For PatternFly, each repo has two sets of teams, "Contributors" and "Maintainers". Therefore I decided to keep consistent with our current definitions as seen on our teams page: https://github.com/orgs/patternfly/teams

WDYT?

ohadlevy · 2018-04-10T15:36:03Z

+
+If you want to become a maintainer, we expect you to:
+
+- Review and test pull requests submitted by others .


nitpick: extra space EOL before the .

dlabrecq

Would these nomination rules apply to UXD devs who wish to be a maintainer?

dlabrecq · 2018-04-10T19:22:44Z

+
+### Quick tips for new maintainers
+
+- If something you merged broke something, it’s your responsibility to provide a fix.


I don't agree with this how this statement is worded. I believe this applies more to a committer, not necessarily the person merging a PR.

It's typically not the maintainer's responsibility to provide fixes for code they did not write? The person who wrote the code really should be the one to provide a fix.

Perhaps it should be the maintainer's responsibility to back out a commit that caused the breakage? That is, until the developer of the code can provide a fix.

Perhaps better worded as "All maintainers are responsible for the health of the repo. If unit tests, doc/example site, or CI (travis, jenkins) fails; you are responsible for investigating and/or providing a fix".

I don't object to @dtaylor113's comment, but I also like being explicit about what to do if you merged a PR and it broke something. I agree with Dan's comment that the person merging might not be the best person to fix the issue, and we should give them the option to back out a commit.

yes - this has happened in the past and during high activity periods, i think a judgement call should be made about other contributors submitting a fix (if the committer can't provide one immediately). But usually it is just an open discussion...but yes, agree w/ @dtaylor113 ' s point about all maintainers trying to retain the health of the repo at all times. The test coverage/CI tooling in this repo is steadily improving so hopefully this is less and less of an issue..

dlabrecq · 2018-04-10T19:28:24Z

+
+### How do I lose maintainers status?
+
+If you are inactive in the community for one year, we will remove you from the maintainers list and revoke your permission, but we will make a mention of you on a  list of previous maintainers.


One year is very generous.

dlabrecq · 2018-04-10T19:31:22Z

+If you are inactive in the community for one year, we will remove you from the maintainers list and revoke your permission, but we will make a mention of you on a  list of previous maintainers.
+
+In the event that a maintainer continues to disregard good citizenship (or actively disrupts the project), we may need to revoke that person’s status. The process is the same as for nominating a new maintainer: someone suggests the revocation with a good reason, two people second the motion, and a vote may be called if consensus cannot be reached. We hope that’s simple enough, and that we never have to test it in practice.
+


The process for nominating a person sounds good, but the same process for revoking? I can see that publicly revoking a person's privileges could get ugly. This should probably be done more discreetly so people are not publicly shamed.

Similar to Dan's comment, if I can privately object to a nomination, then should I be able to privately request that their status be removed? If that's the case, who would I send my request to?

dtaylor113 · 2018-04-11T13:31:35Z

Not part of the changes for this PR, just general comment for Contribution Process ->
Creating Issues for Bugs:

In the issue tracker, check if the bug has already been reported.
If it does exist, ...add a comment to the existing bug.

For existing bugs, OpenShift/Bugzilla also have a practice of updating a counter, or adding +1', whenever a dev/user hits the bug. Good indication of frequency of occurrence.

dtaylor113 · 2018-04-11T13:48:16Z

+- Review and test pull requests submitted by others.
+- Encourage and ensure design remains an integral part of the review process and pull in designers for review as needed (you can leverage @patternfly/pf-<framework>-design if there is no known associated designer).
+- Maintain sustained activity versus sporadic.
+- Support users and other developers on [PatternFly Slack](https://patternfly.slack.com/) and the [mailing list](mailto:[email protected]).


Probably want to add the pf-react slack channel and mailing list.

There is no react ML. I will update the slack info though.

jgiardino · 2018-04-11T14:58:58Z

+If you want to become a maintainer, we expect you to:
+
+- Review and test pull requests submitted by others.
+- Encourage and ensure design remains an integral part of the review process and pull in designers for review as needed (you can leverage @patternfly/pf-<framework>-design if there is no known associated designer).


@patternfly/pf-<framework>-design should be updated to @patternfly/patternfly-react-ux

priley86 · 2018-04-11T17:25:36Z

+### Quick tips for new maintainers
+
+- If something you merged broke something, it’s your responsibility to provide a fix.
+- Do not merge your own commits


does this tie in to the existing approval process agreed upon in pf-react? I guess it is good to distinguish that we don't self-merge when one becomes a maintainer...and we typically wait for ux/design/development review (unless a PR doesn't pertain such as tests/build/tooling changes/dependency changes only). This repo is mission critical now for many teams so it's important to keep it healthy as we introduce new components/new build processes/new tooling/new workspaces for development. I believe that additional maintainers will help further this and continue to improve the quality overall, as well as the performance downstream.

priley86 · 2018-04-11T17:26:48Z

exciting - thanks @LHinson 🌟

LHinson · 2018-04-14T11:12:52Z

Thanks everyone for the awesome review! @jgiardino @dtaylor113 @dlabrecq @priley86 @ohadlevy @jeff-phillips-18 I've made updates per your recommendations.

@dlabrecq becoming a maintainer can apply to anyone, not just UXD devs.

@dtaylor113 I didn't update the part about code since I'm not so knowledgable about this. File an issue to update this in a separate PR.

jeff-phillips-18 · 2018-04-17T15:19:00Z

@dlabrecq @priley86 @dtaylor113 @ohadlevy @jgiardino Any further comments?

jgiardino

Looks good!

Updating the contributing.md to include information about how to beco…

22a21cc

…me a maintainer.

jeff-phillips-18 previously approved these changes Apr 10, 2018

View reviewed changes

ohadlevy reviewed Apr 10, 2018

View reviewed changes

removes the extra space

ec29cf4

LHinson dismissed jeff-phillips-18’s stale review via ec29cf4 April 10, 2018 16:23

dlabrecq reviewed Apr 10, 2018

View reviewed changes

dtaylor113 reviewed Apr 11, 2018

View reviewed changes

jgiardino reviewed Apr 11, 2018

View reviewed changes

priley86 reviewed Apr 11, 2018

View reviewed changes

Updates per all the great feedback!

4548247

jeff-phillips-18 approved these changes Apr 16, 2018

View reviewed changes

dlabrecq approved these changes Apr 17, 2018

View reviewed changes

priley86 approved these changes Apr 17, 2018

View reviewed changes

jgiardino approved these changes Apr 17, 2018

View reviewed changes

jeff-phillips-18 merged commit b755cdb into patternfly:master Apr 17, 2018

This was referenced Apr 27, 2018

adding how to become a maintainer doc to contributing.md patternfly/angular-patternfly#736

Merged

how to become a maintainer doc updates patternfly/patternfly-design#654

Merged

how to become a maintainer doc patternfly/patternfly-ng#339

Merged


		If you want to become a maintainer, we expect you to:

		- Review and test pull requests submitted by others .


		### Quick tips for new maintainers

		- If something you merged broke something, it’s your responsibility to provide a fix.


		### How do I lose maintainers status?

		If you are inactive in the community for one year, we will remove you from the maintainers list and revoke your permission, but we will make a mention of you on a list of previous maintainers.

		If you are inactive in the community for one year, we will remove you from the maintainers list and revoke your permission, but we will make a mention of you on a list of previous maintainers.

		In the event that a maintainer continues to disregard good citizenship (or actively disrupts the project), we may need to revoke that person’s status. The process is the same as for nominating a new maintainer: someone suggests the revocation with a good reason, two people second the motion, and a vote may be called if consensus cannot be reached. We hope that’s simple enough, and that we never have to test it in practice.

Conversation

LHinson commented Apr 10, 2018

Uh oh!

coveralls commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1167

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtaylor113 commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 commented Apr 11, 2018

Uh oh!

LHinson commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeff-phillips-18 commented Apr 17, 2018

Uh oh!

jgiardino left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

coveralls commented Apr 10, 2018 •

edited

Loading

dlabrecq left a comment •

edited

Loading

dlabrecq Apr 10, 2018 •

edited

Loading

dlabrecq Apr 10, 2018 •

edited

Loading

LHinson commented Apr 14, 2018 •

edited

Loading