Add project governance! #679

dave-tucker · 2023-07-20T10:02:30Z

Adds documentation to explain:

Maintainer roles and responsibilities
Maintainer election/removal process
Pull Request Workflow
Security Response Handling

This change is

netlify · 2023-07-20T10:02:59Z

✅ Deploy Preview for aya-rs-docs ready!

Name	Link
🔨 Latest commit	`082a5f1`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/67c9e0af74eca50008726af4
😎 Deploy Preview	https://deploy-preview-679--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dave-tucker · 2023-07-20T10:23:39Z

@aya-rs/aya-maintainers This PR is aimed to solve some open issues around process. I borrowed a lot of this from bpfd 😏 which in turn was borrowed from some CNCF project or template or something.

MAINTAINERS.md is where we are all listed - please ensure your areas of responsibility, names and affiliations (if required) are accurate.

GOVERNANCE.md is where all the process around maintainers lives.
TL;DR

Be a good open source citizen
New maintainers are added via PR and requires a majority vote (either LGTM on PR or via Discord)
Maintainers are removed via PR and requires a 2/3 majority vote

The other important part is in CONTRIBUTING.md which outlines the Pull Request Workflow.
TL;DR:

At least 2 approving reviews are required for any PR.
Any PR that touches aya's public API needs explicit approval from @alessandrod in addition to one other maintainer review.

Since this is techincally a governance change, this PR will not merge until we have a 2/3 majority (that 6 of the 8 listed maintainers).

Feel free to discuss here or in Discord.

CONTRIBUTING.md

.mergify.yml

SECURITY.md

tamird · 2023-07-31T15:01:16Z

MAINTAINERS.md

@@ -0,0 +1,16 @@
+# Maintainers


do we need this file? https://github.com/orgs/aya-rs/people is the source of truth (though we should make everyone's membership public)

Yes. There's no transparency around changes to GitHub teams.
Having changes happen via MAINTAINERS.md forces us to codify our decisions and leaves an audit trail.
Also, I'm not sure it's possible to force org membership to be public - same with team membership.

Consider writing that down as well please.

GOVERNANCE.md

CONTRIBUTING.md

astoycos

Thanks for doing this, just a couple nits/comments!

The one thing I don't see mentioned is the fact of joining the org WITHOUT becoming a maintainer. For contributors who just want to be able to manually assign issues and stuff it's nice to be made an org member I think. Something along the lines of what K8s calls a "member"

.mergify.yml

CONTRIBUTING.md

MAINTAINERS.md

SECURITY.md

GOVERNANCE.md

mergify · 2023-11-16T21:28:43Z

@dave-tucker, this pull request is now in conflict and requires a rebase.

mergify · 2024-02-06T12:37:00Z

@dave-tucker, this pull request is now in conflict and requires a rebase.

mergify · 2025-01-01T16:41:35Z

@dave-tucker, this pull request is now in conflict and requires a rebase.

dave-tucker · 2025-03-06T11:51:34Z

This PR has now been updated based on previous comments.
TL;DR:

Mardownlint'ed everything
Mergify rules for automated merge has been removed
Number of approving reviews required has been changed to "at least 1"
Requirements for new maintainers has been written as a "guideline" and not a strict requirement
Process of adding/removing maintainers has been clarified
Pull request template added

Things to do in a follow up:

Document the aya-rs/owners team role and what access that provides, who has access and how to gain access
Document how to join the Aya org to gain issue read/triager permissions
Issue templates?

I'll open issues for these soon.

tamird

Reviewed 6 of 7 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @astoycos, @dave-tucker, and @vadorovsky)

.mergify.yml line 7 at r2 (raw file):

    actions:
      comment:
        message: "@dependabot merge"

btw this doesn't work since mergify doesn't have a commit bit. not for this PR but we should nuke it

Code quote:

pull_request_rules:
  - name: automatic merge for Dependabot pull request that pass CI
    conditions:
      - author=dependabot[bot]
    actions:
      comment:
        message: "@dependabot merge"

.github/PULL_REQUEST_TEMPLATE.md line 40 at r3 (raw file):

- [ ] Rust code has been formatted with `cargo +nightly fmt`
- [ ] All clippy lints have been fixed.
      You can find failing lints with `cargo +nightly clippy`

technically not true, there's a ./clippy.sh at the root for this reason

i don't think we need to include things CI will do for us in this list

Code quote:

- [ ] All clippy lints have been fixed.
      You can find failing lints with `cargo +nightly clippy`

.github/PULL_REQUEST_TEMPLATE.md line 7 at r2 (raw file):

    Before submitting a Pull Request, please ensure you've done the following:
     - 📖 Read the Forem Contributing Guide: https://github.com/aya-rs/aya/blob/main/CONTRIBUTING.md

