airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeremiah Lowin <jlo...@apache.org>
Subject Re: Airflow Committers: Landscape checks doing more harm than good?
Date Fri, 17 Mar 2017 00:31:01 GMT
Sounds like a probable consensus. I think this is a good time for me to
admit I have no idea how to turn it off or move it to Travis.

Could anyone please volunteer to take that on?

On Thu, Mar 16, 2017 at 8:11 PM Alex Guziel <alex.guziel@airbnb.com.invalid>
wrote:

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