These guidelines make sense generally, but I’m concerned a bit with the requirement for two reviews for core changes.
1. It’s not clear to me what core changes are. Does this just mean anything not related to providers? 2. Airflow, to put it delicately, does not excel at getting changes merged in a timely manner. I routinely find myself asking stakeholders multiple times to review a change, and that’s before addressing any of their feedback. I’m worried that doubling the approval requirement will exacerbate one problem, without meaningfully solving the core issue of changes getting released with bugs or breaking changes. There’s also a psychological component to consider. If a PR has passed all tests and been reviewed/approved by another committer, it’s far too easy for the second reviewer to simply rubber-stamp without doing the kind of deep review needed to catch bugs that were missed by the original programmer and the first reviewer. Thanks, James On Nov 11, 2021, 11:03 PM -0800, Jarek Potiuk <[email protected]>, wrote: > Good guideline. All for it. Good point Ace, about automation. I think > any "sustainable" guidelines that are going to survive and be followed > in the future should be automated. > > I actually think we should introduce automation for those - especially > that this seems entirely possible: > > 1) I believe it should be possible (and even easy) to add a check in > CI that "2 reviews from committers are needed" when any of the core > files change. Should be easy by the right "change set". We already > have all the tools in our selective_chceks and notification to do > that. We could even make a new "check" (similarly as we do > "up-to-date" check for migrations" that will signal the status nicely > if a core change has not been reviewed by two committers. > > 2) Similarly we could also similarly flag a code that does not change > unit tests. This would also be a nice "signal" for new contributors, > that they still have some work to do - and we could explain why in the > message. That makes for a nice communication tool towards new > contributors.. > > 3) Rests for migration raised by Ace- I think that is a good idea, but > we would need to run it outside of the main CI, because the only > reasonable tests that we can do if we run it or "biggish" amount of > data. The problems with migrations are only really apparent when the > amount of data is sizable. But maybe we could do that as an automated > "pre-release" check - we just need a dump of a biggish database > (mysql/postgres/mssql) and run the migration and measure the added > migration time. > > J. > > On Fri, Nov 12, 2021 at 5:08 AM Ace Haidrey > <[email protected]> wrote: > > > > Hey Kaxil, > > I think this is great guidelines. Quick question , do we have tests > > watching an increase in runtime for installing the migration scripts for > > example? Any monitoring there? > > > > On Nov 11, 2021, at 4:26 PM, Kaxil Naik <[email protected]> wrote: > > > > Hi all committers and reviewers, > > > > Let’s be more stricter for PR reviews. Some of the PRs have slipped by and > > merged (I have been guilty too) that had breaking changes in the last > > couple of versions which are now fixed but let's be more vigilant. > > > > I propose the following guidelines (not rules): > > > > Ask for unit tests coverage wherever applicable > > Require at least 2 approvals for Core changes > > Be extra sensitive to DB migrations > > > > Verify the logic to confirm that it would not take an unreasonable amount > > of time to run it - especially the ones containing task_instance table. > > Use the utilities added in > > https://github.com/apache/airflow/commit/7622f5e08261afe5ab50a08a6ca0804af8c7c7fe > > to create migrations to avoid cases where for example we miss precisions > > for datetime for MySQL - PR. > > Ideally, each Migration should be idempotent. > > > > At least 1 minor release every 3 months so we don't diverge hugely from the > > main branch > > > > > > Thanks. > > > > Regards, > > Kaxil > > > >
