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 adr/rfc section to contributing #15385

Merged
merged 13 commits into from
Mar 21, 2023
Merged

docs: add adr/rfc section to contributing #15385

merged 13 commits into from
Mar 21, 2023

Conversation

tac0turtle
Copy link
Member

Description

This pr adds a section to contributing on when to write an ADR or an RFC. Would love feedback on if this makes sense.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@tac0turtle tac0turtle requested a review from a team as a code owner March 14, 2023 10:08
@aaronc
Copy link
Member

aaronc commented Mar 14, 2023

Based on this flow chat I think the ValidateBasic RFC might actually be an ADR... Are there existing processes in other projects we can reference?

@tac0turtle
Copy link
Member Author

Based on this flow chat I think the ValidateBasic RFC might actually be an ADR... Are there existing processes in other projects we can reference?

This diagram was used in tendermint so i copied it over. I can modify it to be more towards capturing conversations and documenting them

the reason i think its more of a RFC is because how its removed is a bit more unclear, in the sdk for now we are making them a noop but in the future it would be remove entirely.

@aaronc
Copy link
Member

aaronc commented Mar 14, 2023

A few links I came across:

Based on this we have been using ADRs as RFCs. I think a more simple flow chart would be:

  • are we considering designs and want feedback? -> RFC
  • have we made a decision and want to document and report it? - ADR

ADR means decision record so really we shouldn't be requesting any comments at that stage really, right?

@tac0turtle
Copy link
Member Author

tac0turtle commented Mar 14, 2023

this makes sense to me. I think we should still get comments on adrs but not as much as an rfc or ADR currently.

Ill adjust

@aaronc
Copy link
Member

aaronc commented Mar 14, 2023

Yeah maybe we can think of an ADR review as a last call for comments space. Any ADR that has evolved into a longer discussion should have been an RFC

@aaronc
Copy link
Member

aaronc commented Mar 14, 2023

Although our ADR process is currently like an RFC process: https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/PROCESS.md. We should probably split a good part of this out to RFCs

Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

generally looks good
aaron's comment here makes more sense

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

We should remove or update the image since it no longer reflects this process

docs/architecture/PROCESS.md Outdated Show resolved Hide resolved
docs/architecture/PROCESS.md Outdated Show resolved Hide resolved

5. Merged RFCs SHOULD NOT be pruned.

6. If there is consensus and enough feedback, an ADR can be written on
Copy link
Member

@aaronc aaronc Mar 14, 2023

Choose a reason for hiding this comment

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

We should maybe say about more about when and how RFCs turn into ADRs (or not). Does an RFC stop being an RFC and become an ADR once there's consensus? Or does it remain an RFC and there's a separate ADR? Or is it just an RFC with status accepted and no ADR?

Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

nits

CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/architecture/PROCESS.md Outdated Show resolved Hide resolved
docs/architecture/PROCESS.md Outdated Show resolved Hide resolved
@tac0turtle tac0turtle enabled auto-merge (squash) March 21, 2023 12:08
@tac0turtle tac0turtle merged commit b77e51a into main Mar 21, 2023
@tac0turtle tac0turtle deleted the marko/rfc/adr_doc branch March 21, 2023 12:11
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.

4 participants