-
-
Notifications
You must be signed in to change notification settings - Fork 48
Introduce ADR's to the project #1345
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
Introduce ADR's to the project #1345
Conversation
| @@ -0,0 +1,43 @@ | |||
| # [Decision Title] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 fileFrom 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| # Static Analysis Baseline | ||
|
|
||
| Proposed by: @norberttech | ||
| Date: 2025-01-07 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added 🤝
|
Implemented changes suggested by @stloyd and added ADR about Extension Points in the project |
Change Log
Added
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!