[OpenStack-Infra] Puppet lint checks for system-config

Paul Belanger pabelanger at redhat.com
Wed Aug 12 16:51:30 UTC 2015


On Wed, Aug 12, 2015 at 09:00:10AM -0700, James E. Blair wrote:
> Paul Belanger <pabelanger at redhat.com> writes:
> 
> ...
> > However, recently. I got my hand smacked in 2 different code reviews for arrow
> > alignment issues. Honestly, I wasn't even mad about the -1 for the alignment.
> > However, I'm concerned about the wasted effort the -1 caused me. Basically, I
> > had to wait a few days to get the -1, since it was a human doing the review, not
> > the gate. Additionally, if I was getting a -1 for style checks, why didn't
> > jenkins do it?
> 
> To me, this is the crux of the problem.  It's okay for us to have a
> conversation about whether we want to change the lint rules for
> system-config, but for the moment, we have chosen to have them
> disabled (let's set that conversation aside for now).
> 
Ya, this was the initial reason for my post, to revisit the discussion.

> Consider that when you leave a -1 on a code review, you are asking a
> contributor to go back and re-do their work.  You are also asking other
> reviewers to re-review a change.  It's a lot of work, and to do it
> frivolously means we are wasting peoples' time.  So, please, when you
> think about whether to leave a -1, don't think "Aha!  I found something
> that could be improved!", instead think "I found something, but is it
> worth redoing already completed work?"
> 
> In particular, many of us have a very strong aversion to nitpicking
> about whitespace.  In many cases, we are not in universal agreement on
> what constitutes the most aesthetically pleasing indentation.  In these
> cases, we often loosen the enforcement of our tools so that a wider
> variety of possibilities is accepted.  If we've made such a choice,
> consider embracing the philosophy of IDIC and accepting that there may
> be more than one way to wrap a line.
> 
> I have proposed a change to system-config to document what we look for
> in code-review, and what should and should not constitute a -1, so that
> new folks have some background:
> 
> https://review.openstack.org/#/c/212073/
> 
Thanks for doing this.

> -Jim



More information about the OpenStack-Infra mailing list