And Daniel - the Up-to-date checker actually **is** sometimes useful - this was particularly useful when we had two people adding new migrations of sqlalchemy resulting in two heads. Maybe after the recent changes in naming/sequential migration numbers it is actually not something that is likely to happen (or easier to spot) - previously they were just random hashes, so it was next to impossible to guess if someone merged another migration in the meantime.
On Sat, Jun 25, 2022 at 7:14 PM Jarek Potiuk <[email protected]> wrote: > > This is the one that produces those bot comments) > > > The PR is likely OK to be merged with just subset of tests for > default Python and Database > versions without running the full matrix of tests, because it does not > modify the core of > Airflow. If the committers decide that the full tests matrix is > needed, they will add the label > 'full tests needed'. Then you should rebase to the latest main or > amend the last commit > of the PR, and push it with --force-with-lease. > > in most cases or > > > The PR is likely ready to be merged. No tests are needed as no important > > environment files, > nor python files were modified by it. However, committers might decide > that full test matrix is > needed and add the 'full tests needed' label. Then you should rebase > it to the latest main > or amend the last commit of the PR, and push it with --force-with-lease. > > and sets "ok to merge" > > or > > > The PR most likely needs to run full matrix of tests because it modifies > > parts of the core > of Airflow. However, committers might decide to merge it quickly and > take the risk. > If they don't merge it quickly - please rebase it to the latest main > at your convenience, > or amend the last commit of the PR, and push it with --force-with-lease. > > and sets "full tests needed" > > It does it based on the heuristics of what files were modified in the PR. > > I believe (and maybe that's the reason you have not seen it) - we, ve > all developed a sort of blind spot for it. > Those messages are anyhow useful (or supposed to be useful) for > committers to guide them in > their merging decisions, but I **think** we are able to judge > ourselves and even if we make mistake, > no real harm is done (except occasional full-test-matrix failures). > > J. > > > On Sat, Jun 25, 2022 at 5:57 PM Daniel Standish > <[email protected]> wrote: > > > > I never noticed this one.... but on the topic of useless CI checks > > Up-to-date checker seems like a strong candidate for removal for similar > > reasons. > > > > On Sat, Jun 25, 2022 at 2:26 AM Jarek Potiuk <[email protected]> wrote: > >> > >> Hey all, > >> > >> I think this workflow we have in CI is rather useless now. > >> It has been somewhat useful at some point in the past when we tried to > >> automate CI checks and see if we can get more info if the PR needs > >> more thorough check (i.e. whether it touches core airflow, or whether > >> it requires "full tests" but there are few problems: > >> > >> * it introduces a lot of noise > >> * it's not 100% foolproof - there are both false positives and false > >> negatives > >> * we can (and sometimes do) manually set the "full test needed" label > >> for PRs that need more "thorough" CI check before merge > >> > >> I personally see that as mostly noise for quite some time now and I > >> see no value in it. Anyone disagree? > >> > >> Should we remove it ? > >> > >> J.
