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

Module Lead Maintainers Protocol #301

Merged
merged 10 commits into from
May 1, 2018
Merged

Module Lead Maintainers Protocol #301

merged 10 commits into from
May 1, 2018

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Apr 25, 2018

This PR follows ipfs/team-mgmt#600

Hi y'all, here is an update to our contributing guidelines that introduces a new section based on ipfs/team-mgmt#600 which I would love to have your eyeballs to go over it.

This will require the creation of many PRs to all the js-* repos and if no one is opposed, I would like to get it started :)

Moved tracking table to ipfs/team-mgmt#600 (comment)

@daviddias daviddias self-assigned this Apr 25, 2018
@ghost ghost added the status/in-progress In progress label Apr 25, 2018

Always run tests before pushing and PR'ing your code.
- A Tech Lead directs the development of an entire ecosystem of modules (i.e js-ipfs, js-libp2p, js-libp2p and js-multiformats), it has a complete understanding of the stack, the IPFS project, its goals and participates actively in the ROADMAP planning.

Choose a reason for hiding this comment

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

There are 2 js-libp2p references, perhaps one should be js-ipld?

### Linting & Code Style
- Be proactive in increasing the quality of the module. This goes from improving documentation, tests, codebase and more.
- Show a great level of rigor and polish in the code that they ship.
- Help others in understanding how the module and why it exists.

Choose a reason for hiding this comment

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

edit: how the module and why it exists to how the module works and why it exists

- Respect and follow the [IPFS Code of Conduct](https:/ipfs/community/blob/master/code-of-conduct.md).
- Have a complete understanding of the module purpose, its specification (if any) and how the module is used by other parts of the project.
- Review and Release PRs
- Publish new versions of the module to npm

Choose a reason for hiding this comment

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

What is the expectation for curating issues? That's probably something we'd want the Lead Maintainers responsible for to make sure we are managing that list to avoid duplicate or stale issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a note for this one. Thanks for bringing it up


