airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Guziel <alex.guz...@airbnb.com.INVALID>
Subject Re: Airflow Committers: Landscape checks doing more harm than good?
Date Fri, 17 Mar 2017 00:03:45 GMT
+1 also

We have code review already and the amount of false positives makes this
useless.

On Thu, Mar 16, 2017 at 5:02 PM, Maxime Beauchemin <
maximebeauchemin@gmail.com> wrote:

> +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