accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sean Busbey <bus...@cloudera.com>
Subject Re: moving rat to a profile?
Date Fri, 11 Jul 2014 11:07:54 GMT
Changes to ease ignoring the rat report (but leaving it enabled to fail the
build by default) only help established contributors. Our changes to make
clean up of trash easier similarly only help more established contributors.

We have a duty to help grow the community. A part of that is committers
helping to ease new contributors into the requirements that they need to
fulfill. We already know that having the rat plugin fail the build, in
combination with it having no way to ignore trash from other branches, is a
stumbling block for new contributors.

It is much better to have a committer guide a new contributor through
something like a missing license header than it is to have that new
contributor run into the current failure.

* It's a failed build when they "haven't done anything yet" - something I
use as a major red flag about new communities and I'm sure others do as well
* It's a failure message that gives you 0 information until you dig into
the report
* In the most common case, that report tells you it's a problem unrelated
to anything you did
* It's a problem that isn't fixed with "mvn clean", so new users have to
ask for guidance (presuming they don't give up)

Provided that we have instructions on how committers should be checking new
contributions with the plugin combined with instructions for committers to
default the plugin to fail the build, what is the added risk?

We'll have Jenkins as a fall back to keep us honest, just like we do to
make sure committers are checking that things compile at all.


On Thu, Jul 10, 2014 at 10:25 PM, Eric Newton <eric.newton@gmail.com> wrote:

> -1 for removing the rat plugin for default builds
> +1 for a simple way to ignore it
>
> I find Christopher's reasoning sound.  I was guilty of committing half the
> non-compliant files before the rat check was on by default.
>
> I wish the rat check was less annoying about build trash.
> I wish the rat check produced error messages that didn't require me to
> consult a report.
> I wish the template mechanism in eclipse worked every time so source files
> would always get the copyright header by default.
>
> But, automating the check, and moving it to an earlier point in the dev
> cycle is a good thing.
>
> -Eric
>
>
>
>
>
> On Thu, Jul 10, 2014 at 6:09 PM, Christopher <ctubbsii@apache.org> wrote:
>
> > Since this came up in Review (https://reviews.apache.org/r/23391/), I'm
> > revisiting this thread.
> >
> > In short, it seems there was some confusion on whether or not we had
> > reached consensus on enabling rat by default, with rat.ignoreErrors=true
> > set to default, and requiring any entities that should be responsible for
> > checking legal status of contributions to turn it in (set
> > rat.ignoreErrors=false for Jenkins, committers, etc.)
> >
> > I think one problem is that there was not actually consensus. This was a
> > proposal made previously by Bill Havanki to do the above, after which we
> > agreed to minimally making changes to make "git clean -df" sufficient.
> But
> > there did not appear to be any consensus reached on disabling error
> > checking by default, as proposed. Billie had expressed concerns about
> > encouraging committers to not take care in checking the legal status of
> > their commits, and not only do I share those concerns, I'd also extend my
> > concerns to any contributor. This is simply something that everybody
> > contributing to ASF should be aware of. Committers have the extra
> > responsibility, but even contributors and potential committers should
> > understand the contribution requirements, the development process, and
> have
> > put in some minimal amount of thought about whether they have the ability
> > to contribute a particular piece of code legally.
> >
> > Additionally, I'm concerned about who we'd be easing things for. There's
> > clearly an increased risk of commits/contributions without strict
> checking
> > for licenses (we've seen past commits with these errors... just do `git
> log
> > | grep -i license` to see that). So, we're accepting that risk by making
> > the easy route one that serves who, exactly? The ASF makes it quite clear
> > that SCM is for developers, not end users, and this is an SCM-specific
> > problem (specifically branch switching in git).
> >
> > So, I do not think there is consensus. There are certainly outstanding
> > concerns for disabling the rat check by default. However, I think there
> is
> > room for further discussion and compromise, and I'd love to consider more
> > solutions (or discuss the merits of the proposed solutions). For
> instance,
> > one area of compromise I've conceded (as stated in the review) is that I
> > think it'd be a good idea to put the rat check in a profile which can be
> > optionally disabled, so that it doesn't take the 3 extra seconds to run,
> > which could impact some iterative testing (like running one IT at a time,
> > or doing automated `git bisect`).
> >
> >
> >
> >
> > --
> > Christopher L Tubbs II
> > http://gravatar.com/ctubbsii
> >
> >
> > On Thu, Jun 19, 2014 at 11:15 AM, Christopher <ctubbsii@apache.org>
> wrote:
> >
> > > Agreed. That's a minimum.
> > >
> > >
> > > --
> > > Christopher L Tubbs II
> > > http://gravatar.com/ctubbsii
> > >
> > >
> > > On Thu, Jun 19, 2014 at 10:54 AM, Bill Havanki <
> > bhavanki@clouderagovt.com>
> > > wrote:
> > >
> > >> I've filed ACCUMULO-2927 to make 'git clean -df' sufficient. No matter
> > how
> > >> we decide about the rat plugin, I think not requiring -x is a
> worthwhile
> > >> goal.
> > >>
> > >>
> > >> On Tue, Jun 17, 2014 at 5:43 PM, Christopher <ctubbsii@apache.org>
> > wrote:
> > >>
> > >> > I agree that instructing users to use this option to modify the
> build
> > >> isn't
> > >> > acceptable and I wouldn't recommend this as a response to users...
I
> > was
> > >> > only stating this as a fact, to point out that a special profile on
> by
> > >> > default with an option to disable isn't needed, since that's the
> > current
> > >> > behavior.
> > >> >
> > >> > I'm more interested in the targeted .gitignore with the recommended
> > "git
> > >> > clean -df" option without -x. This helps contributors understand
> build
> > >> > tools, makes them aware of the differences between branches, and
> > doesn't
> > >> > hide problems introduced by switching branches in an obscure error,
> > all
> > >> > without blowing away their IDE build files. (though switching
> branches
> > >> > often warrants blowing these IDE files away anyway, since different
> > >> modules
> > >> > in different branches will be problematic for most IDEs).
> > >> >
> > >> >
> > >> > --
> > >> > Christopher L Tubbs II
> > >> > http://gravatar.com/ctubbsii
> > >> >
> > >> >
> > >> > On Tue, Jun 17, 2014 at 4:47 PM, Alex Moundalexis <
> > >> alexm@clouderagovt.com>
> > >> > wrote:
> > >> >
> > >> > > This kind of response is hardly conducive to prospective
> > contributors.
> > >> > >
> > >> > > We should consider ourselves lucky whenever a contributor
> provides a
> > >> > patch,
> > >> > > let alone runs a build. Expecting a new contributor be fully
aware
> > of
> > >> the
> > >> > > Apache licensing details isn't realistic, much less being aware
of
> > the
> > >> > > arguments concerning Rat; if the ignoreErrors argument is TheWay,
> it
> > >> > ought
> > >> > > to be mentioned prominently in the source documentation [1],
but I
> > >> don't
> > >> > > think that's correct either...
> > >> > >
> > >> > > I don't want to encourage contributors to skip the build. I want
> > >> > > contributors to be aware of the licensing requirements, but not
at
> > the
> > >> > > expense of providing otherwise-viable patches. I'd recommend
> > relaxing
> > >> the
> > >> > > Rat checks for contributors, and making it a required part of
the
> > >> profile
> > >> > > for automated Jenkins builds and during the release process.
> > >> > >
> > >> > > The onus should be on the committers to ensure that all of the
> > >> licensing
> > >> > is
> > >> > > in place before the release, but preferably long before that
point
> > by
> > >> > > guiding the contributor to make the necessary license additions
> > before
> > >> > the
> > >> > > commit.
> > >> > >
> > >> > > I've been told to correct whitespace at the end of a line before
> and
> > >> to
> > >> > > re-submit a patch, seems trivial to address missing licensing
> files
> > in
> > >> > the
> > >> > > same way.
> > >> > >
> > >> > > [1] https://accumulo.apache.org/source.html
> > >> > >
> > >> > > On Tue, Jun 17, 2014 at 3:15 PM, Christopher <ctubbsii@apache.org
> >
> > >> > wrote:
> > >> > >
> > >> > > > There's already a way to skip it for those who don't understand
> > why
> > >> its
> > >> > > > failing and are incapable/unwilling to troubleshoot:
> > >> > > > -Drat.ignoreErrors=true
> > >> > > >
> > >> > > >
> > >> > > > --
> > >> > > > Christopher L Tubbs II
> > >> > > > http://gravatar.com/ctubbsii
> > >> > > >
> > >> > > >
> > >> > > > On Tue, Jun 17, 2014 at 3:09 PM, Billie Rinaldi <
> > >> > > billie.rinaldi@gmail.com>
> > >> > > > wrote:
> > >> > > >
> > >> > > > > I'm not thrilled about turning it off by default. 
How about
> > >> putting
> > >> > it
> > >> > > > in
> > >> > > > > a profile that would be enabled by default, but could
be
> > disabled
> > >> > with
> > >> > > a
> > >> > > > > flag for those who don't understand why it's failing?
> > >> > > > >
> > >> > > > >
> > >> > > > > On Tue, Jun 17, 2014 at 11:44 AM, Sean Busbey <
> > >> busbey@cloudera.com>
> > >> > > > wrote:
> > >> > > > >
> > >> > > > > > I've had a few different new-to-Accumulo contributors
> recently
> > >> run
> > >> > > into
> > >> > > > > the
> > >> > > > > > issue of Rat failing the build after changing
branches.
> > >> > > > > >
> > >> > > > > > I know we already have a warning about this[1],
but AFAICT
> > it's
> > >> > over
> > >> > > > the
> > >> > > > > > threshold for consumable information.
> > >> > > > > >
> > >> > > > > > Even after pointing people to the warning, the
existing
> > >> workaround
> > >> > > > > tripped
> > >> > > > > > up atleast one of them. Despite the warning about
using "git
> > >> > clean,"
> > >> > > > the
> > >> > > > > > destruction of their local IDE changes were surprising.
> > >> > > > > >
> > >> > > > > > For contributions to Accumulo that aren't coming
from
> > >> committers,
> > >> > the
> > >> > > > Rat
> > >> > > > > > plugin seems much more likely to give a false
positive than
> to
> > >> > catch
> > >> > > an
> > >> > > > > > error. Additionally, whatever committer is reviewing
the
> > >> > contribution
> > >> > > > > > should be checking for license compliance anyways.
> > >> > > > > >
> > >> > > > > > In the interests of reducing the surprise for
new
> > contributors,
> > >> I'd
> > >> > > > like
> > >> > > > > to
> > >> > > > > > move our use of Rat to a profile that is only
default
> enabled
> > >> > during
> > >> > > a
> > >> > > > > > release run.
> > >> > > > > >
> > >> > > > > > The profile would still let those who want rat
to run on
> every
> > >> > build
> > >> > > to
> > >> > > > > > enable it and we could update the guide for handling
new
> > >> > > contributions
> > >> > > > to
> > >> > > > > > say committers should enable the rat profile to
help guard
> > >> against
> > >> > > > > errors.
> > >> > > > > >
> > >> > > > > > Any objections?
> > >> > > > > >
> > >> > > > > > [1]: http://accumulo.apache.org/source.html#running-a-build
> > >> > > > > >
> > >> > > > > > --
> > >> > > > > > Sean
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >>
> > >>
> > >> --
> > >> // Bill Havanki
> > >> // Solutions Architect, Cloudera Govt Solutions
> > >> // 443.686.9283
> > >>
> > >
> > >
> >
>



-- 
Sean

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