-
Notifications
You must be signed in to change notification settings - Fork 35
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
OEP-55:ADR-5 Clarify Maintainer Interaction with Core Contributors #566
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
71 changes: 71 additions & 0 deletions
71
oeps/processes/oep-0055/decisions/0005-managing-core-contributors.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
0005 Managing Core Contributors Of Your Maintained Repository | ||
############################################################# | ||
|
||
Status | ||
****** | ||
|
||
**Under Review** | ||
|
||
Context | ||
******* | ||
|
||
Per `OEP-55 | ||
<https://docs.openedx.org/projects/openedx-proposals/en/latest/processes/oep-0055-proc-project-maintainers.html>`_, | ||
repository maintainers are responsible "for the health of the component and the | ||
component's community". Further, OEP-55 specifies that maintainers must | ||
participate in the "selection and approval of Core Contributors with commit | ||
rights to their repositories, by participating in the comment period for Core | ||
Contributor nominees nominated to commit code to their repo(s)". | ||
|
||
However, OEP-55 does not provide sufficient guidance around how Maintainers and | ||
Core Contributors should interact. | ||
|
||
Decision | ||
******** | ||
|
||
Maintainers must participate in, and support, the Core Contributor program. | ||
Maintainers may not take actions that effectively prevent a Core Contributor | ||
from being able to carry out their Rights, as defined by `OEP-54 | ||
<https://docs.openedx.org/projects/openedx-proposals/en/latest/processes/oep-0054-core-contributors.html>`_. | ||
|
||
Maintainers should communicate architectural principles and code standards to | ||
Core Contributors on their repository. They may initiate the process to remove | ||
Core Contributors that do not follow standards or respond to feedback. | ||
|
||
Specification | ||
************* | ||
|
||
Maintainer Responsibilities | ||
=========================== | ||
|
||
* Maintainers must participate in the selection of Core Contributors to their | ||
repository by expressing their opinion of nominees during the `Core | ||
Contributor nomination process <https://docs.openedx.org/projects/openedx-proposals/en/latest/processes/oep-0054-core-contributors.html#new-core-contributor-nomination-process>`_ | ||
|
||
* Maintainers must carefully weigh the merits of a nominee. Nominees may only be rejected on the basis of their contributions and conduct, not for the sake of limiting the number of contributors. For nominees that are objected to, Maintainers | ||
must provide feedback that includes what the nominee needs to do in order to | ||
become a Core Contributor on the repository. | ||
|
||
* Maintainers may not take any action that would prevent existing Core | ||
Contributors from being able to merge code. This includes, but is not limited | ||
to, setting up branch protection rules that require a Maintainer to approve a | ||
hurtstotouchfire marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pull request before it can be merged. | ||
|
||
Maintainer Rights | ||
================= | ||
|
||
* Maintainers have the right to know who has Commit access on their repository, | ||
sarina marked this conversation as resolved.
Show resolved
Hide resolved
|
||
and communicate their code standards and architectural principles with them. | ||
See the `Core Contributors wiki page <https://openedx.atlassian.net/wiki/spaces/COMM/pages/3156344833/Core+Contributors+to+the+Open+edX+Project>`_ | ||
for a list of all current and former CCs. | ||
New Maintainers taking over maintenance of an existing repository should | ||
especially ensure they are in contact with their repo's CCs. See the `maintainers how-to | ||
<https://docs.openedx.org/en/latest/developers/how-tos/maintain-a-repo.html#your-ccs-are-aligned-with-your-expectations-as-maintainer>`_ | ||
for more detail. | ||
|
||
* Maintainers have the right to remove Core Contributors who are not | ||
arbrandes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
appropriately following Open edX community standards, Core Contributor | ||
standards (`the "3 Cs" | ||
<https://docs.openedx.org/projects/openedx-proposals/en/latest/processes/oep-0054-core-contributors.html#adding-new-core-contributors>`_) | ||
or project expectations, and have also not been responsive to Maintainer feedback. | ||
`OEP-54 specifies how to initiate this process <https://docs.openedx.org/projects/openedx-proposals/en/latest/processes/oep-0054-core-contributors.html#removing-core-contributors>`_. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rights definition encourages the use of feature flags, is that something we also want to change?
The way this right is worded also makes it sound like CC's are responsible for checking for feature flags, which I think would still be correct if the Product Requirements stipulate that one should be used, but maybe feature flags should be fully decoupled from merge rights and considered separately in the context of "meeting product requirements" or something that moves the feature flag to the Product domain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You bring up a good point that I want to spend a bit of time thinking about. I think feature flags can be a good thing (and a thing we want to encourage) in many situations, for example to enable gradual rollout or to gate a new feature for one Named Release before the new behavior becomes default.
Can you point to exactly where this line is coming from? I don't see it when I search for "toggle" in https://docs.openedx.org/projects/openedx-proposals/en/latest/processes/oep-0054-core-contributors.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here: https://openedx.atlassian.net/wiki/spaces/COMM/pages/1529675973/Coding+Contributors+Materials#Committer-Rights-and-Responsibilities
I can't know what the intention was when that was written but I read this as more like "don't merge things that will break other operators' instances or cause surprising behavior", which does still seem valuable, but more like a responsibility than a right. I was looking for a better place to put that kind of information and I think that Merge Guidelines for Coding Core Contributors probably also needs a revamp as there is a lot of 2U specific stuff there. Might even be preferable to pull the contents of that page into an ADR on OEP-54 and hammer out the changes needed. Maintainer expectations for merge-readiness could just refer to the CC expectation.
I think the language in this line of the ADR is fine, we should just clean up the CC rights / responsibilities pages so this can refer to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hurtstotouchfire: I have also had thoughts on merge readiness, so let me know if you ticket or work on any related doc updates. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh yes. Goodness. Those pages definitely need a revamp; I'm embarrassed to say that's been on my todo list since January 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robrap @hurtstotouchfire - feel free to review those pages and leave a pile of comments. For context, Nimisha made those original pages in early 2020 and I believe the last meaningful update was by me in 2021. That is, pre-split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hurtstotouchfire - Feanil & I made a bunch of updates on these pages last week. Please feel free to provide a review, if you have time!