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

docs: add request for maintainership md file #2453

Merged
merged 10 commits into from
Oct 25, 2022

Conversation

charleenfei
Copy link
Contributor

@charleenfei charleenfei commented Sep 30, 2022

Description

As the Cosmos ecosystem has expanded and flourished, it has become increasingly apparent that along with ecosystem growth, there has been the growth of great development teams working on IBC applications and features. We recognise that having these applications in the ibc-go repo can be valuable for ease of discoverability, "officialness", and of course assurance of seamless performance as updates to the repo continue to be pushed by our team and collaborators.

We've spent a lot of time working on processes to handle inbound requests for maintaining important IBC applications which may be valuable for the entire Interchain ecosystem but developed outside of our engineering team. This document seeks to articulate this process (as well as our our own internal process of developing any new feature).


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Thanks @charleenfei :) Left some initial thoughts. Very open to differing opinions

MAINTAINERSHIP.md Outdated Show resolved Hide resolved


<p align="center">
<img src="maintainership.png?raw=true" alt="maintainership" width="80%" />
Copy link
Contributor

Choose a reason for hiding this comment

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

One question about the image, if there isn't an ICS spec, there should be right? Or is it supposed to imply an ICS spec won't always be necessary?

Copy link
Contributor Author

@charleenfei charleenfei Oct 14, 2022

Choose a reason for hiding this comment

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

some cases (specified in the spec section below) will not require an ics spec -- we can update the image to add the spec process as a dotted box perhaps to suggest it's not always necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

"Is there an ICS spec?" I think can be "Is an ICS spec required?" ?

MAINTAINERSHIP.md Outdated Show resolved Hide resolved
MAINTAINERSHIP.md Outdated Show resolved Hide resolved
MAINTAINERSHIP.md Outdated Show resolved Hide resolved
MAINTAINERSHIP.md Outdated Show resolved Hide resolved
MAINTAINERSHIP.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
MAINTAINERSHIP.md Show resolved Hide resolved
MAINTAINERSHIP.md Outdated Show resolved Hide resolved
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Thanks @charleenfei :) Everything looks good to me. We can always make modifications as we try this process out



<p align="center">
<img src="maintainership.png?raw=true" alt="maintainership" width="80%" />
Copy link
Contributor

Choose a reason for hiding this comment

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

"Is there an ICS spec?" I think can be "Is an ICS spec required?" ?

Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

Content looks great, I left a few small suggestions and some tiny nits! Great work 🥇!


## Step 2: Submit spec to the IBC protocol repo

A detailed review of the specification can be expected **within 2 weeks** of submission of the specification to the repo. Please notify the specification team if this does not occur, so it can be corrected as soon as possible. Please note that this timeline may be subject to amendment based on complexity of the spec and team capacity considering other ongoing reviews, but we will strive to ensure a 2 week turnaround.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we include an email address for the specification team as we need for product?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question. @AdityaSripal do you have a preference here?


*(this step can be initiated in parallel w/ spec submission, subject to feature complexity)*

Once the spec has been given initial approval, `ibc-go` engineering will coordinate a code walkthrough in preparation for taking the module into the repo. Any requested changes from the `ibc-go` engineering team after the code walkthrough should be discussed and/or addressd in a timely manner.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Once the spec has been given initial approval, `ibc-go` engineering will coordinate a code walkthrough in preparation for taking the module into the repo. Any requested changes from the `ibc-go` engineering team after the code walkthrough should be discussed and/or addressd in a timely manner.
Once the spec has been given initial approval, `ibc-go` engineering will coordinate a code walkthrough in preparation for taking the module into the repo. Any requested changes from the `ibc-go` engineering team after the code walkthrough should be discussed and/or addressed in a timely manner.


The code that is presented should adhere to our [code contributor guidelines](https://github.com/cosmos/ibc-go/blob/main/CONTRIBUTING.md).

More details on what code walkthrough should cover will be provided by the `ibc-go` engineering team on a case by case basis. However, the code should be sufficiently unit and E2E tested. Think about preparing for this process similarly to submitting a codebase for audit :)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] we are using walkthrough and walk through, we should use one to stay consistent.
[mega-nit] we should keep punctuation consistent (missing full stop)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can link to the E2E Readme here


More details on what code walkthrough should cover will be provided by the `ibc-go` engineering team on a case by case basis. However, the code should be sufficiently unit and E2E tested. Think about preparing for this process similarly to submitting a codebase for audit :)

Please indicate the expected contribution of your team maintainership, if any. This contribution should also include ideas about devrels support and support for product on social media
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Please indicate the expected contribution of your team maintainership, if any. This contribution should also include ideas about devrels support and support for product on social media
Please indicate the expected contribution of your team maintainership, if any. This contribution should also include ideas about devrels support and support for product on social media.

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

🙏

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thank you, @charleenfei. I just left a couple of nits.

@@ -0,0 +1,74 @@
# Request For Maintainership

This document details the acceptance process for requests from external contributors who require for the `ibc-go` core team to take over **maintainership** of a complex block of code (ie: an application module or a light client implementation). It is the process we also follow internally for features that we develop which will go into the `ibc-go` codebase.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the rest of the docs in the repo we write ibc-go in normal text.

Suggested change
This document details the acceptance process for requests from external contributors who require for the `ibc-go` core team to take over **maintainership** of a complex block of code (ie: an application module or a light client implementation). It is the process we also follow internally for features that we develop which will go into the `ibc-go` codebase.
This document details the acceptance process for requests from external contributors who require for the ibc-go core team to take over **maintainership** of a complex block of code (ie: an application module or a light client implementation). It is the process we also follow internally for features that we develop which will go into the `ibc-go` codebase.


Answer these questions in a requirements doc:

```
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 just write this in normal text as an unordered list.

@charleenfei charleenfei enabled auto-merge (squash) October 25, 2022 09:11
@charleenfei charleenfei merged commit c06c204 into main Oct 25, 2022
@charleenfei charleenfei deleted the charly/maintainership_doc branch October 25, 2022 09:14
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.

5 participants