airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxime Beauchemin <maximebeauche...@gmail.com>
Subject Re: Airflow Committers: Landscape checks doing more harm than good?
Date Fri, 17 Mar 2017 00:02:25 GMT
+1 as well

I'm disappointed because the service is inches away from getting everything
right. As Bolke said, behind the cover it's little more than pylint, git
hooks, and a somewhat-fancy ui.

Operationally it's been getting in the way.

There's a way to pipe the output of git diff into pylint and check whether
the touched lines need linting, in which case we should break the build.
This could run in it's own slot in the Travis build matrix.

Max

On Thu, Mar 16, 2017 at 4:51 PM, Bolke de Bruin <bdbruin@gmail.com> wrote:

> We can do it in Travis’ afaik. We should replace it.
>
> So +1.
>
> B.
>
> > On 16 Mar 2017, at 16:48, Jeremiah Lowin <jlowin@apache.org> wrote:
> >
> > This may be an unpopular opinion, but most Airflow PRs have a little red
> > "x" next to them not because they have failing unit tests, but because
> the
> > Landscape check has decided they introduce bad code.
> >
> > Unfortunately Landscape is often wrong -- here it is telling me my latest
> > PR introduced no less than 30 errors... in files I didn't touch!
> > https://github.com/apache/incubator-airflow/pull/2157 (however, it
> gives me
> > credit for fixing 23 errors in those same files, so I've got that going
> for
> > me... which is nice.)
> >
> > The upshot is that Github's "health" indicator can be swayed by minor or
> > erroneous issues, and therefore it serves little purpose other than
> making
> > it look like every PR is bad. This creates committer fatigue, since every
> > PR needs to be parsed to see if it actually is OK or not.
> >
> > Don't get me wrong, I'm all for proper style and on occasion Landscape
> has
> > pointed out problems that I've gone and fixed. But on the whole, I
> believe
> > that having it as part of our red / green PR evaluation -- equal to and
> > often superseding unit tests -- is harmful. I'd much rather be able to
> scan
> > the PR list and know unequivocally that "green" indicates ready to merge.
> >
> > J
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message