Skip to content
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 project governance! #679

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

dave-tucker
Copy link
Member

Adds documentation to explain:

  • Maintainer roles and responsibilities
  • Maintainer election/removal process
  • Pull Request Workflow (codified in the repo and enforced by Mergify)
  • Security Response Handling

Code changes:

  • Consolidates CI into a single pipeline - this gives us better execution control + allows us to block on only one CI status passing (build-workflow-complete).
  • Add cargo public-api for Aya, to ensure Alessandro reviews Aya API changes

TODO before merge:

  • Add Mergify to the repo
  • Add DCO bot to the repo

TODO after merge:

  • Enable the GitHub Security Reporting feature

@netlify
Copy link

netlify bot commented Jul 20, 2023

Deploy Preview for aya-rs-docs ready!

Name Link
🔨 Latest commit bf4722d
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64c7bcef965aa100086a25cd
😎 Deploy Preview https://deploy-preview-679--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

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
Copy link
Member Author

@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.

@dave-tucker dave-tucker force-pushed the governance branch 2 times, most recently from 5961b66 to c7ed48f Compare July 20, 2023 10:41
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 <dave@dtucker.co.uk>

Thanks for your help improving the project!
* [New Contributor Guide](#contributing-guide)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a native English speaker, but is using capital letters everywhere necessary? I'd rather go with "Contributing guide", "Ways to contribute", "Find an issue" etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's Title Case

Comment on lines +44 to +48
We have good first issues for new contributors and help wanted issues suitable
for any contributor. [good first issue](https://github.com/aya-rs/aya/labels/good%20first%20issue) has extra information to
help you make your first contribution. [help wanted](https://github.com/aya-rs/aya/labels/help%20wanted) are issues
suitable for someone who isn't a core maintainer and is good to move onto after
your first pull request.
Copy link
Member

Choose a reason for hiding this comment

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

And here some sentences start without capital letters. 😛 Anyway, I would still rephrase it entirely.

Suggested change
We have good first issues for new contributors and help wanted issues suitable
for any contributor. [good first issue](https://github.com/aya-rs/aya/labels/good%20first%20issue) has extra information to
help you make your first contribution. [help wanted](https://github.com/aya-rs/aya/labels/help%20wanted) are issues
suitable for someone who isn't a core maintainer and is good to move onto after
your first pull request.
Issues labelled as ["good first issue"](https://github.com/aya-rs/aya/labels/good%20first%20issue)
are suitable for new contributors. They provide extra information to help you make your first
contribution.
Issues labelled as ["help wanted"](https://github.com/aya-rs/aya/labels/help%20wanted) are
usually more difficult. We recommend them for people who aren't core maintainers, but either
made some contributions already or feel comfortable with starting from more demanding tasks.


The best way to reach us with a question when contributing is to ask on:

* The original github issue
Copy link
Member

Choose a reason for hiding this comment

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

GitHub

The best way to reach us with a question when contributing is to ask on:

* The original github issue
* Our Discord
Copy link
Member

Choose a reason for hiding this comment

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

I'd include an invite link here (and everywhere we mention Discord).


This is my commit message

Signed-off-by: Your Name <your.name@example.com>
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to require signoffs? @alessandrod

I don't mind it (I did it already in some of my PRs without even thinking about it), but we have a lot of not signed off commits. I'm also not a lawyer, but I don't think is really necessary for our licensing stuff?

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

- name: automatic merge conditions for main
conditions:
- "#approved-reviews-by>=2"
Copy link
Member

Choose a reason for hiding this comment

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

do we really need two reviewers?

Copy link
Member

Choose a reason for hiding this comment

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

Normally I'd say no....but I feel like with there being 10 approvers it won't have nearly as much effect on velocity and will ultimately help stability.

- base=main
- label!=hold
- label!=work-in-progress
- check-success=DCO
Copy link
Member

Choose a reason for hiding this comment

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

this makes it require the "signed off by" line? what does that do for us?

i think requiring signed commits is a much stronger stance (https://docs.github.com/en/enterprise-server@3.7/authentication/managing-commit-signature-verification/about-commit-signature-verification) but also considerably more onerous.

I think we should just drop this requirement.

Copy link
Member

Choose a reason for hiding this comment

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

I feel signing your commits (just the sign-off line not commit verification) is just the Open source community standard tbh, I'm not against having it.

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

- name: automatic merge conditions for main
Copy link
Member

Choose a reason for hiding this comment

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

do we really want automatic merges? I'd really rather let a human being decide on the merge decision.

Perhaps we can allow folks to merge their own changes after they've been approved (pushing a new revision should require re-approval).


## Supported Versions

No released versions of aya or it's subprojects will receive regular security updates until a mainline release has been performed.
Copy link
Member

Choose a reason for hiding this comment

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

s/it's/its/

## Supported Versions

No released versions of aya or it's subprojects will receive regular security updates until a mainline release has been performed.
A reported and fixed vulnerability will be included in the next minor release, which depending on the severity of the vulnerability may be immediate.
Copy link
Member

Choose a reason for hiding this comment

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

the line length here and in the other docs is inconsistent. can we pick one and use it everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

++++

@@ -0,0 +1,16 @@
# Maintainers
Copy link
Member

Choose a reason for hiding this comment

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

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)

To become a Maintainer you need to demonstrate the following:

- commitment to the project:
- participate in discussions, contributions, code and documentation reviews, for 6 months or more,
Copy link
Member

Choose a reason for hiding this comment

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

these requirements seem overly rigid to me. we should keep this list as a reference, but i don't think it is helpful to frame it as strictly required.

Maintainers may resign at any time if they feel that they will not be able to
continue fulfilling their project duties.

Maintainers may also be removed after being inactive, failing to fulfill their
Copy link
Member

Choose a reason for hiding this comment

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

does this happen without a vote? who is the executor?

The Maintainers will appoint a Security Response Team to handle security reports.
This committee may simply consist of the Maintainer Council themselves. If this
responsibility is delegated, the Maintainers will appoint a team of at least two
contributors to handle it. The Maintainers will review who is assigned to this
Copy link
Member

Choose a reason for hiding this comment

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

nit: double space after this period should be single

edit: there are a few others, please check them all

@@ -86,3 +155,22 @@ nicely even when it is indented.
Fixes: #1337
Refs: #453, #154
```

## Pull Request Checklist
Copy link
Member

Choose a reason for hiding this comment

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

can we move some or all of this into the github pull request template?

Copy link
Member

@astoycos astoycos left a comment

Choose a reason for hiding this comment

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

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"

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

- name: automatic merge conditions for main
conditions:
- "#approved-reviews-by>=2"
Copy link
Member

Choose a reason for hiding this comment

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

Normally I'd say no....but I feel like with there being 10 approvers it won't have nearly as much effect on velocity and will ultimately help stability.

- base=main
- label!=hold
- label!=work-in-progress
- check-success=DCO
Copy link
Member

Choose a reason for hiding this comment

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

I feel signing your commits (just the sign-off line not commit verification) is just the Open source community standard tbh, I'm not against having it.


## Documentation
If anything doesn't make sense, or doesn't work when you run it, please open a
bug report and let us know!
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add some issue templates along with this

i.e bug-report, enhancement-request, etc

before you submit your code:

* That Rust code has been formatted with `cargo +nightly fmt` and that all clippy lints have been fixed - you can find failing lints with `cargo +nightly clippy`
* That Go code has been formatted and linted
Copy link
Member

Choose a reason for hiding this comment

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

nit: not applicable here :)

* Answering questions on Discord
* Web design
* Communications / Social Media / Blog Posts
* Release management
Copy link
Member

Choose a reason for hiding this comment

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

Community-management?

Our process is currently as follows:

1. When you open a PR a maintainer will automatically be assigned for review
1. Make sure that your PR is passing CI - if you need help with failing checks please feel free to ask!
Copy link
Member

Choose a reason for hiding this comment

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

NIT:

Suggested change
1. Make sure that your PR is passing CI - if you need help with failing checks please feel free to ask!
2. Make sure that your PR is passing CI - if you need help with failing checks please feel free to ask!

And so-on

| [Mary](https://github.com/marysaka) | ? | Compatibility with older kernels |
| [](https://github.com/ajwerner) | ? | ? |
| [Tamir Duberstein](https://github.com/tamird) | ? | ? |
| [Andrew Stoycos](https://github.com/astoycos) | Red Hat | ? |
Copy link
Member

Choose a reason for hiding this comment

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

Lol Idk maybe

Suggested change
| [Andrew Stoycos](https://github.com/astoycos) | Red Hat | ? |
| [Andrew Stoycos](https://github.com/astoycos) | Red Hat | Map API |

🤷

## Supported Versions

No released versions of aya or it's subprojects will receive regular security updates until a mainline release has been performed.
A reported and fixed vulnerability will be included in the next minor release, which depending on the severity of the vulnerability may be immediate.
Copy link
Member

Choose a reason for hiding this comment

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

++++

status. Emeritus Maintainers will still be consulted on some project matters
and can be rapidly returned to Maintainer status if their availability changes.

## Meetings
Copy link
Member

Choose a reason for hiding this comment

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

I know no one is a fan but I feel like even a monthly or bi-monthly community meeting would be nice, I know it's hard to get together but as Aya grows more and more folks will get involved.

Copy link

mergify bot commented Nov 16, 2023

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

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

mergify bot commented Feb 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants