-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add initial draft of Podman project Governance #25398
base: main
Are you sure you want to change the base?
Conversation
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.
Overall looks good to me.
One piece that I think is missing is to describe the github user permissions. I think we should likely define that here so that the permissions of each contributor are clearly defined somewhere, e.g. core maintainers will be admin/owner of the github org, maintainers will be admin in their specific repo(s) but not org wide, a reviewer should likely only get triage permissions then as they are not allowed to merge
|
||
A Maintainer must meet the responsibilities and requirements of a Reviewer, plus: | ||
* Responsibilities include: | ||
* Sustained high level of reviews of pull requests to the project or subproject, with a goal of one or more a week when averaged across the year. |
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 like cncf added us to their stat tracking, I have not looked into how they count PR reviews but here is the filter I came up with if we consider reviews over the last year per week, on the right it shows the averages for each person.
Nothing unusual in terms of names there, personally I think 1 is a pretty low bar but at the same time I am also not a fan of having "arbitrarily" interactions counts, because then people might start to game the metrics.
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.
Confirm. I think one a week is a bit high for a maintainer; I'd go one a month. However, this is a reasonable level for a core maintainer. I, too, dislike an arbitrary number
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.
and re-reading, it's a goal, not a requirement, so not as bad.
GOVERNANCE.md
Outdated
|
||
## Contact | ||
* For inquiries, please reach out to: | ||
* Tom Sweeney, Community Manager |
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.
might as well add the email address here so people don't have to look up contact info elsewhere
GOVERNANCE.md
Outdated
* Has participated in pull request review and/or issue triage on the project for at least 6 months. The time requirement may be overridden by a supermajority vote of Maintainers. | ||
* Is supportive of new and occasional contributors and helps get useful PRs in shape to merge | ||
* Additional privileges: | ||
* Has rights to approve pull requests in the Podman project or a subproject |
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 don't think it is defined what approving means? It is not clear to me what it means, is it simply to say it has been reviewed by a reviewer?
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.
Github's "approve" functionality, and/or the /approve
function in the bot
51408e0
to
027818e
Compare
MAINTAINERS.md
Outdated
|
||
## Maintainers | ||
|
||
| Maintainer | GitHub ID | Project Roles | Affiliation | |
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.
table seems broken, it is not rendered correctly on github
https://github.com/containers/podman/blob/027818e4e4a610f6587d70fceadc86135aec8750/MAINTAINERS.md
MAINTAINERS.md
Outdated
| Jake Correnti | [jakecorrenti](https://github.com/jakecorrenti) | Reviewer | [Red Hat](https://www.github.com/redhat/) | | ||
| Ashley Cui | [ashley-cui](https://github.com/ashley-cui) | Maintainer | [Red Hat](https://www.github.com/redhat/) | | ||
| Nalin Dahyabhai | [nalind](https://github.com/nalind) | Core Maintainer | [Red Hat](https://www.github.com/redhat/) | | ||
| Jason Greene | [n1hility](https://github.com/n1hility) | Reviewer | [Red Hat](https://www.github.com/redhat/) | |
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.
Jason has not been active in a while here I think? Does he want to be reviewer? If we assign roles with responsibilities then we should likely ask everyone here if they are good with that but maybe you have done that?
But I guess this question applies to everyone here really?
MAINTAINERS.md
Outdated
| Aditya Rajan | [flouthoc](https://github.com/flouthoc) | Reviewer | [Red Hat](https://www.github.com/redhat/) | | ||
| Valentin Rothberg | [vrothberg](https://github.com/vrothberg) | Reviewer | [Red Hat](https://www.github.com/redhat/) | | ||
| Giuseppe Scrivano | [giuseppe](https://github.com/giuseppe) | Core Maintainer | [Red Hat](https://www.github.com/redhat/) | | ||
| Neil Smith | [Neil-Smith](https://github.com/Neil-Smith) | Community Manager | [Red Hat](https://www.github.com/redhat/) | |
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.
Being "Community Manager" but per the governance a "Community Manager" is also a Maintainer and I think that is not right for Neil as he doesn't fulfil other important Maintainer tasks.
And another thing that is unclear if one is listed as "Community Manager" are they a normal maintainer or core maintainer? Maybe it would be better to define "Community Manager" as a role that is not tied to the maintainer/reviewer roles.
Then someone can only be a Community Manager. And for a Maintainer such as Tom we could just list as Community Manager and Core Maintainer?
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 originally had Community Manager as a separate role, but moved it into Maintainer at the suggestion of @baude - I have no issues with separating it again.
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 that is mischaracterized a bit. My opinion is that a community manager should not just be anyone off the street. As such, I would agree that community manager in itself does not entitle you to privileges to the repository. In other words, you can be a community manager and maintainer; but being a community manager does not make you maintainer.
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.
Updated to make Community Manager a separate role.
I'm going to push another draft this afternoon which includes changes to the Community Manager role to separate it from maintainer. |
4599a02
to
6da0973
Compare
Updated version pushed with community manager definition changed. I think we can probably go ahead and merge this, in a non-binding, open for changes form, early next week. |
MAINTAINERS.md
Outdated
|-------------------|----------------------------------------------------------|----------------------------------|-------------------------------------------| | ||
| Brent Baude | [baude](https://github.com/baude) | Core Maintainer | [Red Hat](https://www.github.com/redhat/) | | ||
| Ygal Blum | [ygalblum](https://github.com/ygalblum) | Maintainer | [Red Hat](https://www.github.com/redhat/) | | ||
| Jake Correnti | [jakecorrenti](https://github.com/jakecorrenti) | Reviewer | [Red Hat](https://www.github.com/redhat/) | |
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 Red Hat GH link here is some random user. The proper link is https://github.com/RedHatOfficial
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.
Excellent catch, TY
Cockpit tests failed for commit d7d744f. @martinpitt, @jelly, @mvollmer please check. |
GOVERNANCE.md
Outdated
|
||
## Contributor Ladder | ||
|
||
Hello! We are excited that you want to learn more about our project contributor ladder! This contributor ladder outlines the different contributor roles within the project, along with the responsibilities and privileges that come with them. Community members generally start at the first levels of the "ladder" and advance up it as their involvement in the project grows. Our project members are happy to help you advance along the contributor ladder. At all levels, contributors are required to follow the CNCF Code of Conduct (COC). |
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.
Hello! We are excited that you want to learn more about our project contributor ladder! This contributor ladder outlines the different contributor roles within the project, along with the responsibilities and privileges that come with them. Community members generally start at the first levels of the "ladder" and advance up it as their involvement in the project grows. Our project members are happy to help you advance along the contributor ladder. At all levels, contributors are required to follow the CNCF Code of Conduct (COC). | |
Hello! We are excited that you want to learn more about our project contributor ladder! This contributor ladder outlines the different contributor roles within the project, along with the responsibilities and privileges that come with them. Community members generally start at the first levels of the "ladder" and advance as their involvement in the project grows. Our project members are happy to help you advance along the contributor ladder. At all levels, contributors are required to follow the CNCF Code of Conduct (COC). |
GOVERNANCE.md
Outdated
### Reviewer | ||
Description: A Reviewer has responsibility for the triage of issues and review of pull requests on the Podman project or a subproject, consisting of one or more of the Git repositories that form the project. They are collectively responsible, with other Reviewers, for reviewing changes to the repository or repositories and indicating whether those changes are ready to merge. They have a track record of contribution and review in the project. | ||
|
||
Reviewers have all the rights and responsibilities of an Contributor, plus: |
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.
Reviewers have all the rights and responsibilities of an Contributor, plus: | |
Reviewers have all the rights and responsibilities of a Contributor, plus: |
GOVERNANCE.md
Outdated
* Regular contribution of pull requests to the Podman project or its subprojects | ||
* Triage of Github issues on the Podman project or its subprojects | ||
* Regularly fixing Github issues on the Podman project or its subprojects | ||
* Following the reviewing guide |
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.
Can we link to the guideline if there is one?
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.
Done
GOVERNANCE.md
Outdated
#### The process of becoming a Reviewer is: | ||
1. The contributor must be sponsored by a Maintainer. That sponsor will open a PR against the appropriate repository, which adds the contributor to the MAINTAINERS.md file as a reviewer. | ||
2. The contributor will add a comment to the pull request indicating their willingness to assume the responsibilities of a Reviewer. | ||
3. At least two members of the team that owns the repository in question, who are already Maintainers, must concur to merge the PR. |
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.
3. At least two members of the team that owns the repository in question, who are already Maintainers, must concur to merge the PR. | |
3. At least two team members that own the repository in question, who are already Maintainers, must concur to merge the PR. |
GOVERNANCE.md
Outdated
3. At least two members of the team that owns the repository in question, who are already Maintainers, must concur to merge the PR. | ||
|
||
### Maintainer | ||
Description: Maintainers are very established contributors with deep technical knowledge of the Podman project and/or one of its subprojects. Maintainers are granted the authority to merge pull requests, and are expected to participate in making decisions about the strategy and priorities of the project. Maintainers are responsible for code review and merging in a single repository or subproject. It is possible to become Maintainer of additional repositories or subprojects, but each additional repository or project will require a separate application and vote They are able to participate in all maintainer activities, including Core Maintainer meetings, but do not have a vote at Core Maintainer meetings. |
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.
Description: Maintainers are very established contributors with deep technical knowledge of the Podman project and/or one of its subprojects. Maintainers are granted the authority to merge pull requests, and are expected to participate in making decisions about the strategy and priorities of the project. Maintainers are responsible for code review and merging in a single repository or subproject. It is possible to become Maintainer of additional repositories or subprojects, but each additional repository or project will require a separate application and vote They are able to participate in all maintainer activities, including Core Maintainer meetings, but do not have a vote at Core Maintainer meetings. | |
Description: Maintainers are very established contributors with deep technical knowledge of the Podman project and/or one of its subprojects. Maintainers are granted the authority to merge pull requests, and are expected to participate in making decisions about the strategy and priorities of the project. Maintainers are responsible for code review and merging in a single repository or subproject. It is possible to become Maintainer of additional repositories or subprojects, but each additional repository or project will require a separate application and vote. They are able to participate in all maintainer activities, including Core Maintainer meetings, but do not have a vote at Core Maintainer meetings. |
GOVERNANCE.md
Outdated
* Additional privileges: | ||
* Represent the project in public as a senior project member | ||
* Communicate with the CNCF on behalf of the project | ||
* Have a voice, but not a vote, in Core Maintainer decision-making meetings |
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.
* Have a voice, but not a vote, in Core Maintainer decision-making meetings | |
* Have a voice, but not a vote, in Core Maintainer decision-making meetings |
GOVERNANCE.md
Outdated
* Communicate with the CNCF on behalf of the project | ||
* Have a voice, but not a vote, in Core Maintainer decision-making meetings |
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.
* Communicate with the CNCF on behalf of the project | |
* Have a voice, but not a vote, in Core Maintainer decision-making meetings | |
* Communicate with the CNCF on behalf of the project | |
* Have a voice, but not a vote, in Core Maintainer decision-making meetings |
Ephemeral COPR build failed. @containers/packit-build please check. |
REVIEWING.md
Outdated
This document contains general principles for how to perform code reviews in the Podman repository. | ||
It does not aim to be a complete guide to how to perform code review, but rather to provide general guidance on how code reviews should be performed. | ||
|
||
This document is aimed at Reviewers, Maintainers, and Core Maintainers (see [GOVERNANCE.md](./GOVERNANCE.md) for definitions of these roles), but these guidelines shpould be followed by all who wish to review code in the Podman project's Github repositories, even those who are not currently a maintainer. |
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.
This document is aimed at Reviewers, Maintainers, and Core Maintainers (see [GOVERNANCE.md](./GOVERNANCE.md) for definitions of these roles), but these guidelines shpould be followed by all who wish to review code in the Podman project's Github repositories, even those who are not currently a maintainer. | |
This document is aimed at Reviewers, Maintainers, and Core Maintainers (see [GOVERNANCE.md](./GOVERNANCE.md) for definitions of these roles), but these guidelines should be followed by all who wish to review code in the Podman project's Github repositories, even those who are not currently a maintainer. |
REVIEWING.md
Outdated
If it is not one of those periods, reviewers should be on the lookout for breaking changes. | ||
PRs with breaking changes should not be merged. | ||
Please guide the contributor in how to make the change in a non-breaking fashion. | ||
if this is not possible, deferring the PR to the next breaking change window or closing it entirely is appropriate. |
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.
if this is not possible, deferring the PR to the next breaking change window or closing it entirely is appropriate. | |
If this is not possible, deferring the PR to the next breaking change window or closing it entirely is appropriate. |
REVIEWING.md
Outdated
For all branches except the main branch, this should always remain static. | ||
PRs into a non-main branch which change the supported Go version should be modified to not require such a change, or rejected if that is not possible. | ||
Changing supported Go version in the main branch is allowed, but not encouraged. | ||
Doing so will require significant discussion among maintainers, as it can effect on what distributions the main branch can be built. |
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.
Doing so will require significant discussion among maintainers, as it can effect on what distributions the main branch can be built. | |
Doing so will require significant discussion among maintainers, as it can affect on what distributions the main branch can be built. |
REVIEWING.md
Outdated
|
||
Please avoid bikeshedding during reviews. | ||
|
||
Trivial changes that do not effect the ultimate functionality of the PR - for example, small grammer or phrasing changes in documentation, unnecessary renaming of variables - should not block merge of a PR. |
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.
Trivial changes that do not effect the ultimate functionality of the PR - for example, small grammer or phrasing changes in documentation, unnecessary renaming of variables - should not block merge of a PR. | |
Trivial changes that do not affect the ultimate functionality of the PR - for example, small grammar or phrasing changes in documentation, unnecessary renaming of variables - should not block merge of a PR. |
REVIEWING.md
Outdated
It is acceptable to make such comments, but they should be marked as nice to have changes. | ||
|
||
Also, it should not have to be said, but please be polite during code reviews. | ||
The [CNCF Code of Conduct](https://github.com/cncf/foundation/blob/main/code-of-conduct.md) applies to all reviewers, of course, but in order to encourage repeat contributions, reviewers should be polite and pleasant when performing reviews. |
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.
Politeness can be hard to judge given the same language is spoken differently around the world. Can we add assume good intent
here? Also, if there's a recommended list of words to avoid that can be linked here, that'd be nice I guess.
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.
Sure
REVIEWING.md
Outdated
### Breaking Changes | ||
|
||
The Podman project aims to present a stable API for its users. | ||
Breaking changes to the project's Command Line Interfaces or public APIs (the Podman REST API) must only be made in approved breaking change releases - Podman 6.0, 7.0, etc. |
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 pkg/bindings should be stable as well?
ad2c0c7
to
a02a09a
Compare
GOVERNANCE.md
Outdated
* Advocates for the community in Maintainer and Core Maintainer meetings | ||
* Additional privileges: | ||
* Represent the project in public | ||
* Communicate with the CNCF on behalf of the project |
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.
Do you want just "Communicate" with the CNCF or do you want the ability to represent on behalf of the project?
For example do you want this person to be able to ask for resources, do event coordination with us, submit maintainer CFPs, be able to file support tickets on behalf of other maintainers, access to #maintainers-circle, etc.
If so I'd strengthen this a tad so we know to give them the proper rights.
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 was aiming for the same rights as maintainers, so I'll use the same wording we use for them. Good catch.
GOVERNANCE.md
Outdated
|
||
* Responsibilities include: | ||
* Regular contribution of pull requests to the Podman project or its subprojects | ||
* Triage of Github issues on the Podman project or its subprojects |
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.
"Github" s/b "GitHub" throughout.
* Mentoring new Reviewers | ||
* Participating in CNCF maintainer activities for the projects they are maintainers of | ||
* Assisting Core Maintainers in determining strategy and policy for the project | ||
* Participating in, and leading, community meetings |
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 don't think you can have every maintainer leading the meetings. Also, if this is a common Governance, does the community meeting thing still hold as we only have it for "Podman".
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.
They'll have to be "Podman Project" community meetings from here on out
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.
Is there an official definition of "Podman Project"? From what I understand "containers" is an umbrella project for various projects not necessarily pertaining to Podman, while the Podman Project would include--I assume--projects such as buildah, c/*, netavark, etc.
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 CNCF submission (which they are referring to as Podman Container Tools, but I've just designated the Podman project in this doc) is Podman, Buildah, and Skopeo, which will eventually be getting their own separate GH org (IE moving out of containers/ on Github) to make it very clear what is CNCF and what isn't, but before that happens we'll update the readmes on relevant projects to indicate they are/are not CNCF.
(As for the c/ libraries, Netavark/Aardvark - we can potentially add those to the Podman Project CNCF at a later date as subprojects. We'd like to get settled in and accustomed to running things in public before we expand further, though.)
GOVERNANCE.md
Outdated
* Can commit to maintaining a high level of contribution to the project as a whole | ||
* Additional privileges: | ||
* Represent the project in public as a senior project member | ||
* Represent the project in interactions with the CNCF |
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.
Do you have too many tab/spaces in this line?
GOVERNANCE.md
Outdated
* Advocates for the community in Maintainer and Core Maintainer meetings | ||
* Additional privileges: | ||
* Represent the project in public | ||
* Represent the project in interactions with the CNCF |
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.
ditto extra ident here.
|
||
Involuntary removal/demotion of a contributor happens when responsibilities and requirements aren't being met. This may include repeated patterns of inactivity, an extended period of inactivity, a period of failing to meet the requirements of your role, and/or a violation of the Code of Conduct. This process is important because it protects the community and its deliverables while also opening up opportunities for new contributors to step in. | ||
|
||
Involuntary removal or demotion of Maintainers and Reviewers is handled through a vote by a majority of the current Maintainers. Core Maintainers may be involuntarily removed by a majority vote of current Core Maintainers or, if all Core Maintainers have stepped down or are inactive according to the inactivity policy, by a supermajority (66%) vote of maintainers. |
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.
Who handles CoC complaints? I'd suggest that the people who do have the power to remove the offender from the project entirely without a maintainer vote. It might be something to work on for 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.
I think we should probably do complaints through a vote of the core maintainers, I don't know if I want to add a role with arbitrary remove-from-project permissions without a check
REVIEWING.md
Outdated
This document contains general principles for how to perform code reviews in the Podman repository. | ||
It does not aim to be a complete guide to how to perform code review, but rather to provide general guidance on how code reviews should be performed. | ||
|
||
This document is aimed at Reviewers, Maintainers, and Core Maintainers (see [GOVERNANCE.md](./GOVERNANCE.md) for definitions of these roles), but these guidelines should be followed by all who wish to review code in the Podman project's Github repositories, even those who are not currently a maintainer. |
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.
"Github" to "GitHub" in here too please.
The Podman project aims to ensure that all PRs are reviewed by at least 2 people prior to merge, at least one of which must be a repository Maintainer. | ||
There are some exceptions to this: Updates to libraries (including Go vendor updates) that pass CI cleanly and require no code changes may be merged by a maintainer without further review. | ||
|
||
All code merged must pass CI. |
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.
Do we want to also talk about requiring signed commits, or is that changing with CNCF?
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.
That's enforced by CI, no need for reviewers to pay particular attention
REVIEWING.md
Outdated
go 1.22.8 | ||
``` | ||
|
||
For all branches except the main branch, this should always remain static. |
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.
"should" is probably a good word here. We may end up bumping the go version in the go.mod files of some of the RHEL branches to resolve the recent crypto issue.
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.
IMO RHEL policy should have nothing to do with this. I think we are all in agreement to move the RHEL branches out of this repo so these must be upstream policies only.
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.
Still, I think I should weaken this to say "almost always". We occasionally do need to bump for CVEs and such.
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.
FWIW, fedora reviews have must and should . So, I'd say "should" is already kinda weak. But I'm cool either way. We could also account for special cases here: except where the situation demands, for e.g. CVE fixes
.
REVIEWING.md
Outdated
|
||
Please avoid bikeshedding during reviews. | ||
|
||
Trivial changes that do not affect the ultimate functionality of the PR - for example, small grammar or phrasing changes in documentation, unnecessary renaming of variables - should not block merge of a PR. |
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 agree with non-blocking PRs for internal code comments, but I disagree with anything that is customer-facing, whether it's output to be read by a customer or, more importantly, the man pages. Those errors in documentation should be blockable, IMO.
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.
Would be good to make spellcheck or other tools a part of CI (I think we already do it in some projects). While they need not be PR blockers, I'd say it reflects poorly if we have simple typos in documentation. While we're at it, maybe we should also stick with a variant of English unless chosen already. For example, Fedora enforces American English:
MUST: The spec file must be written in American English. See Packaging Guidelines: Summary and description
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.
Reworded
REVIEWING.md
Outdated
|
||
Please do not request that a contributor make significant changes to code their PR did not touch. | ||
If you are reviewing and find problems in pre-existing code in a file that the PR changed, you should not require that the contributor change this code as well. | ||
Asking if they are willing to do so is fine, but do not block merge of the PR if they are unwilling. |
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.
Asking if they are willing to do so is fine, but do not block merge of the PR if they are unwilling. | |
Asking if they are willing to do so is fine, but do not block the merge of the PR if they are unwilling. |
091a5fa
to
9ee6fb3
Compare
I'm going to drop the WIP, while we're still working on comments I think the core of the document is where we want it to be |
We will merge only with an LGTM from all 6 of the Core Maintainers in the MAINTAINERS doc in this PR. |
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.
LGTM
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.
LGTM
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.
As discussed I think a little intro would be good before we dive right in. And separators for the sections so you know if you are in a different subsection.
PRs into a non-main branch which change the supported Go version should be modified to not require such a change, or rejected if that is not possible. | ||
Changing supported Go version in the main branch is allowed, but not encouraged. | ||
|
||
### Tests |
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.
Documentation changes and certain changes don't require tests
for such cases No New test
label is used, should it reflect here ?
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.
One nit above but LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, giuseppe, Luap99, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is the initial version of the governance model we're looking to implement. It is still very early, and comments and suggestions are very welcome! Signed-off-by: Matt Heon <[email protected]>
Matt I tried two things one making the main headers a bit bigger and also adding some more separation lines. I am thinking that with them being bigger we maybe don't need the lines.
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.
LGTM — I think the discussions/reviews to date have covered a lot and resulted in a pretty polished document.
(Also, I generally think that over longer periods, there’s a reasonably high likelihood of something unexpected happening no matter what we do [e.g. who could have predicted the CNCF donation?], so I’m not that worried about making the governance rules bulletproof.)
Each of the project member roles below is organized into lists of three types of things. | ||
|
||
* "Responsibilities" – functions of a member | ||
* "Requirements" – qualifications of a member | ||
* "Privileges" – entitlements of member |
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.
Non-blocking, non-substantive:
Consider moving this to a comment. Reading this document, I emotionally feel that starting with a “terminology” section, the way some laws do, says “this was written by a bureaucracy for a bureaucracy, expect the processes to be painfully formalistic”.
I do agree that having some common structure to the ladder entries is useful, but maybe that structure can be understood well enough from context when reading the actual entries.
[I’m even wondering about not describing the ladder at all, but that’s really just a question, not even a weak preference.]
(This is exacerbated in that I’m also wondering what “responsibilities” are; starting with a terminology section I don’t immediately understand worries me. The “responsibilities” are not requirements / expectations, that’s a different section. But then again, the first example says “follow the CNCF CoC”, which is probably a hard requirement. And in the other ladder entries, they are closer to “privileges”. Is it that “requirements are how we quantify responsibilities”? Are “responsibilities” closer to ”typical kinds of contribution”? OTOH sometimes they imply specific access rights on the project.
Maybe the “follow CoC”, part which is already in the paragraph above, can be removed from the “contributor” ladder entry, and then s/Responsibilities/Typical activities/
?
Ultimately all of this matters very little.)
Some changes are OK to ask for - for example, asking a contributor to refactor existing code very similar to something being added to prevent code duplication. | ||
However, larger changes that would substantially increase the size of the PR should be avoided. | ||
Reviewers should use their best judgement to balance respect for the contributor's time and the code hygiene of the project. | ||
If a change is too large to be reasonably asked for, consider asking the contributor to add a comment with a "TODO" or "FIXME" to the area that needs changing (or making a PR yourself with such a comment). |
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’m leaning a bit more towards the “let the PR contributor do all the work” side, at least in the sense that letting a drive-by contributor add a badly-structured workaround or a kludgy minimal implementation of a feature, “succeed”, and disappear, even if accompanied by a FIXME documenting this, is not a good trade for the long-term maintainability of the project.
The only tools to manage maintainer overload we have is:
- finding new contributors, and
- backpressure: not encouraging/accepting contributions that increase maintainer overload.
And there’s a trade-off between being welcoming and encouraging to new long-term contributors, vs. discouraging drive-by tech debt additions, and it’s hard to tell in advance which one we are dealing with.
Ultimately, I don’t have any specific wording suggestions, no objections to “best judgement” — and, anyway, this PR already includes a process to resolve disagreements between reviewers/maintainers.
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.
E.g. researching a situation, documenting it, and adding a FIXME, without actually fixing it, can certainly be a valuable contribution alone.
Adding a hack that works 40% of the time, and a FIXME to turn that into a production feature, maybe not?
One example that I think happens somewhat regularly, incl. by regular contributors, is adding a Podman feature which doesn’t work podman-remote
: just not doing the HTTP work when it is clearly possible, or, in situations where it’s unclear whether it is possible or where it seems difficult, not arriving at a plan and merging the local-only feature anyway.
I think the past practice for this has been “use best judgement”, and that’s fine. Just something to point at / think about.
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.
My basic intent here was to discourage asking drive-by contributors to refactor massive sizable amounts of the program (that they likely have no direct knowledge of) to improve our tech-debt situation. That kind of thing is definitely valuable, but should be done by actual maintainers or willing volunteers; we shouldn't be asking someone trying to submit a trivial patch to do it.
On the other hand, the points you make are quite valid. We should have the ability to reject contributions on grounds they will introduce excessive complexity or tech debt, and asking for a refactor of that is quite fine.
I'll try and think of a better way to phrase this. One thing I might include is that maintainers should be held to a higher standard - I'm a lot more forgiving of asking someone deeply familiar with the project to refactor than I am a drive-by contributor, and I don't think that's controversial.
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.
discourage asking drive-by contributors to refactor massive amounts of the program
Agreed.
Overall I think we are on the same page, and I can’t think of any obvious wording improvement — so maybe it’s not worth spending a lot of effort trying to polish this to perfection. “There are tradeoffs, they are not 100% clear-cut, just be aware of the principal concerns.” is fine.
This is the initial version of the governance model we're looking to implement. It is still very early, and comments and suggestions are very welcome!
DO NOT MERGE THIS until we have a broad agreement that the current version is acceptable.
Does this PR introduce a user-facing change?