Forem was the inspo, I take it? I think we can drop the proper noun for this and the next line

.github/PULL_REQUEST_TEMPLATE.md line 11 at r2 (raw file):

     - 👷‍♀️ Create small PRs. In most cases this will be possible.
     - ✅ Provide tests for your changes.
     - 📝 Use descriptive commit messages.

it might be good to be more specific e.g. by linking to https://cbea.ms/git-commit/

.github/PULL_REQUEST_TEMPLATE.md line 16 at r2 (raw file):

-->

# Description

i don't love this. the detailed information should be in the commit messages rather than the github-only PR text

tamird · 2025-03-06T14:31:06Z

MAINTAINERS.md

@@ -0,0 +1,16 @@
+# Maintainers


Consider writing that down as well please.

dave-tucker

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @alessandrod, @astoycos, @tamird, and @vadorovsky)

.github/PULL_REQUEST_TEMPLATE.md line 7 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Forem was the inspo, I take it? I think we can drop the proper noun for this and the next line

gah i've been found out! Yes it was. Fixed thanks.

.github/PULL_REQUEST_TEMPLATE.md line 11 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

it might be good to be more specific e.g. by linking to https://cbea.ms/git-commit/

Good suggestion. Done.

.github/PULL_REQUEST_TEMPLATE.md line 16 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i don't love this. the detailed information should be in the commit messages rather than the github-only PR text

Changed to "Summary"

.github/PULL_REQUEST_TEMPLATE.md line 40 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

technically not true, there's a ./clippy.sh at the root for this reason

i don't think we need to include things CI will do for us in this list

I know this feels repetitive of what CI is testing, however I have 2 counter points:

I do find it irritating when we receive PRs that fail the basic formatting and lint checks. These are fairly trivial to resolve pre-PR and we don't need to spend our CI minutes to solve this. I'm guilty of this on occasion too.
New contributor experience. It's pretty demotivating to open a PR here, have to wait for one of us to "Approve" the PR to run on CI, and then see everything turn red immediately. The loop to fix this requires us to hit "Approve" each time IIRC. I feel it's better to be able to set expectations ahead of time (before PR is opened) for contributors to follow.

CONTRIBUTING.md

GOVERNANCE.md

dave-tucker · 2025-03-06T17:28:51Z

MAINTAINERS.md

@@ -0,0 +1,16 @@
+# Maintainers


MAINTAINERS.md

tamird

Reviewed 2 of 2 files at r4, all commit messages.
Dismissed @astoycos and @vadorovsky from 5 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @astoycos, @dave-tucker, and @vadorovsky)

.github/PULL_REQUEST_TEMPLATE.md line 40 at r3 (raw file):

Previously, dave-tucker (Dave Tucker) wrote…

I know this feels repetitive of what CI is testing, however I have 2 counter points:

I do find it irritating when we receive PRs that fail the basic formatting and lint checks. These are fairly trivial to resolve pre-PR and we don't need to spend our CI minutes to solve this. I'm guilty of this on occasion too.

New contributor experience. It's pretty demotivating to open a PR here, have to wait for one of us to "Approve" the PR to run on CI, and then see everything turn red immediately. The loop to fix this requires us to hit "Approve" each time IIRC. I feel it's better to be able to set expectations ahead of time (before PR is opened) for contributors to follow.

I've removed the need to hit approve FWIW. CI now runs for all PRs regardless of author.

If you really want to improve the new contributor experience you might also include the API blessing incantation.

.github/PULL_REQUEST_TEMPLATE.md line 20 at r4 (raw file):

      Summarize the changes you're making here.
      Detailed information belongs in the Git Commit messages.
      Feel free to flag anything you thing needs a reviewers attention.

missing apostrophe

.github/PULL_REQUEST_TEMPLATE.md line 46 at r4 (raw file):

      You can find failing lints with `./clippy.sh`
- [ ] Unit tests are passing locally with `cargo test`
- [ ] Integration tests are passing locally

doesn't say how to do it, unlike the other bullets

MAINTAINERS.md

Explains how Maintainers are selected and their responsibilities. Explains the Pull Request review workflow. Adds config for Mergify to enforce this workflow. Signed-off-by: Dave Tucker <[email protected]>

dave-tucker

Reviewable status: 6 of 7 files reviewed, 7 unresolved discussions (waiting on @astoycos, @tamird, and @vadorovsky)

.github/PULL_REQUEST_TEMPLATE.md line 40 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I've removed the need to hit approve FWIW. CI now runs for all PRs regardless of author.

If you really want to improve the new contributor experience you might also include the API blessing incantation.

Oh wonderful thank you! And yes, I just tripped up on public-api and having a reminder in the PR template would have indeed been helpful. Done.

.github/PULL_REQUEST_TEMPLATE.md line 20 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

missing apostrophe

Done

