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

Repo reorg, cleaning up the spider webs #214

Merged
merged 18 commits into from
Oct 16, 2019
Merged

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Jul 15, 2019

This PR follows a conversation with @momack2 in which we identified that it is time to have a structure around how we create and maintain specs for the IPFS Protocol.

This PR contains:

@@ -0,0 +1,80 @@
# Spec Maintainer Protocol
Copy link
Member Author

Choose a reason for hiding this comment

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

@momack2 @alanshaw @Stebalien this is the main document to review

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Spec review.


Reviewers:
**Maintainer(s)**:
- [Juan Benet](github.com/jbenet)
Copy link
Member

Choose a reason for hiding this comment

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

Maintainers != Authors. Let's have two sections to make it clear who's actively maintaining a spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

I've mixed feelings with the value of having an Author. In the world of open collaboration and with everything being version controlled, Authorship gets tracked by the commit log. Unless you see a special responsibility that the original author would have (for ever, given that the original author never changes) that the maintainer could not take.

As a note, in the JS modules, we removed the author field in favor of the maintainer. It also serves to recognize the awesome contributor that is taking the responsibility.

Copy link
Member

@alanshaw alanshaw Jul 16, 2019

Choose a reason for hiding this comment

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

I had understood author to be the person who came up with the idea, i.e. it's more than just who wrote the document. So in that sense it would be nice to recognize that with an "Author" field, provided we agree on the definition.

+1 on maintainers != authors - we should either not auto-promote authors to maintainers or ensure we do a second PR where we re/un-assign maintainers.

Update: https://github.com/ipfs/specs/pull/214/files#r303776496

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a difference between code and a spec in this case. An example that comes to mind is IETF RFCs, which aren't maintained but updates that supersede old versions are created and it is the authors that are listed. I personally think knowing who authored the original document is important to state even if they are no longer maintaining it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Valuable points. I still have mixed feelings though, specially because there are "specs" that are no more than the placeholder or "specs" that are other docs glued together.

Having an author field also begs to ask: What happens when we have multiple Authors but uneven distribution of the work?

In the end, will this ever be used? Again, we have version control. We have line by line authorship anyway :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
REPO_FS.md Outdated Show resolved Hide resolved
REPO_FS.md Outdated Show resolved Hide resolved
# ![](https://img.shields.io/badge/status-wip-orange.svg?style=flat-square) Addressing on the Decentralized Web

**Maintainer(s)**:
- [Lars Gierth](mailto:lgierth@ipfs.io)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe include @lidel here since he has lead this work in browsers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't make any decisions on who maintains what for this PR, only focused or reorg & cleanup of the files + writing the maintainer protocol. Let's leave the election/volunteering of spec maintainers for an iteration after merging the protocol (so that expectations are clear from the start).

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

We desperately need a spec protocol but this needs to define a complete protocol that allows us to merge changes to specs with confidence.

My go-to example for specs is https://github.com/rust-lang/rfcs (see the readme) as this seems to be one of the fastest-moving spec processes I've seen.

My primary concern with this proposal is that it leaves everything up to a single maintainer.


> We have a protocol to maintain specs of protocols! How meta, but very useful.

## Motivation
Copy link
Member

Choose a reason for hiding this comment

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

There are some good ideas here but they have little to do with the spec maintainer protocol. The motivation behind the spec maintainer protocol is:

Specs are only useful if they're maintained. Without a maintainer, a spec will stagnate and implementations of that spec will eventually move on and diverge.

Or, maybe

Specs are only useful if there exists a protocol for iterating on them...


