Seeing a general positive sentiment here. Not sure if there is any specific action here other than encouraging writing the records and better commit messages.
I will try to encourage others and will add some records in areas where I will see things need to be captured. As usual the regular "lead by example" :) . J. On Tue, Dec 5, 2023 at 5:02 AM Wei Lee <weilee...@gmail.com> wrote: > I like this idea! It would make it easier to follow why these decisions > were made. But if we are to adopt this idea, we might need to decide which > decisions we want to include in these ADRs. Or maybe we could include a > checkbox on the PR template for the committers to decide whether to ask the > author to add an ADR before merging. > > On the other hand, I also like Niko's suggestion a lot. Some basic > annotation/metadata makes it easier to trace the log, and git blame is > really handy. It is easier to check compared to the doc. And the commit > rule doesn't need to be complicated. But yes, this kind of rule would > definitely be a barrier to new contributors, which will be something we > need to consider as well. > > Best, > Wei > > > On Dec 5, 2023, at 6:20 AM, Jarek Potiuk <ja...@potiuk.com> wrote: > > > >> > >> I love this idea! > >> > >> Another option, that I don't think we as a community are very good at, > is > >> putting the context of the change in the git commit message itself. > Those > >> messages are already tightly associated into git history and the code > >> itself via blame without needing to introduce an new concept for this > >> purpose. Those commit messages can be viewed with many existing tools > that > >> can browse Git blame/log. Some projects like Linux or Git itself have > very > >> large commit messages with all the context required, a random example I > >> pulled from the first page of most recent commits: > >> > https://github.com/torvalds/linux/commit/d67f39d2b81b6a8259944d2400c1ff4fe283ff72 > > > > > > Oh absolutely - if the single commit is a "complete" change - I am all > for > > long messages. > > Take a look at some of my commit messages :D. > > > > I'd love some of our commit messages that are detailed and describe the > > context of the PRs. Especially in cases where it affects more than just > > obvious "Adding this or that new operator" for example. > > > > I think that we do not also need to capture all decisions this way. For > me > > this is really a question of having something in between a substantial > > commit (that needs a long message) and AIP. Something that is a "shared" > > and "common" piece of infrastructure that introduces a new "idea" or new > > "common utility" or "concept" to be followed by others in the future. > > > > The "common.sql" and "serde" are IMHO good examples of such "concepts". > > The consequences of decisions made there will be carried over in multiple > > other commits, and it's great to have it written and recorded in the repo > > as a "markdown" file - so that we do not have to find the original commit > > that introduced it. It is right there, where you need it - in the folder > > where the source code lives that it relates to. > > > > I am not a big fan of having "one rule" for all commits and changes, I > > really think we should have a gradation: > > > > 1) simple fix or adding new operator/hook that needs no extra context -> > > fine with single line commit > > 2) bigger fix that needs more explanation why we are doing it/context > that > > will not be obvious after some time -> long commit message explaining why > > we are doing it, context, problem it solves, etc. > > 3) a concept that comes and gets refined in probably multiple PRs > > introducing a "shared" way of doing things that we want others to follow > -> > > ADR describing the concept, decisions, alternatives, consequences (and > can > > evolve over time by adding more records). > > 4) big architectural change which requires a lot of deliberation and > voting > > and has wide impact on the architecture of Airflow -> AIP > > > > I think we do not really want to introduce a lot of bureaucracy, we > should > > only add more "documentation" expectations where it really matters to > keep > > it for future maintainers (and future selves as well as I often find > those > > kind of notes and commit messages very useful months and years from the > > time they were made. > > > > BTW. Something that made my day recently "If bureaucracy was meant to be > > easy, they'd have made the word easier to spell". > > > > J. > > > > > > > >> > >> > >> You can see the git commit messages is MUCH longer than the code change > >> itself even! So if you're curious why that code is the way it is, you > can > >> just git blame it and have all the context there. > >> > >> The ADR having markdown is nice, it allows you a bit more formatting, > but > >> then it also requires a couple more steps to view that formatting. > >> > >> Cheers, > >> Niko > >> > >> ________________________________ > >> From: Vincent Beck <vincb...@apache.org> > >> Sent: Monday, December 4, 2023 11:22:54 AM > >> To: dev@airflow.apache.org > >> Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [DISCUSS] Capturing > >> Architectural decisions (ADRS?) > >> > >> CAUTION: This email originated from outside of the organization. Do not > >> click links or open attachments unless you can confirm the sender and > know > >> the content is safe. > >> > >> > >> > >> AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur > externe. > >> Ne cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous ne > pouvez > >> pas confirmer l’identité de l’expéditeur et si vous n’êtes pas certain > que > >> le contenu ne présente aucun risque. > >> > >> > >> > >> I love this idea. I definitely think it can improve a lot the knowledge > >> sharing across Airflow. Given the history and the number of components > in > >> Airflow, it is hard to keep up with everything, so having these ADRs > would > >> help a lot I think! > >> > >> On 2023/12/03 23:57:11 Jarek Potiuk wrote: > >>> Hey everyone, > >>> > >>> I think we had a bit of clash in > >>> https://github.com/apache/airflow/pull/32319 where both "ideas" > >>> (serialization and common.sql) had not been sufficiently > >>> discussed/explained and I hope we can address it by adding (a bit) more > >>> "whys" to our (developer) documentation. > >>> > >>> I think a number of our past decisions and reasoning behind them are > >> often > >>> staying in the heads of the people who were discussing them, and even > if > >> it > >>> is captured in past discussions, PRs, it's difficult to do "archeology" > >> on > >>> them and re-process them and understand what we wanted to achieve and > >> why. > >>> Some of those are big enough to have impact on future PRs etc. while > not > >>> big enough to get to Airflow Improvement Proposals and I think we miss > >>> a bit of persistent "decision records" for them. > >>> > >>> Two cases in question: Serialization and Common.sql API - both of which > >>> have not been understood well by people involved in one, but not the > >> other > >>> in the past. > >>> > >>> With the "common.sql" PR (https://github.com/apache/airflow/pull/36015 > ) > >> - > >>> my proposal is to add it in the form of ADR ("Architecture Decision > >>> Records' - which is a very simple and lightweight way of storing the > >>> decisions we made - and evolving them. > >>> > >>> ADRs are pretty popular and adopted in mature organisations/projects > and > >>> I've used them in `breeze` > >>> https://github.com/apache/airflow/tree/main/dev/breeze/doc/adr and I > >> think > >>> they are perfect for capturing, context, decisions and putting down the > >>> consequences of some decisions. > >>> > >>> They are usually kept close to the code the decision is about, they are > >>> usually short and describe a single aspect of architectural decision, > and > >>> they are aimed to be read whenever in the future, people who were not > >>> involved in those decisions can easily recover why the decisions are > made > >>> and what are the consequences of it. > >>> > >>> I am not saying - of course - we should do it for all or even most > >> changes > >>> - I am talking about decisions that have potential impact on others - > in > >>> the future. I.e. when we tell (this is how our approach should look in > >> the > >>> future for "general" behaviour. > >>> > >>> Both - serialization and common.sql are good examples of such > decisions - > >>> that I believe deserve to be captured "why" we are doing them and what > we > >>> wanted to achieve. > >>> > >>> WDYT? > >>> > >>> J. > >>> > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >> For additional commands, e-mail: dev-h...@airflow.apache.org > >