- Update each package.json and README.md to have a `maintainer` field.
- Update packages table on each entry module (e.g https:/ipfs/js-ipfs#packages) to also list the maintainer for each.
- Protect the master branch and only grant permissions for merge to the Maintainer and the Tech Lead, only.

Choose a reason for hiding this comment

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

This sounds like merge and publish permissions will be limited to 2 people. Has there been any thought about expanding that for larger modules? I see the value around module integrity and quality with a smaller set of responsible individuals, but there is also the risk of only 2 people becoming blockers if they are unavailable during the same period of time (conferences, retreats, vacations, 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.

Fair point, that said, do note that this is 100% for almost every module. I expect that there there will be a few or almost no situation where there is a need for a urgent release without the Tech Lead and the Lead Maintainer being around.

In the case that one or both of the Tech Lead and Lead Maintainer need to be absent for a long period of time, both of these should notify the rest of the group and a plan for releasing during that time should be prepared. My take is that if someone takes a week or two offline during a quiet period, it is almost certainly ok to wait 2 weeks to release, the only exception would be a security fix but those need to be handled with care and in with the the Security team.

Also, remember:

![Barbossa's warning](assets/CodeIsMoreLikeGuidelines.jpg)
Our toolkit for each of these is not set in stone, and we don't plan to halt our constant search for better tools. Get in touch if you've got ideas. [These are guidelines and rather then rules](assets/CodeIsMoreLikeGuidelines.jpg).
Copy link
Member

Choose a reason for hiding this comment

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

wordo: These are guidelines and rather then rules

Copy link
Member

Choose a reason for hiding this comment

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

These are guidelines rather then than rules

Copy link
Member Author

Choose a reason for hiding this comment

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

Inception, a wordo inside a wordo! Thanks both! 😀


With this structure, we expect to achieve the following goals:

- Recognize extraordinary contributions and empower contributors to take even more important role in the project
Copy link
Member

Choose a reason for hiding this comment

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

wordo: empower contributors to take an even more important role

- Recognize extraordinary contributions and empower contributors to take even more important role in the project
- Reduce PR review time and Time To Release
- Increase the overall quality of the project
- Help users know who to reach out for help
Copy link
Member

Choose a reason for hiding this comment

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

wordo: Help users know who to reach out to for help


Always run tests before pushing and PR'ing your code.
- A Tech Lead directs the development of an entire ecosystem of modules (i.e js-ipfs, js-libp2p, js-libp2p and js-multiformats), it has a complete understanding of the stack, the IPFS project, its goals and participates actively in the ROADMAP planning.
- A Lead Maintainer is a contributor that has showned extraordinary ability to contribute to the project and willingness to make the project better by taking the responsibility of stewarding one of its modules forward.
Copy link
Member

Choose a reason for hiding this comment

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

showned -> shown

- [...for maintainers](#for-maintainers)
- [Setting up `aegir`](#setting-up-aegir)
- [Directory Structure](#directory-structure)
- [Default `require`](#default-require)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this line links to anything.


For each JavaScript repository in the IPFS, libp2p, IPLD, or related GitHub orgs, there should be a single Captain specified in the Maintainers section of the README for that repository. The Captain is in charge of merging PRs, keeping issues up to date, and overall quality of a repository.
- [David Dias](http:/diasdavid/) for js-ipfs, js-libp2p js-multiformats ecosystems.
- [Volker Mische](github.com/vmx) for the js-ipld ecosystem.
Copy link
Member

Choose a reason for hiding this comment

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

Missing the https:// in the link


Sometimes, a Captain may elect to have other maintainers that also have merge ability and commit access to the main repo. These maintainers can help out, but defer to the Captain as the person in charge of maintaining quality across the repo. It is important that this distinction is explicit; if there are long-standing PRs or issues, it is ultimately up to the Captain to gather information about the issue or PR. A Captain only makes a decision if he _needs_ to and _all_ methods of discussion are exhausted. Our community strives to trust the Captain as someone who ultimately has the most knowledge of a repo (even if they are also opinionated, and even if they have to spend effort to source that knowledge). This may change in the future, if we go with more non-hierarchical model.
The current Lead Maintainers can be identified either by the `maintainer` field in the package.json of the module and/or the section `Maintainer` in the README.md of the module.
Copy link
Member

Choose a reason for hiding this comment

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

Also watch for typos as maintainers field exists but is overwritten/handled by npm.

Ref: The "maintainers" field in the top-level of the registry metadata (ie, not versioned) is the npm-controlled list of the npm usernames of the people with permission to write to that package. This can be modified via the npm owners command. (Confusing, I know.)
npm/www#133 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for catching this. Changed to lead-maintainer instead :)

Copy link
Member

@travisperson travisperson left a comment

Choose a reason for hiding this comment

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

This looks awesome @diasdavid!

Copy link

@pgte pgte left a comment

Choose a reason for hiding this comment

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

Looks good!

@daviddias
Copy link
Member Author

Thanks everyone for the review and positive feedback! I'm super excited for this change \o/

I've issued the first PR to multiformats/js-multihash#51, please double check the permissions I set for Github.

@daviddias
Copy link
Member Author

On @vmx's point #301 (review)

I'm sad that it's not about captains anymore ;)

We are still on time to make to change to Captain instead of Lead Maintainer. Add a 👍 for yes or 👎 for no on this comment.

@daviddias
Copy link
Member Author

Github perms template from multiformats/js-multihash#51 (comment)

image

@victorb
Copy link
Member

victorb commented May 1, 2018

I've added the branch protection from js-multihash to all the js-* projects we have now (except one that doesn't have a master branch). You can see a snapshot of the status here: https://ipfs.io/ipfs/QmcAbqYyknVVJbKWihXU1TGNJQ8d3gXuNWpw5vaqizU3cC/status.html

@daviddias
Copy link
Member Author

I was 99% convinced with the rename to Captain, but then after applying it to a couple of modules I realized it actually has a very different connotation. We have been using Captain and Tech Lead interchangeably in the past and now using Captain for Lead Maintainer would confuse folks.

@daviddias
Copy link
Member Author

@vmx can you handle adding the Lead Maintainers to the IPLD repos?

@daviddias daviddias changed the title wip: Module Lead Maintainers Protocol Module Lead Maintainers Protocol May 1, 2018
@daviddias
Copy link
Member Author

I declare that the document is now complete. Thank you all for the feedback, testing things and setting up tooling to make the migration smoother.

There is still a lot of PRs to do. I'll try to run through these as fast as possible.

@daviddias
Copy link
Member Author

Moved tracking table to ipfs/team-mgmt#600 (comment), merging this PR to get master with the latest guidelines! :)

@daviddias daviddias merged commit 9825cba into master May 1, 2018
@ghost ghost removed the status/in-progress In progress label May 1, 2018
@daviddias daviddias deleted the module-lead-maintainers branch May 1, 2018 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.