Skip to content

Conversation

@norberttech
Copy link
Member

@norberttech norberttech commented Jan 8, 2025

Change Log

Added

  • Project ADR's

Fixed

Changed

Removed

Deprecated

Security


Description

Resolves: #1344

Since this is strongly impacting contributions I would love to hear opinions about this PR.

@stloyd
@mleczakm
@Bellangelo

I mentioned only the most active recent contributors to approve that PR but anyone is welcome to participate in this conversation, we care about all opinions 🤗

Of Course, ADR's are going to be available also at https://flow-php.com which thanks to amazing https://github.com/thephpleague/commonmark library allowed us to also have "mentions" and many more amazing features!

@@ -0,0 +1,43 @@
# [Decision Title]
Copy link
Contributor

@Bellangelo Bellangelo Jan 8, 2025

Choose a reason for hiding this comment

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

What template is this one? It look close to a MADR template.

Copy link
Member Author

Choose a reason for hiding this comment

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

Norbert template 🤣 I just put that together based on what worked for me before at bigger scale than we are

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the comments of @stloyd, maybe we could use the MADR template? You can find a more complete version of it here: https://github.com/adr/madr/blob/4.0.0/template/adr-template.md

There are also a lot of tools about it: https://adr.github.io/adr-tooling/

Copy link
Member Author

Choose a reason for hiding this comment

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

sure we can, but like with everything, I need some reason 😅 Do you have some past experience with using that template? How it worked over time? Any sections become problematic at some point? Did anyone actually contributed?
Or maybe it was perfect and you would nit change it at all?

I'm not trying here to reject that idea, but I have this principal that whenever alternatives are proposed they must come with clear benefits, otherwise its just art for art's sake.

Anyway I'm open, what would be also helpful when proposing alternatives is to provide the same ADR that I wrote under a different template so we can easier compare them

Copy link
Contributor

Choose a reason for hiding this comment

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

I get your points. Despite my personal experience with it, I mainly suggested it because your template is already close to its minimal version ( as per my last comments ) and adding what stloyd suggested ( #1345 (comment), #1345 (comment) ) basically makes it the full version which I posted in my last comment.

Back to my experience, we use it in my current job and we don't have any problems with it. At the moment only people in the architecture team write them.

I have created this PR which uses the MADR template for the ADR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Despite my personal experience with it, I mainly suggested it because your template is already close to its minimal version ( as per my last comments ) and adding what stloyd suggested ( #1345 (comment), #1345 (comment) ) basically makes it the full version which I posted in my last comment.

yeah, but this is still an art for art's sake since it does not bring any additional benefits.

For example those two sections:

## Decision Drivers

* Availability of maintainers
* Maintenance costs

## Considered Options

* No baseline file - keep everything as is
* Add errors to the baseline file

From my point of view, they are not making ADR better in any way, and the only thing they will create is deter people from contributing/writing ADR's (been there, done that).

Splitting Decision Outcome into Consequences and Confirmation is also not making it better IMO, just more granular but that granularity is not useful anyway.

Back to my experience, we use it in my current job and we don't have any problems with it. At the moment only people in the architecture team write them.

That's one of the things I don't appreciate, some architects think that only architects can write ADR's and they deliver them to the team instead of building them together with the team.
It's pretty much a side effect of making the ADR template overengineered. That approach might work for some systems, but IMO it's not in the spirit of the open source.
I want to make ADR's minimalistic, but still precise, easy to understand but detailed enough to eliminate conflicts. I aim to allow anyone to write an ADR, not just an "architects".

So long story short, unless you can provide some strong arguments about how the project will benefit from having those extra sections I would still move forward with the more minimalistic version.

@stloyd what's your opinion on this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

From my point of view, they are not making ADR better in any way, and the only thing they will create is deter people from contributing/writing ADR's (been there, done that).

The section provide a quick overview of the ADR without reading it completely. Its more like the summary of a book and that's why they are at the top and the details are in the bottom.

Splitting Decision Outcome into Consequences and Confirmation

Here is just a separation of concerns. The Decision should be mostly about the chosen option. The Consequences are the good and the bad things of the chosen option and the Confirmation is how this option would be enforced / applied.

You could indeed group them together, as it happens in the minimal template of MADR.

Copy link
Member Author

Choose a reason for hiding this comment

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

long story short, nothing in madr makes is superior to this one 😅
look, at the end of the day, someone also put it together like this one here and its pretty much a matter of personal preferences which one to use. My preference is to keep it sharp and minimalistic, tailored precisely for our needs.
I'm gonna move forward with this template and if at any point in time it will turn out that something is missing it will get adjusted

@norberttech norberttech requested a review from Bellangelo January 8, 2025 16:59
# Static Analysis Baseline

Proposed by: @norberttech
Date: 2025-01-07
Copy link
Member

Choose a reason for hiding this comment

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

What I miss is: Status: proposal/accepted/denied 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.

There is an explanation already in the ADR's readme that any ADR that is merge is considered accepted, propals are open PR's, denied are closed PR - adding status here IMO is redundant.

My goal with this template is to make it as minimalistic as possible and as descriptive as possible, that's why it does not come with all the sections that are mentioned by @Bellangelo

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively for denied there could be list of PRs with specific tag and link to that list in ADR list, so it would be easily to find if someone already proposed something that got denied, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm we could put a label "ADR" and a link to all closed PRs with label ADR under index od merged ADRs as "Rejected ADRs"

Copy link
Member

Choose a reason for hiding this comment

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

That's what I had in mind 👍

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 🤝

@norberttech
Copy link
Member Author

Implemented changes suggested by @stloyd and added ADR about Extension Points in the project

@norberttech norberttech added the AD label Jan 10, 2025
@norberttech norberttech merged commit 879d4db into flow-php:1.x Jan 10, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Start writing down ADR's

3 participants