.github/PULL_REQUEST_TEMPLATE.md line 46 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

doesn't say how to do it, unlike the other bullets

Done

CONTRIBUTING.md

MAINTAINERS.md

tamird

Reviewed 2 of 2 files at r5, all commit messages.
Dismissed @astoycos and @vadorovsky from 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @astoycos and @vadorovsky)

MAINTAINERS.md

dave-tucker force-pushed the governance branch from e7d2d2e to cb3e92f Compare July 20, 2023 10:04

dave-tucker requested a review from a team July 20, 2023 10:10

dave-tucker force-pushed the governance branch from cb3e92f to d9f9556 Compare July 20, 2023 10:15

dave-tucker force-pushed the governance branch 2 times, most recently from 5961b66 to c7ed48f Compare July 20, 2023 10:41

dave-tucker force-pushed the governance branch from c7ed48f to bf4722d Compare July 31, 2023 13:53

vadorovsky requested changes Jul 31, 2023

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

tamird reviewed Jul 31, 2023

View reviewed changes

astoycos reviewed Aug 1, 2023

View reviewed changes

mergify bot added the needs-rebase label Nov 16, 2023

mergify bot removed the needs-rebase label Jan 1, 2025

mergify bot added the needs-rebase label Jan 1, 2025

dave-tucker force-pushed the governance branch from bf4722d to c8e408b Compare March 6, 2025 11:36

mergify bot added test A PR that improves test cases or CI and removed needs-rebase labels Mar 6, 2025

dave-tucker force-pushed the governance branch from c8e408b to f08680e Compare March 6, 2025 11:44

dave-tucker marked this pull request as ready for review March 6, 2025 11:51

tamird requested review from astoycos and vadorovsky March 6, 2025 14:31

tamird reviewed Mar 6, 2025

View reviewed changes

MAINTAINERS.md

@@ -0,0 +1,16 @@

# Maintainers

Copy link

Member

tamird Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider writing that down as well please.

dave-tucker force-pushed the governance branch from f08680e to 7127907 Compare March 6, 2025 17:28

dave-tucker commented Mar 6, 2025

View reviewed changes

tamird reviewed Mar 6, 2025

View reviewed changes

MAINTAINERS.md Outdated Show resolved Hide resolved

dave-tucker force-pushed the governance branch from 7127907 to edcca02 Compare March 6, 2025 17:48

Add project governance documentation

082a5f1

Explains how Maintainers are selected and their responsibilities. Explains the Pull Request review workflow. Adds config for Mergify to enforce this workflow. Signed-off-by: Dave Tucker <[email protected]>

dave-tucker force-pushed the governance branch from edcca02 to 082a5f1 Compare March 6, 2025 17:51

dave-tucker commented Mar 6, 2025

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

MAINTAINERS.md Outdated Show resolved Hide resolved

tamird approved these changes Mar 6, 2025

View reviewed changes

MAINTAINERS.md Outdated Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add project governance! #679

Add project governance! #679

dave-tucker commented Jul 20, 2023 •

edited

Loading

netlify bot commented Jul 20, 2023 •

edited

Loading

dave-tucker commented Jul 20, 2023

tamird Jul 31, 2023

dave-tucker Mar 6, 2025

tamird Mar 6, 2025

dave-tucker Mar 6, 2025

astoycos left a comment

mergify bot commented Nov 16, 2023

mergify bot commented Feb 6, 2024

mergify bot commented Jan 1, 2025

dave-tucker commented Mar 6, 2025

tamird left a comment

tamird Mar 6, 2025

dave-tucker left a comment

dave-tucker Mar 6, 2025

tamird left a comment

dave-tucker left a comment

tamird left a comment

Add project governance! #679

Are you sure you want to change the base?

Add project governance! #679

Conversation

dave-tucker commented Jul 20, 2023 • edited Loading

netlify bot commented Jul 20, 2023 • edited Loading

✅ Deploy Preview for aya-rs-docs ready!

dave-tucker commented Jul 20, 2023

tamird Jul 31, 2023

Choose a reason for hiding this comment

dave-tucker Mar 6, 2025

Choose a reason for hiding this comment

tamird Mar 6, 2025

Choose a reason for hiding this comment

dave-tucker Mar 6, 2025

Choose a reason for hiding this comment

astoycos left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 16, 2023

mergify bot commented Feb 6, 2024

mergify bot commented Jan 1, 2025

dave-tucker commented Mar 6, 2025

tamird left a comment

Choose a reason for hiding this comment

tamird Mar 6, 2025

Choose a reason for hiding this comment

dave-tucker left a comment

Choose a reason for hiding this comment

dave-tucker Mar 6, 2025

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

dave-tucker left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

dave-tucker commented Jul 20, 2023 •

edited

Loading

netlify bot commented Jul 20, 2023 •

edited

Loading