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

Update the ADR process #7498

Closed
4 tasks
robert-zaremba opened this issue Oct 9, 2020 · 22 comments · Fixed by #7621
Closed
4 tasks

Update the ADR process #7498

robert-zaremba opened this issue Oct 9, 2020 · 22 comments · Fixed by #7621
Assignees
Labels
T: ADR An issue or PR relating to an architectural decision record

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Oct 9, 2020

Summary

During the Architecture call on 2020-10-09 we discussed the current ADR process and when and how we should change an ADR status. We propose to review this current process and see how we can make it more efficient

Problem Definition

This is how it works currently:

  • An author drafts an ADR
  • Once the draft it ready, ADR is published as a pull request. During that time the ADR status = proposed.
  • During the PR review process, we try to validate the legitimacy of the problem and the soundness of the solution.
  • We also want to check if we can reuse one of the existing solutions.
  • If the ADR is not accepted, then we .... do we have a clear process? Merge / don't merge the PR?
  • Otherwise, once all issues are addressed and the ADR is validated and ready to merge, the default next step is to change the status to "accepted".

This can be an overkill for a complex ADR. Without proper management, it can put some ADRs in a very lengthy cycle, and putting all context into one big discussion. Also, exploring ADRs is more tricky - we need to go through all merged, active PR or closed PR the list of ADRs.

Proposal 1

We can apply an iterative approach. Instead of trying to solve all issues in one PR, we can focus first on:

  • clear understatement
  • collecting the feedback
  • address all issues which are easy to address

This will also encourage of creating the ADRs without a clear solution and focusing on problem / a need validation.

ADR status

Proposal: change the status types and status flow:

DRAFT ->  REVIEW -> LAST CALL yyyy-mm-dd -> FINAL | REJECTED -> SUPERSEEDED by ADRxxx
               \->   ABANDONED
  • DRAFT: an ADR which is work in progress, without a being ready for a general review. This is to present an early work and get an early feedback
  • REVIEW: and ADR covering a full solution architecture
  • LAST CALL <date for the last call>: clear notify that we are close to accept updates
  • FINAL: ADR which will represent a currently implemented or to be implemented architecture design.
  • SUPERSEEDED by ADRxxx: ADR which has been superseded by a new ADR.
  • ABANDONED: the ADR is no longer pursued by the original authors.

NOTE about state change:

  • It is possible to jump through the some states (eg going from REVIEW -> FINAL) if something is clear and needed.
  • We can make a PR for an ADR without a state change. Example: for an ADR in REVIEW, a PR can address an iterative improvements.

ADR sub directories

To easily navigate between files which are in draft / review we can introduce one subdirectory: wip - this will collect all ADRs which are in DRAFT or in REVIEW

Proposal 2

As above, but use versioning for the Final / Accepted ADRs, eg:

ACCEPTED v2

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added the T: ADR An issue or PR relating to an architectural decision record label Oct 9, 2020
@robert-zaremba
Copy link
Collaborator Author

The process above resembles an Improvement Proposal processes. Please comment, and if some things are not necessary from the ADR perspective (eg too many statuses) we can scrap them.

@alexanderbez
Copy link
Contributor

  • If the ADR is not accepted, then we .... don't merge the PR?
  • Otherwise, once all issues are addressed and the ADR is validated and ready to merge, the default next step is to change the status to "accepted".

The PR should always be merged. In the case of a faulty ADR, we still (should) merge it with a status of rejected. The only time the ADR should not be merged is if the author abandons it.

This can be an overkill for a big PRs. Without proper management, it can put some ADRs in a very lengthy cycle, and putting all context into one big discussion.

What's a big PR? You mean a verbose ADR? True, the review cycle can be lengthy.

Also, exploring ADRs is more tricky - we need to go through all merged, active PR or closed PR the list of ADRs.

