You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@airflow.apache.org by Jarek Potiuk <ja...@potiuk.com> on 2022/06/25 09:26:23 UTC
[PROPOSAL] Remove "Label when reviewed" workflow
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.
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Jarek Potiuk <po...@apache.org>.
Merged :)
On Tue, Jun 28, 2022 at 3:37 PM Kaxil Naik <ka...@gmail.com> wrote:
>
> Approved
>
> On Tue, 28 Jun 2022 at 13:53, Jarek Potiuk <po...@apache.org> wrote:
>>
>> https://github.com/apache/airflow/pull/24704 - PR here
>>
>> On Tue, Jun 28, 2022 at 11:01 AM Jarek Potiuk <po...@apache.org> wrote:
>> >
>> > OK. I see no opposition. I will remove it :)
>> >
>> > On Tue, Jun 28, 2022 at 8:23 AM Tobiasz Kędzierski
>> > <to...@gmail.com> wrote:
>> > >
>> > > +1 for removing (non-binding).
>> > >
>> > > wt., 28 cze 2022 o 00:13 Jed Cunningham <je...@apache.org> napisał(a):
>> > >>
>> > >> +1 for removing it.
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Kaxil Naik <ka...@gmail.com>.
Approved
On Tue, 28 Jun 2022 at 13:53, Jarek Potiuk <po...@apache.org> wrote:
> https://github.com/apache/airflow/pull/24704 - PR here
>
> On Tue, Jun 28, 2022 at 11:01 AM Jarek Potiuk <po...@apache.org> wrote:
> >
> > OK. I see no opposition. I will remove it :)
> >
> > On Tue, Jun 28, 2022 at 8:23 AM Tobiasz Kędzierski
> > <to...@gmail.com> wrote:
> > >
> > > +1 for removing (non-binding).
> > >
> > > wt., 28 cze 2022 o 00:13 Jed Cunningham <je...@apache.org>
> napisał(a):
> > >>
> > >> +1 for removing it.
>
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Jarek Potiuk <po...@apache.org>.
https://github.com/apache/airflow/pull/24704 - PR here
On Tue, Jun 28, 2022 at 11:01 AM Jarek Potiuk <po...@apache.org> wrote:
>
> OK. I see no opposition. I will remove it :)
>
> On Tue, Jun 28, 2022 at 8:23 AM Tobiasz Kędzierski
> <to...@gmail.com> wrote:
> >
> > +1 for removing (non-binding).
> >
> > wt., 28 cze 2022 o 00:13 Jed Cunningham <je...@apache.org> napisał(a):
> >>
> >> +1 for removing it.
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Jarek Potiuk <po...@apache.org>.
OK. I see no opposition. I will remove it :)
On Tue, Jun 28, 2022 at 8:23 AM Tobiasz Kędzierski
<to...@gmail.com> wrote:
>
> +1 for removing (non-binding).
>
> wt., 28 cze 2022 o 00:13 Jed Cunningham <je...@apache.org> napisał(a):
>>
>> +1 for removing it.
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Tobiasz Kędzierski <to...@gmail.com>.
+1 for removing (non-binding).
wt., 28 cze 2022 o 00:13 Jed Cunningham <je...@apache.org>
napisał(a):
> +1 for removing it.
>
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Jed Cunningham <je...@apache.org>.
+1 for removing it.
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by "Ferruzzi, Dennis" <fe...@amazon.com.INVALID>.
Yeah, I'd agree that it is just noise.
________________________________
From: Daniel Standish <da...@astronomer.io.INVALID>
Sent: Saturday, June 25, 2022 10:25 AM
To: dev@airflow.apache.org
Subject: RE: [EXTERNAL][PROPOSAL] Remove "Label when reviewed" workflow
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.
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 <ja...@potiuk.com>> 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 <ja...@potiuk.com>> 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
> <da...@astronomer.io.invalid> 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 <ja...@potiuk.com>> 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.
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Daniel Standish <da...@astronomer.io.INVALID>.
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 <ja...@potiuk.com> 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 <ja...@potiuk.com> 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
> > <da...@astronomer.io.invalid> 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 <ja...@potiuk.com> 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.
>
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Jarek Potiuk <ja...@potiuk.com>.
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 <ja...@potiuk.com> 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
> <da...@astronomer.io.invalid> 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 <ja...@potiuk.com> 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.
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Jarek Potiuk <ja...@potiuk.com>.
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
<da...@astronomer.io.invalid> 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 <ja...@potiuk.com> 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.
Re: [PROPOSAL] Remove "Label when reviewed" workflow
Posted by Daniel Standish <da...@astronomer.io.INVALID>.
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 <ja...@potiuk.com> 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.
>