-
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
Conversation
8b07351
to
b32ac31
Compare
Looks great! :) Thanks for taking the time to clarify this, and for the balanced approach. |
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.
Looks good to me. Thanks for taking the time to clarify this.
|
||
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 |
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?
Merge to master, using defensive CI/CD techniques such as feature toggles
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.
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!
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.
I think most of my concerns are about following up and laying out clear expectations for CC's & maintainers around merge readiness. Seems worth revising the CC rights and resonsibilities to generalize or remove any 2U-specific provisions and also make them more generic so we can then say they apply to maintainers as well.
Further discussion here, but I don't think that needs to block this PR.
ca153f2
to
911a38a
Compare
This ADR clarifies the rights and responsibilities of Maintainers with respect to the Core Contributors.
Much of this interaction was previously inferred, but as it was not spelled out, there has been some confusion with respect to how much control Maintainers have over Core Contributors. In short, Maintainers do not control Core Contributors; CCs have passed a nomination process and have gained the support of the community to make good decisions in the repository. However, Maintainers are allowed to know who has CC rights on their repo, communicate repo standards and architectural vision to them, and initiate CC remove procedures if a CC is not following community standards and is unresponsive to feedback.
Relevant parts of OEP-54 Core Contributors & OEP-55 Maintainers are cited within the ADR to make clear that this ADR is not a new idea but rather cements the intentions of the original OEP authors.
Rendered: https://open-edx-proposals--566.org.readthedocs.build/en/566/processes/oep-0055/decisions/0005-managing-core-contributors.html