IMHO, this is orthogonal to the actual process. We could theoretically have the same process we do now and just organize better (as you've suggested).

I would also propose we use ACCEPTED instead of FINAL. Otherwise, I generally agree with this proposal 👍

@robert-zaremba
Copy link
Collaborator Author

big PR - I should find a better phrase. Let's say a complex ADR. Does it sound better?

@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Oct 9, 2020

While thinking about the FINAL / ACCEPTED I've got an idea to use accepted with version, eg:

ACCEPTED v1

@alexanderbez
Copy link
Contributor

Sure. However, from my experience generally ADRs & RFCs aren't versioned. They just have a concise changelog of dates/modifications.

@robert-zaremba
Copy link
Collaborator Author

Sure. However, from my experience generally ADRs & RFCs aren't versioned. They just have a concise changelog of dates/modifications.

It will make sense to add versioning instead of superseding the whole ADR. With VC systems / Github we can very efficiently trace the evolution of ADR, which is an advantage in my opinion. If v2 requires more changes, then we will have to options:

  • create a new ADR
  • make change status to REVIEW v2 until it will get accepted. It might be cumbersome if the v2 is not good and we will reject it and go back to v1. Though if we would like to keep always increasing versioning, then we would make v3 which is a rollback to v1 with possible small updates.

@aaronc
Copy link
Member

aaronc commented Oct 12, 2020

My suggestion would be to start with small, meaningful improvements on the current process and then iterate from there.

To me the most cumbersome part of what we currently have is related to statuses. There is stuff that is implemented that is Proposed. I kind of liked the idea of different directories proposed, accepted, etc.

I'm not sure different versions makes sense. It might be better to just deprecate one ADR and write a new one.

@robert-zaremba
Copy link
Collaborator Author

The main purpose here is to make the ADR process more lively by introducing iterations (draft -> last call ... ).

I was thinking what is better: having an ADR read-only once it is accepted, or adding versions. If we can have backward compatible changes, then adding versions will show more the evolution of a given ADR. Maybe that's an idea: backward compatible changes can be made as a new version, breaking changes should come as a new ADR?

@clevinson
Copy link
Contributor

Currently we do have ADR's as updatable, but not versioned. As @alexanderbez mentions above, the inline changelog in the ADR serves that purpose, so that it can be updated with small updates as necessary. I also think versioning ADRs overcomplicates, as its hard to know at what point a version is active (proposed/implemented/deprecated).

It's also worth nothing that right now, the "problem definition" and initial discovery phase is usually done in a github issue. In our current process, it only transitions to an ADR when there feels to be rough consensus on the high level direction, and worthy of somebody writing up a more formal solution / architecture proposal.

I do like the idea of having separate folders for statuses (for readability), but what is still unclear to me is where the commenting / review actually happens if not directly in a PR, as that is the only state in which there can be conversation about an ADR together with explicit references to the content in the document being proposed.

@amaury1093
Copy link
Contributor

Maybe that's an idea: backward compatible changes can be made as a new version, breaking changes should come as a new ADR?

I don't think this is realistic. ADR-020 had a number of breaking changes (pubkey, sequence field...), I don't think creating a new ADR in those cases would have made sense.

Overall, I would favor the current process (it's simple, and fast enough by experience), on top of which we add your recommendation from #7458 (comment): initial PR as proposed, separate PR for accepted (optionally with implementation).

@alexanderbez
Copy link
Contributor

I think the big takeaway here is to have separate directories for ADRs based on their status -- seems there is consensus on that so let's just do that for now. And we can continue to revise and improve on the process in the future. WDYT @robert-zaremba?

@robert-zaremba
Copy link
Collaborator Author

Sounds reasonable. I would also like to break the rule that a PR to merge has to have an accpeted status. For discoverability, and would prefer that we merge the work in progress PRs (have an option for that), and be able to include TODOs, comments in the ADR itself until it's approved. We still have flexibility to merge a well thought and agreed ADR in one go, but for long ADRs we will have power to accept (only if it make sense) ADRs in progress, which will land in the separate directory.

@robert-zaremba
Copy link
Collaborator Author

The only disadvantage of separate directories is that it looses an edit track in git (when you move a file you loose a track). Though, currently we are doing an ADR in one go, so there is no track anyway.

@aaronc
Copy link
Member

aaronc commented Oct 14, 2020

Sounds reasonable. I would also like to break the rule that a PR to merge has to have an accpeted status. For discoverability, and would prefer that we merge the work in progress PRs (have an option for that), and be able to include TODOs, comments in the ADR itself until it's approved. We still have flexibility to merge a well thought and agreed ADR in one go, but for long ADRs we will have power to accept (only if it make sense) ADRs in progress, which will land in the separate directory.

That makes sense.

So what directories would we have? Maybe:

  • proposed
  • accepted
  • deprecated
  • rejected

@robert-zaremba
Copy link
Collaborator Author

The only disadvantage of separate directories is that it looses an edit track in git (when you move a file you loose a track). Though, currently we are doing an ADR in one go, so there is no track anyway.

When thinking more about it, ... if we have a clear lists / tables in the README file of ADRs groupped by status, it would provide similar effect, without having to move files.

@aaronc
Copy link
Member

aaronc commented Oct 14, 2020

When thinking more about it, ... if we have a clear lists / tables in the README file of ADRs groupped by status, it would provide similar effect, without having to move files.

I like this approach 👍

@alexanderbez
Copy link
Contributor

Funny enough that was going to be my initial counter proposal to this issue :)

@clevinson
Copy link
Contributor

When thinking more about it, ... if we have a clear lists / tables in the README file of ADRs groupped by status, it would provide similar effect, without having to move files.

Adding to backlog as it seems there's enough alignment here to proceed with a small PR with the above changes.

@robert-zaremba
Copy link
Collaborator Author

How about wrapping up this task and agreeing on the update:

  • allow incremental changes
  • tables with clear status
  • updated status flow:
REVIEW -> LAST CALL yyyy-mm-dd -> FINAL | REJECTED -> SUPERSEEDED by ADRxxx
               \->   ABANDONED

@robert-zaremba
Copy link
Collaborator Author

and if we have that agreement, I can do a small PR, and we close the issue.

@aaronc
Copy link
Member

aaronc commented Oct 21, 2020

Sure @robert-zaremba. Can you start a PR?

@robert-zaremba
Copy link
Collaborator Author

Sure @robert-zaremba. Can you start a PR?

PR created: #7621

@mergify mergify bot closed this as completed in #7621 Oct 22, 2020
@aaronc aaronc removed the backlog label Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants