Ah yes I think I fully tune out that one "pr likely to ... this or that...". No concerns with chopping.
Re up-to-date checker... re migrations ... yeah now with the migration reference automated in pre-commit, we always have a conflict in outstanding PRs with migrations when another is merged; just dealt with this recently. If that's the main reason for keeping I think we can remove but if there are other reasons maybe not. On Sat, Jun 25, 2022 at 10:18 AM Jarek Potiuk <[email protected]> wrote: > 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. >