- Respect and follow the [IPFS Code of Conduct](https://github.com/ipfs/community/blob/master/code-of-conduct.md).
- Have a great understanding of the subsystem purpose and how it is used by other parts of the project.
- Review and merge PRs to the Spec
Copy link
Member

Choose a reason for hiding this comment

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

Specs are not code and changes are hard to revert. There should be a review process for every non-trivial change (even if everyone in the process just gives a 👍).

I'd expect a maintainer to merge cleanups/corrections, shepard discussions, and bring proposals to the attention of the IPFS Spec WG (or something like that).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go as far as to say you don't revert a spec. Once someone has built on top, that is it, that spec is a thing, there is no taking it back, there is only moving forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I now see @Stebalien that you say essentially the same thing in another comment


The Specification Status description can be found on the main [README of this repo](https://github.com/ipfs/specs#badges-and-spec-lifecycle).

## Disclaimer as 2019 Q3
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this section go in the README?

- **User Interface (aka Public APIs):**
- [ ] [Core API (aka using IPFS as a package/module)](./API_CORE.md)
- [x] [JavaScript implementation details](https://github.com/ipfs/interface-js-ipfs-core)
- [ ] [Golang implementation details](https://github.com/ipfs/interface-go-ipfs-core)
Copy link
Member

Choose a reason for hiding this comment

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

This is up-to-date and tested by go-ipfs's CI.

Copy link
Member

Choose a reason for hiding this comment

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

Note these should be updated to whatever we decide here https://github.com/ipfs/specs/pull/214/files#r303472977

- [x] [multihash](https://github.com/multiformats/multihash) - self-describing hash digest format.
- [x] [multiaddr](https://github.com/multiformats/multiaddr) - self-describing addressing format.
- [x] [multicodec](https://github.com/multiformats/multicodec) - self-describing protocol/encoding streams (note: a file is a stream).
- [x] [multistream](https://github.com/multiformats/multistream) - multistream is a format -- or simple protocol -- for disambiguating, and layering streams. It is extremely simple.
Copy link
Member

Choose a reason for hiding this comment

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

See multiformats/multistream#10. The history around multistream, multicodec, and multistream select are all really confusing and I'm not sure if we can reasonably say that the specs are "up to date".


The very aspirational future is to have a spec language in which we can describe how the program should function and it will automatically create the tests to verify it. The aspiration future ++ is having such spec language that writes the code itself and verifies it 🚀.

The Spec Maintainer Protocol takes a large amount of inspiration from the repo [Lead Maintainer Protocol](https://github.com/ipfs/team-mgmt/blob/master/LEAD_MAINTAINER_PROTOCOL.md).
Copy link
Member

Choose a reason for hiding this comment

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

We need to be careful about this (also noted below). Specs are, effectively, append-only logs. They're not code and shouldn't be treated like code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do not disagree. Any special point you want to highlight as a note of caution?

SPEC_MAINTAINER_PROTOCOL.md Outdated Show resolved Hide resolved
SPEC_MAINTAINER_PROTOCOL.md Outdated Show resolved Hide resolved
SPEC_MAINTAINER_PROTOCOL.md Outdated Show resolved Hide resolved
daviddias and others added 2 commits July 16, 2019 09:06
Co-Authored-By: Adrian Lanzafame <adrianlanzafame92@gmail.com>
Co-Authored-By: Adrian Lanzafame <adrianlanzafame92@gmail.com>
README.md Outdated Show resolved Hide resolved

Reviewers:
**Maintainer(s)**:
- [Juan Benet](github.com/jbenet)
Copy link
Member

@alanshaw alanshaw Jul 16, 2019

Choose a reason for hiding this comment

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

I had understood author to be the person who came up with the idea, i.e. it's more than just who wrote the document. So in that sense it would be nice to recognize that with an "Author" field, provided we agree on the definition.

+1 on maintainers != authors - we should either not auto-promote authors to maintainers or ensure we do a second PR where we re/un-assign maintainers.

Update: https://github.com/ipfs/specs/pull/214/files#r303776496

README.md Outdated Show resolved Hide resolved
SPEC_MAINTAINER_PROTOCOL.md Outdated Show resolved Hide resolved
SPEC_MAINTAINER_PROTOCOL.md Outdated Show resolved Hide resolved
SPEC_MAINTAINER_PROTOCOL.md Outdated Show resolved Hide resolved
SPEC_MAINTAINER_PROTOCOL.md Outdated Show resolved Hide resolved
SPEC_MAINTAINER_PROTOCOL.md Outdated Show resolved Hide resolved
- **User Interface (aka Public APIs):**
- [ ] [Core API (aka using IPFS as a package/module)](./API_CORE.md)
- [x] [JavaScript implementation details](https://github.com/ipfs/interface-js-ipfs-core)
- [ ] [Golang implementation details](https://github.com/ipfs/interface-go-ipfs-core)
Copy link
Member

Choose a reason for hiding this comment

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

Note these should be updated to whatever we decide here https://github.com/ipfs/specs/pull/214/files#r303472977

@daviddias
Copy link
Member Author

My go-to example for specs is https://github.com/rust-lang/rfcs (see the readme) as this seems to be one of the fastest-moving spec processes I've seen.

Definitely looking to take a ton of inspiration from very successful examples. At the same time, I also worry of taking complex processes wholesale (specially without having anyone that saw it evolve). The Rust org had the time to develop that RFC process and respective habits and practices overtime vs. overnight.

My primary concern with this proposal is that it leaves everything up to a single maintainer.

This as "the one in this PR" or "the one used by rust"??

- [x] Self Describing Formats ([multiformats](http://github.com/multiformats/multiformats)):
- [x] [multihash](https://github.com/multiformats/multihash) - self-describing hash digest format.
- [x] [multiaddr](https://github.com/multiformats/multiaddr) - self-describing addressing format.
- [x] [multicodec](https://github.com/multiformats/multicodec) - self-describing protocol/encoding streams (note: a file is a stream).
Copy link
Member

Choose a reason for hiding this comment

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

I also wouldn't say multicodec is up to date, see multiformats/multicodec#134

@Stebalien
Copy link
Member

This as "the one in this PR" or "the one used by rust"??

The one in this PR. I'm not suggesting we copy it wholesale but we should learn from the process. My favorite parts are:

  • Closing a spec is good and allows the author to revise it and submit a new one.
  • Postponing a spec is good and allows the community to revisit ideas later.
  • The template: https://github.com/rust-lang/rfcs/blob/master/0000-template.md. We'll likely have to adapt it for specs but it forces the author to lay out their motivation, demonstrate that they've done their research, bring up alternatives, etc.
  • Clear review cycle that forces a responsible working group to make a decision.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
REPO_FS.md Outdated Show resolved Hide resolved
REPO_FS.md Outdated Show resolved Hide resolved
daviddias and others added 3 commits July 29, 2019 11:24
Co-Authored-By: Alan Shaw <alan.shaw@protocol.ai>
Co-Authored-By: Adrian Lanzafame <adrianlanzafame92@gmail.com>
Co-Authored-By: Alan Shaw <alan.shaw@protocol.ai>
@daviddias
Copy link
Member Author

Following on @Stebalien's comments

The template: https://github.com/rust-lang/rfcs/blob/master/0000-template.md. We'll likely have to adapt it for specs but it forces the author to lay out their motivation, demonstrate that they've done their research, bring up alternatives, etc.

This is good, but also very RFC oriented (which is a great process once well established within a team culture). Have we taken a moment to consider how we see these discussions being made in the long term? Example, do we want to a) go more on the traditional RFC direction or b) take the approach of treating specs as documents that present the current state of reality and use Pull Requests as the way to have changes reviewed.

a)

  • Means that info and arguments behind certain decisions will leave with the document itself
  • Also means that the specs documents are not a technical specifications document, rather, a collection of changes that happen over time that once collapsed together will yield the actual spec
  • This is the traditional/conventional way in networks land

b)

  • Anything that is on master, is the de-facto technical specification
  • Discussions will leave on the PR that proposes the changes

Clear review cycle that forces a responsible working group to make a decision.

At the moment, we have only one Working Group that is responsible for IPFS Implementations, this suggests that it should be that Working Group to review and iterate. Does this match what you are thinking?

On review cycle. Do you expect a well established cadence? (e.g. only make changes at every epoch of time) or a more streamlined (as they come) way?

@Stebalien
Copy link
Member

So, libp2p is trying to fix the same issue and the solution they're considering is to just do both:

  1. Have a living document describing the state of the spec.
  2. Have RFCs for changing the spec. That way we have a concrete record of every change that doesn't rely on github comments and issues.

A new RFC would both patch the living spec(s) and drop a new text file into an RFCs directory.

At the moment, we have only one Working Group that is responsible for IPFS Implementations, this suggests that it should be that Working Group to review and iterate. Does this match what you are thinking?

At the moment, yes.

On review cycle. Do you expect a well established cadence? (e.g. only make changes at every epoch of time) or a more streamlined (as they come) way?

We've introduced a "design review nomination" section to the core implementations call for nominating on-demand design review calls. At the moment, we're running the design reviews without official RFCs but RFCs with clear motivations, designs, alternatives, etc. would make this easier.

@b5
Copy link
Contributor

b5 commented Aug 8, 2019

Qri adopted the rust RFC process wholesale almost a year back. repo here.

Major wins that came out of this:

  • people who aren't me, and even people who don't work at our project have actively contributed new ideas to how Qri works. I feel far less design burden, and others have greater ownership of our project.
  • When an RFC is approved we all have a high degree of clarity on what a feature will do, how it relates to features that already exist, and how to review code that implements the RFC.
  • As a project we all have a natural instinct for when something needs an RFC. It's common parlance at Qri to say "we'll need an RFC for this". New hires have been able to self-onboard to this process by reading the repo and reviewing previous PRs to get a feel for the process.
  • The guide-level explanation is documentation. We copy-paste accepted RFC explainers into tutorials & reference docs all the time, making feature releases easier.

Downsides:

  • Adopting this process requires total buy-in, and is very painful at first. Everyone has to be on board with writing RFCs, which works better for some than others.
  • It requires continued vigilance. every 4-6 months I need to remind folks not to let design choices sneak into PRs. We sometimes do have to write "retroactive" RFCs, which isn't great.

@daviddias
Copy link
Member Author

daviddias commented Sep 11, 2019

Thank you for jumping in @b5 and sharing your experience! Thanks to everyone that has reviewed and commented on this so far. I might need a bit more time to come up with a revamped proposal taking into account all the examples linked.

To avoid having the PR that cleans the Spider Webs 🕸🕸 get full of Spider Webs 🕸🕸🕸🕸🕸, I'm going to move the Spec Maintainer Protocol / RFC process to a consequent PR, while keeping the reorg from this one.

And yes, I take on rebasing all the outstanding PRs into this new structure :)

@daviddias daviddias changed the title Spec Maintainer Protocol Repo reorg, cleaning up the spider webs Sep 11, 2019
@daviddias daviddias requested review from alanshaw and removed request for momack2 September 20, 2019 12:58
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Thanks!

@Stebalien
Copy link
Member

🙇‍♂️

@Stebalien Stebalien merged commit 51577eb into master Oct 16, 2019
@Stebalien Stebalien deleted the specs-maintainer-protocol branch October 16, 2019 08:31
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.

6 participants