You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by Tim Armstrong <ta...@cloudera.com.INVALID> on 2018/08/01 20:55:36 UTC
Re: Enabling automatic code review precommit job
The flake8 comments should now line up correctly with your diff. I got more
feedback that some of the comments were a little strict (and it's unclear
which ones will be enforced) so I posted a diff to disable some of the
checks:
https://gerrit.cloudera.org/#/c/11102/
On Tue, Jul 31, 2018 at 10:00 AM, Tim Armstrong <ta...@cloudera.com>
wrote:
> Todd pointed out a bug where it was posting flake8 comments that didn't
> align with the diff. I figured out the issue but will keep the job in a
> silent mode for a bit while I monitor it.
>
> On Tue, Jul 31, 2018 at 9:16 AM, Jim Apple <jb...@cloudera.com.invalid>
> wrote:
>
>> This is from my .emacs. The way it works is that you select a region and
>> then M-x pep8-region.
>>
>> (defun pep8-region (&optional b e)
>> (interactive "r")
>> (call-process-region b e
>> "/home/jbapple/.local/bin/autopep8" t t nil
>> "--indent-size=2" "--max-line-length=90" "-a" "-a"
>> "-a" "-a" "-a" "-a" "-a" "-a" "--experimental"
>> "-"))
>>
>> It's . . . not great. For instance, it doesn't understand that if you're
>> already 4 spaces in at the start of the region, you don't want to revert
>> back to 0 or 2 spaces in. That said, it can still be helpful. I don't
>> think
>> this is as sophisticated as clang-format.el.
>>
>> On Tue, Jul 31, 2018 at 9:09 AM, Todd Lipcon <to...@cloudera.com.invalid>
>> wrote:
>>
>> > On Tue, Jul 31, 2018 at 9:02 AM, Tim Armstrong <
>> > tarmstrong@cloudera.com.invalid> wrote:
>> >
>> > > I agree that it might be a little strict at the moment and disallow
>> some
>> > > reasonable formatting. The tool is controlled by the setup.cfg file in
>> > the
>> > > repo so it's easy enough to change the behaviour.
>> > >
>> > > I think we have been a little sloppy with Python style in general so I
>> > > think some of these would be good to change over time. I think the
>> main
>> > > thing I'd like is to align the tool's behaviour with code reviews - if
>> > > we're going to be strict about PEP 8 compliance in code reviews we
>> should
>> > > keep the tool strict.
>> > >
>> > > > 109 : E125 continuation line with same indent as next logical
>> line
>> > > I think this formatting is hard to read and confusing, I'm in favour
>> of
>> > > leaving this enabled.
>> > >
>> > > > 110 : E701 multiple statements on one line (colon)
>> > > This if because of the one-line if statements we have in the code. I
>> > don't
>> > > feel strongly either way as long as we're consistent.
>> > >
>> > > > 129 : E231 missing whitespace after ','
>> > > > 185 : E251 unexpected spaces around keyword / parameter equals
>> > > > 288 : E502 the backslash is redundant between brackets
>> > > These seems like sloppy/inconsistent formatting to me, I'm in favour
>> of
>> > > keeping these enabled and fixing existing code as we go.
>> > >
>> > > > 368 : E302 expected 2 blank lines, found 1
>> > > Our Python code is very inconsistent here, would be nice to make it
>> more
>> > > consistent. I'm in favour of keeping it enabled and fixing as we go.
>> > >
>> > > > 187 : E121 continuation line under-indented for hanging indent
>> > > > 1295 : E128 continuation line under-indented for visual indent
>> > > On one hand it would be nice to follow the PEP 8 style here (
>> > > https://www.python.org/dev/peps/pep-0008/#indentation) but the other
>> > > idioms
>> > > seem fine to me. I've been asked on Python code reviews to switch to
>> the
>> > > PEP 8 indentation style before so I think we should align the tools
>> > > behaviour with what we're going to ask for code reviews.
>> > >
>> >
>> >
>> > Alright, I agree with all of the above. One suggestion, though: is it
>> > possible to get something like 'autopep8' to run in a 'patch formatting'
>> > mode where it only reformats changed lines? Then we could more easily
>> just
>> > run a single command to ensure that our patches are properly formatted
>> > before submitting to review. Or, at the very least, some instructions
>> for
>> > running the same flake8-against-only-my-changed-lines that gerrit is
>> > running?
>> >
>> > -Todd
>> >
>> >
>> > >
>> > >
>> > > On Mon, Jul 30, 2018 at 7:48 PM, Todd Lipcon
>> <to...@cloudera.com.invalid>
>> > > wrote:
>> > >
>> > > > It seems like flake8 might need some adjustment of its policies.
>> Here
>> > are
>> > > > the most common issues in the current test code:
>> > > >
>> > > > 109 : E125 continuation line with same indent as next logical
>> line
>> > > > 110 : E701 multiple statements on one line (colon)
>> > > > 129 : E231 missing whitespace after ','
>> > > > 185 : E251 unexpected spaces around keyword / parameter equals
>> > > > 187 : E121 continuation line under-indented for hanging indent
>> > > > 288 : E502 the backslash is redundant between brackets
>> > > > 368 : E302 expected 2 blank lines, found 1
>> > > > 1295 : E128 continuation line under-indented for visual indent
>> > > >
>> > > > Maybe worth just disabling some of the indentation-related ones to
>> > start?
>> > > >
>> > > >
>> > > > On Mon, Jul 30, 2018 at 4:09 PM, Tim Armstrong <
>> > > > tarmstrong@cloudera.com.invalid> wrote:
>> > > >
>> > > > > I have a few updates.
>> > > > >
>> > > > > I added an automatic build job for docs changes:
>> > > > > https://jenkins.impala.io/job/gerrit-docs-auto-test/
>> > > > >
>> > > > > I'm going to disable the "Build started" message for
>> > > > > gerrit-code-review-checks. It seems a bit too chatty. Let me know
>> if
>> > > you
>> > > > > disagree.
>> > > > >
>> > > > > I'm adding a job that automatically does some checks on the diff
>> and
>> > > > posts
>> > > > > code review comments. I started off with Python flake8 comments.
>> > > > >
>> > > > > Let me know if you see any problems or if it turns out to be too
>> > noisy.
>> > > > >
>> > > > >
>> > > > >
>> > > > > On Mon, Jul 23, 2018 at 11:55 AM, Tim Armstrong <
>> > > tarmstrong@cloudera.com
>> > > > >
>> > > > > wrote:
>> > > > >
>> > > > > > Hi All,
>> > > > > > I'm enabling an automatic precommit job for code reviews
>> uploaded
>> > > to
>> > > > > > gerrit that will run RAT, clang-tidy and a GCC debug
>> compilation.
>> > > This
>> > > > is
>> > > > > > to provide faster feedback on code reviews:
>> > > https://issues.apache.org/
>> > > > > > jira/browse/IMPALA-7317 . I'll add some more checks but I'm
>> wanting
>> > > to
>> > > > > > test the basic mechanism for a bit now.
>> > > > > >
>> > > > > > It excludes docs/ commits.
>> > > > > >
>> > > > > > Let me know if you see any problems with it.
>> > > > > >
>> > > > > > Thanks,
>> > > > > > Tim
>> > > > > >
>> > > > >
>> > > >
>> > > >
>> > > >
>> > > > --
>> > > > Todd Lipcon
>> > > > Software Engineer, Cloudera
>> > > >
>> > >
>> >
>> >
>> >
>> > --
>> > Todd Lipcon
>> > Software Engineer, Cloudera
>> >
>>
>
>
Re: Enabling automatic code review precommit job
Posted by Tim Armstrong <ta...@cloudera.com>.
It's a little annoying to do that since the decision to trigger is made in
the plugin and afaik when nothing is triggered there's no way to post back
a message.
On Thu, Aug 16, 2018 at 1:03 PM, Zoltan Borok-Nagy <bo...@cloudera.com>
wrote:
> I see, thanks for the answer! And also thanks for updating the description
> of the jenkins job.
>
> I don't know if it should trigger on clean rebases, maybe it could just
> write a short comment in gerrit, something like that: "Code review checks
> didn't run because it is a clean rebase. If you do want to run these
> checks, you can do it here: https://jenkins.impala.io/
> gerrit_manual_trigger/
> "
>
> Zoltan
>
>
> On Thu, Aug 16, 2018 at 7:19 PM Tim Armstrong <ta...@cloudera.com>
> wrote:
>
> > Thanks for letting me know and sorry for the confusion. It's fine to go
> > ahead and do a gerrit-verify-dry-run. I think the missing info that I
> > didn't communicate earlier is:
> >
> > - gerrit-verify-dry-run is a superset of gerrit-code-review-checks so
> > it's fine to run gerrit-verify-dry-run without waiting for the
> previous
> > job. The intent is just to prefetch some of the more common reasons
> for
> > merge failures.
> > - I've configured gerrit-code-review-checks to not trigger on clean
> > rebases, which is why it didn't trigger on PS8.
> > - You can manually trigger a patchset if you have the appropriate
> > jenkins access here: https://jenkins.impala.io/gerrit_manual_trigger/
> >
> > It seems like I should probably document this better. As a stopgap I
> > improved the description of the jenkins job.
> >
> > Should I revisit triggering on clean rebases? I was originally thinking
> > that it would add noise, but I can see that it could cause confusion. And
> > of course clean rebases can break compilation.
> >
> > - Tim
> >
> >
> > On Thu, Aug 16, 2018 at 5:33 AM, Zoltan Borok-Nagy <
> > boroknagyz@cloudera.com>
> > wrote:
> >
> > > Hi,
> > >
> > > I might have ran into a bug of gerrit-code-review-checks with this
> > change:
> > > https://gerrit.cloudera.org/#/c/11160/
> > >
> > > The job failed for PS 7, because it didn't contain
> > > bin/jenkins/build-only.sh.
> > >
> > > So I rebased and uploaded PS 8, but that didn't trigger
> > > gerrit-code-review-checks. So I went to jenkins and hit "Retrigger" on
> > the
> > > failed job, but it executed the job against PS 7, not PS 8.
> > >
> > > And when I look at "Build with parameters", I can only see the
> > > IMPALA_REPO_URL and IMPALA_LZO_BRANCH parameters, so I cannot really
> run
> > > this job against the my newest patch set.
> > >
> > > Anyway the change is already +2ed and I could run gerrit-verify-dryrun
> > > which almost certainly do these checks as well, but if it didn't I
> don't
> > > want to commit something that might break gerrit-core-review-checks for
> > > subsequent changes.
> > >
> > > Thanks,
> > > Zoltan
> > >
> > >
> > >
> > > On Wed, Aug 1, 2018 at 10:56 PM Tim Armstrong
> > > <ta...@cloudera.com.invalid> wrote:
> > >
> > > > The flake8 comments should now line up correctly with your diff. I
> got
> > > more
> > > > feedback that some of the comments were a little strict (and it's
> > unclear
> > > > which ones will be enforced) so I posted a diff to disable some of
> the
> > > > checks:
> > > >
> > > > https://gerrit.cloudera.org/#/c/11102/
> > > >
> > > > On Tue, Jul 31, 2018 at 10:00 AM, Tim Armstrong <
> > tarmstrong@cloudera.com
> > > >
> > > > wrote:
> > > >
> > > > > Todd pointed out a bug where it was posting flake8 comments that
> > didn't
> > > > > align with the diff. I figured out the issue but will keep the job
> > in a
> > > > > silent mode for a bit while I monitor it.
> > > > >
> > > > > On Tue, Jul 31, 2018 at 9:16 AM, Jim Apple
> > > <jbapple@cloudera.com.invalid
> > > > >
> > > > > wrote:
> > > > >
> > > > >> This is from my .emacs. The way it works is that you select a
> region
> > > and
> > > > >> then M-x pep8-region.
> > > > >>
> > > > >> (defun pep8-region (&optional b e)
> > > > >> (interactive "r")
> > > > >> (call-process-region b e
> > > > >> "/home/jbapple/.local/bin/autopep8" t t
> nil
> > > > >> "--indent-size=2" "--max-line-length=90"
> "-a"
> > > > "-a"
> > > > >> "-a" "-a" "-a" "-a" "-a" "-a"
> > "--experimental"
> > > > >> "-"))
> > > > >>
> > > > >> It's . . . not great. For instance, it doesn't understand that if
> > > you're
> > > > >> already 4 spaces in at the start of the region, you don't want to
> > > revert
> > > > >> back to 0 or 2 spaces in. That said, it can still be helpful. I
> > don't
> > > > >> think
> > > > >> this is as sophisticated as clang-format.el.
> > > > >>
> > > > >> On Tue, Jul 31, 2018 at 9:09 AM, Todd Lipcon
> > > <todd@cloudera.com.invalid
> > > > >
> > > > >> wrote:
> > > > >>
> > > > >> > On Tue, Jul 31, 2018 at 9:02 AM, Tim Armstrong <
> > > > >> > tarmstrong@cloudera.com.invalid> wrote:
> > > > >> >
> > > > >> > > I agree that it might be a little strict at the moment and
> > > disallow
> > > > >> some
> > > > >> > > reasonable formatting. The tool is controlled by the setup.cfg
> > > file
> > > > in
> > > > >> > the
> > > > >> > > repo so it's easy enough to change the behaviour.
> > > > >> > >
> > > > >> > > I think we have been a little sloppy with Python style in
> > general
> > > > so I
> > > > >> > > think some of these would be good to change over time. I think
> > the
> > > > >> main
> > > > >> > > thing I'd like is to align the tool's behaviour with code
> > reviews
> > > -
> > > > if
> > > > >> > > we're going to be strict about PEP 8 compliance in code
> reviews
> > we
> > > > >> should
> > > > >> > > keep the tool strict.
> > > > >> > >
> > > > >> > > > 109 : E125 continuation line with same indent as next
> > logical
> > > > >> line
> > > > >> > > I think this formatting is hard to read and confusing, I'm in
> > > favour
> > > > >> of
> > > > >> > > leaving this enabled.
> > > > >> > >
> > > > >> > > > 110 : E701 multiple statements on one line (colon)
> > > > >> > > This if because of the one-line if statements we have in the
> > > code. I
> > > > >> > don't
> > > > >> > > feel strongly either way as long as we're consistent.
> > > > >> > >
> > > > >> > > > 129 : E231 missing whitespace after ','
> > > > >> > > > 185 : E251 unexpected spaces around keyword / parameter
> > > equals
> > > > >> > > > 288 : E502 the backslash is redundant between brackets
> > > > >> > > These seems like sloppy/inconsistent formatting to me, I'm in
> > > favour
> > > > >> of
> > > > >> > > keeping these enabled and fixing existing code as we go.
> > > > >> > >
> > > > >> > > > 368 : E302 expected 2 blank lines, found 1
> > > > >> > > Our Python code is very inconsistent here, would be nice to
> make
> > > it
> > > > >> more
> > > > >> > > consistent. I'm in favour of keeping it enabled and fixing as
> we
> > > go.
> > > > >> > >
> > > > >> > > > 187 : E121 continuation line under-indented for hanging
> > > indent
> > > > >> > > > 1295 : E128 continuation line under-indented for visual
> > indent
> > > > >> > > On one hand it would be nice to follow the PEP 8 style here (
> > > > >> > > https://www.python.org/dev/peps/pep-0008/#indentation) but
> the
> > > > other
> > > > >> > > idioms
> > > > >> > > seem fine to me. I've been asked on Python code reviews to
> > switch
> > > to
> > > > >> the
> > > > >> > > PEP 8 indentation style before so I think we should align the
> > > tools
> > > > >> > > behaviour with what we're going to ask for code reviews.
> > > > >> > >
> > > > >> >
> > > > >> >
> > > > >> > Alright, I agree with all of the above. One suggestion, though:
> is
> > > it
> > > > >> > possible to get something like 'autopep8' to run in a 'patch
> > > > formatting'
> > > > >> > mode where it only reformats changed lines? Then we could more
> > > easily
> > > > >> just
> > > > >> > run a single command to ensure that our patches are properly
> > > formatted
> > > > >> > before submitting to review. Or, at the very least, some
> > > instructions
> > > > >> for
> > > > >> > running the same flake8-against-only-my-changed-lines that
> gerrit
> > > is
> > > > >> > running?
> > > > >> >
> > > > >> > -Todd
> > > > >> >
> > > > >> >
> > > > >> > >
> > > > >> > >
> > > > >> > > On Mon, Jul 30, 2018 at 7:48 PM, Todd Lipcon
> > > > >> <to...@cloudera.com.invalid>
> > > > >> > > wrote:
> > > > >> > >
> > > > >> > > > It seems like flake8 might need some adjustment of its
> > policies.
> > > > >> Here
> > > > >> > are
> > > > >> > > > the most common issues in the current test code:
> > > > >> > > >
> > > > >> > > > 109 : E125 continuation line with same indent as next
> > > logical
> > > > >> line
> > > > >> > > > 110 : E701 multiple statements on one line (colon)
> > > > >> > > > 129 : E231 missing whitespace after ','
> > > > >> > > > 185 : E251 unexpected spaces around keyword / parameter
> > > equals
> > > > >> > > > 187 : E121 continuation line under-indented for hanging
> > > indent
> > > > >> > > > 288 : E502 the backslash is redundant between brackets
> > > > >> > > > 368 : E302 expected 2 blank lines, found 1
> > > > >> > > > 1295 : E128 continuation line under-indented for visual
> > > indent
> > > > >> > > >
> > > > >> > > > Maybe worth just disabling some of the indentation-related
> > ones
> > > to
> > > > >> > start?
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > On Mon, Jul 30, 2018 at 4:09 PM, Tim Armstrong <
> > > > >> > > > tarmstrong@cloudera.com.invalid> wrote:
> > > > >> > > >
> > > > >> > > > > I have a few updates.
> > > > >> > > > >
> > > > >> > > > > I added an automatic build job for docs changes:
> > > > >> > > > > https://jenkins.impala.io/job/gerrit-docs-auto-test/
> > > > >> > > > >
> > > > >> > > > > I'm going to disable the "Build started" message for
> > > > >> > > > > gerrit-code-review-checks. It seems a bit too chatty. Let
> me
> > > > know
> > > > >> if
> > > > >> > > you
> > > > >> > > > > disagree.
> > > > >> > > > >
> > > > >> > > > > I'm adding a job that automatically does some checks on
> the
> > > diff
> > > > >> and
> > > > >> > > > posts
> > > > >> > > > > code review comments. I started off with Python flake8
> > > comments.
> > > > >> > > > >
> > > > >> > > > > Let me know if you see any problems or if it turns out to
> be
> > > too
> > > > >> > noisy.
> > > > >> > > > >
> > > > >> > > > >
> > > > >> > > > >
> > > > >> > > > > On Mon, Jul 23, 2018 at 11:55 AM, Tim Armstrong <
> > > > >> > > tarmstrong@cloudera.com
> > > > >> > > > >
> > > > >> > > > > wrote:
> > > > >> > > > >
> > > > >> > > > > > Hi All,
> > > > >> > > > > > I'm enabling an automatic precommit job for code
> reviews
> > > > >> uploaded
> > > > >> > > to
> > > > >> > > > > > gerrit that will run RAT, clang-tidy and a GCC debug
> > > > >> compilation.
> > > > >> > > This
> > > > >> > > > is
> > > > >> > > > > > to provide faster feedback on code reviews:
> > > > >> > > https://issues.apache.org/
> > > > >> > > > > > jira/browse/IMPALA-7317 . I'll add some more checks but
> > I'm
> > > > >> wanting
> > > > >> > > to
> > > > >> > > > > > test the basic mechanism for a bit now.
> > > > >> > > > > >
> > > > >> > > > > > It excludes docs/ commits.
> > > > >> > > > > >
> > > > >> > > > > > Let me know if you see any problems with it.
> > > > >> > > > > >
> > > > >> > > > > > Thanks,
> > > > >> > > > > > Tim
> > > > >> > > > > >
> > > > >> > > > >
> > > > >> > > >
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > --
> > > > >> > > > Todd Lipcon
> > > > >> > > > Software Engineer, Cloudera
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >> >
> > > > >> >
> > > > >> > --
> > > > >> > Todd Lipcon
> > > > >> > Software Engineer, Cloudera
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>
Re: Enabling automatic code review precommit job
Posted by Zoltan Borok-Nagy <bo...@cloudera.com>.
I see, thanks for the answer! And also thanks for updating the description
of the jenkins job.
I don't know if it should trigger on clean rebases, maybe it could just
write a short comment in gerrit, something like that: "Code review checks
didn't run because it is a clean rebase. If you do want to run these
checks, you can do it here: https://jenkins.impala.io/gerrit_manual_trigger/
"
Zoltan
On Thu, Aug 16, 2018 at 7:19 PM Tim Armstrong <ta...@cloudera.com>
wrote:
> Thanks for letting me know and sorry for the confusion. It's fine to go
> ahead and do a gerrit-verify-dry-run. I think the missing info that I
> didn't communicate earlier is:
>
> - gerrit-verify-dry-run is a superset of gerrit-code-review-checks so
> it's fine to run gerrit-verify-dry-run without waiting for the previous
> job. The intent is just to prefetch some of the more common reasons for
> merge failures.
> - I've configured gerrit-code-review-checks to not trigger on clean
> rebases, which is why it didn't trigger on PS8.
> - You can manually trigger a patchset if you have the appropriate
> jenkins access here: https://jenkins.impala.io/gerrit_manual_trigger/
>
> It seems like I should probably document this better. As a stopgap I
> improved the description of the jenkins job.
>
> Should I revisit triggering on clean rebases? I was originally thinking
> that it would add noise, but I can see that it could cause confusion. And
> of course clean rebases can break compilation.
>
> - Tim
>
>
> On Thu, Aug 16, 2018 at 5:33 AM, Zoltan Borok-Nagy <
> boroknagyz@cloudera.com>
> wrote:
>
> > Hi,
> >
> > I might have ran into a bug of gerrit-code-review-checks with this
> change:
> > https://gerrit.cloudera.org/#/c/11160/
> >
> > The job failed for PS 7, because it didn't contain
> > bin/jenkins/build-only.sh.
> >
> > So I rebased and uploaded PS 8, but that didn't trigger
> > gerrit-code-review-checks. So I went to jenkins and hit "Retrigger" on
> the
> > failed job, but it executed the job against PS 7, not PS 8.
> >
> > And when I look at "Build with parameters", I can only see the
> > IMPALA_REPO_URL and IMPALA_LZO_BRANCH parameters, so I cannot really run
> > this job against the my newest patch set.
> >
> > Anyway the change is already +2ed and I could run gerrit-verify-dryrun
> > which almost certainly do these checks as well, but if it didn't I don't
> > want to commit something that might break gerrit-core-review-checks for
> > subsequent changes.
> >
> > Thanks,
> > Zoltan
> >
> >
> >
> > On Wed, Aug 1, 2018 at 10:56 PM Tim Armstrong
> > <ta...@cloudera.com.invalid> wrote:
> >
> > > The flake8 comments should now line up correctly with your diff. I got
> > more
> > > feedback that some of the comments were a little strict (and it's
> unclear
> > > which ones will be enforced) so I posted a diff to disable some of the
> > > checks:
> > >
> > > https://gerrit.cloudera.org/#/c/11102/
> > >
> > > On Tue, Jul 31, 2018 at 10:00 AM, Tim Armstrong <
> tarmstrong@cloudera.com
> > >
> > > wrote:
> > >
> > > > Todd pointed out a bug where it was posting flake8 comments that
> didn't
> > > > align with the diff. I figured out the issue but will keep the job
> in a
> > > > silent mode for a bit while I monitor it.
> > > >
> > > > On Tue, Jul 31, 2018 at 9:16 AM, Jim Apple
> > <jbapple@cloudera.com.invalid
> > > >
> > > > wrote:
> > > >
> > > >> This is from my .emacs. The way it works is that you select a region
> > and
> > > >> then M-x pep8-region.
> > > >>
> > > >> (defun pep8-region (&optional b e)
> > > >> (interactive "r")
> > > >> (call-process-region b e
> > > >> "/home/jbapple/.local/bin/autopep8" t t nil
> > > >> "--indent-size=2" "--max-line-length=90" "-a"
> > > "-a"
> > > >> "-a" "-a" "-a" "-a" "-a" "-a"
> "--experimental"
> > > >> "-"))
> > > >>
> > > >> It's . . . not great. For instance, it doesn't understand that if
> > you're
> > > >> already 4 spaces in at the start of the region, you don't want to
> > revert
> > > >> back to 0 or 2 spaces in. That said, it can still be helpful. I
> don't
> > > >> think
> > > >> this is as sophisticated as clang-format.el.
> > > >>
> > > >> On Tue, Jul 31, 2018 at 9:09 AM, Todd Lipcon
> > <todd@cloudera.com.invalid
> > > >
> > > >> wrote:
> > > >>
> > > >> > On Tue, Jul 31, 2018 at 9:02 AM, Tim Armstrong <
> > > >> > tarmstrong@cloudera.com.invalid> wrote:
> > > >> >
> > > >> > > I agree that it might be a little strict at the moment and
> > disallow
> > > >> some
> > > >> > > reasonable formatting. The tool is controlled by the setup.cfg
> > file
> > > in
> > > >> > the
> > > >> > > repo so it's easy enough to change the behaviour.
> > > >> > >
> > > >> > > I think we have been a little sloppy with Python style in
> general
> > > so I
> > > >> > > think some of these would be good to change over time. I think
> the
> > > >> main
> > > >> > > thing I'd like is to align the tool's behaviour with code
> reviews
> > -
> > > if
> > > >> > > we're going to be strict about PEP 8 compliance in code reviews
> we
> > > >> should
> > > >> > > keep the tool strict.
> > > >> > >
> > > >> > > > 109 : E125 continuation line with same indent as next
> logical
> > > >> line
> > > >> > > I think this formatting is hard to read and confusing, I'm in
> > favour
> > > >> of
> > > >> > > leaving this enabled.
> > > >> > >
> > > >> > > > 110 : E701 multiple statements on one line (colon)
> > > >> > > This if because of the one-line if statements we have in the
> > code. I
> > > >> > don't
> > > >> > > feel strongly either way as long as we're consistent.
> > > >> > >
> > > >> > > > 129 : E231 missing whitespace after ','
> > > >> > > > 185 : E251 unexpected spaces around keyword / parameter
> > equals
> > > >> > > > 288 : E502 the backslash is redundant between brackets
> > > >> > > These seems like sloppy/inconsistent formatting to me, I'm in
> > favour
> > > >> of
> > > >> > > keeping these enabled and fixing existing code as we go.
> > > >> > >
> > > >> > > > 368 : E302 expected 2 blank lines, found 1
> > > >> > > Our Python code is very inconsistent here, would be nice to make
> > it
> > > >> more
> > > >> > > consistent. I'm in favour of keeping it enabled and fixing as we
> > go.
> > > >> > >
> > > >> > > > 187 : E121 continuation line under-indented for hanging
> > indent
> > > >> > > > 1295 : E128 continuation line under-indented for visual
> indent
> > > >> > > On one hand it would be nice to follow the PEP 8 style here (
> > > >> > > https://www.python.org/dev/peps/pep-0008/#indentation) but the
> > > other
> > > >> > > idioms
> > > >> > > seem fine to me. I've been asked on Python code reviews to
> switch
> > to
> > > >> the
> > > >> > > PEP 8 indentation style before so I think we should align the
> > tools
> > > >> > > behaviour with what we're going to ask for code reviews.
> > > >> > >
> > > >> >
> > > >> >
> > > >> > Alright, I agree with all of the above. One suggestion, though: is
> > it
> > > >> > possible to get something like 'autopep8' to run in a 'patch
> > > formatting'
> > > >> > mode where it only reformats changed lines? Then we could more
> > easily
> > > >> just
> > > >> > run a single command to ensure that our patches are properly
> > formatted
> > > >> > before submitting to review. Or, at the very least, some
> > instructions
> > > >> for
> > > >> > running the same flake8-against-only-my-changed-lines that gerrit
> > is
> > > >> > running?
> > > >> >
> > > >> > -Todd
> > > >> >
> > > >> >
> > > >> > >
> > > >> > >
> > > >> > > On Mon, Jul 30, 2018 at 7:48 PM, Todd Lipcon
> > > >> <to...@cloudera.com.invalid>
> > > >> > > wrote:
> > > >> > >
> > > >> > > > It seems like flake8 might need some adjustment of its
> policies.
> > > >> Here
> > > >> > are
> > > >> > > > the most common issues in the current test code:
> > > >> > > >
> > > >> > > > 109 : E125 continuation line with same indent as next
> > logical
> > > >> line
> > > >> > > > 110 : E701 multiple statements on one line (colon)
> > > >> > > > 129 : E231 missing whitespace after ','
> > > >> > > > 185 : E251 unexpected spaces around keyword / parameter
> > equals
> > > >> > > > 187 : E121 continuation line under-indented for hanging
> > indent
> > > >> > > > 288 : E502 the backslash is redundant between brackets
> > > >> > > > 368 : E302 expected 2 blank lines, found 1
> > > >> > > > 1295 : E128 continuation line under-indented for visual
> > indent
> > > >> > > >
> > > >> > > > Maybe worth just disabling some of the indentation-related
> ones
> > to
> > > >> > start?
> > > >> > > >
> > > >> > > >
> > > >> > > > On Mon, Jul 30, 2018 at 4:09 PM, Tim Armstrong <
> > > >> > > > tarmstrong@cloudera.com.invalid> wrote:
> > > >> > > >
> > > >> > > > > I have a few updates.
> > > >> > > > >
> > > >> > > > > I added an automatic build job for docs changes:
> > > >> > > > > https://jenkins.impala.io/job/gerrit-docs-auto-test/
> > > >> > > > >
> > > >> > > > > I'm going to disable the "Build started" message for
> > > >> > > > > gerrit-code-review-checks. It seems a bit too chatty. Let me
> > > know
> > > >> if
> > > >> > > you
> > > >> > > > > disagree.
> > > >> > > > >
> > > >> > > > > I'm adding a job that automatically does some checks on the
> > diff
> > > >> and
> > > >> > > > posts
> > > >> > > > > code review comments. I started off with Python flake8
> > comments.
> > > >> > > > >
> > > >> > > > > Let me know if you see any problems or if it turns out to be
> > too
> > > >> > noisy.
> > > >> > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > > > > On Mon, Jul 23, 2018 at 11:55 AM, Tim Armstrong <
> > > >> > > tarmstrong@cloudera.com
> > > >> > > > >
> > > >> > > > > wrote:
> > > >> > > > >
> > > >> > > > > > Hi All,
> > > >> > > > > > I'm enabling an automatic precommit job for code reviews
> > > >> uploaded
> > > >> > > to
> > > >> > > > > > gerrit that will run RAT, clang-tidy and a GCC debug
> > > >> compilation.
> > > >> > > This
> > > >> > > > is
> > > >> > > > > > to provide faster feedback on code reviews:
> > > >> > > https://issues.apache.org/
> > > >> > > > > > jira/browse/IMPALA-7317 . I'll add some more checks but
> I'm
> > > >> wanting
> > > >> > > to
> > > >> > > > > > test the basic mechanism for a bit now.
> > > >> > > > > >
> > > >> > > > > > It excludes docs/ commits.
> > > >> > > > > >
> > > >> > > > > > Let me know if you see any problems with it.
> > > >> > > > > >
> > > >> > > > > > Thanks,
> > > >> > > > > > Tim
> > > >> > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > > >
> > > >> > > >
> > > >> > > > --
> > > >> > > > Todd Lipcon
> > > >> > > > Software Engineer, Cloudera
> > > >> > > >
> > > >> > >
> > > >> >
> > > >> >
> > > >> >
> > > >> > --
> > > >> > Todd Lipcon
> > > >> > Software Engineer, Cloudera
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>
Re: Enabling automatic code review precommit job
Posted by Tim Armstrong <ta...@cloudera.com>.
Thanks for letting me know and sorry for the confusion. It's fine to go
ahead and do a gerrit-verify-dry-run. I think the missing info that I
didn't communicate earlier is:
- gerrit-verify-dry-run is a superset of gerrit-code-review-checks so
it's fine to run gerrit-verify-dry-run without waiting for the previous
job. The intent is just to prefetch some of the more common reasons for
merge failures.
- I've configured gerrit-code-review-checks to not trigger on clean
rebases, which is why it didn't trigger on PS8.
- You can manually trigger a patchset if you have the appropriate
jenkins access here: https://jenkins.impala.io/gerrit_manual_trigger/
It seems like I should probably document this better. As a stopgap I
improved the description of the jenkins job.
Should I revisit triggering on clean rebases? I was originally thinking
that it would add noise, but I can see that it could cause confusion. And
of course clean rebases can break compilation.
- Tim
On Thu, Aug 16, 2018 at 5:33 AM, Zoltan Borok-Nagy <bo...@cloudera.com>
wrote:
> Hi,
>
> I might have ran into a bug of gerrit-code-review-checks with this change:
> https://gerrit.cloudera.org/#/c/11160/
>
> The job failed for PS 7, because it didn't contain
> bin/jenkins/build-only.sh.
>
> So I rebased and uploaded PS 8, but that didn't trigger
> gerrit-code-review-checks. So I went to jenkins and hit "Retrigger" on the
> failed job, but it executed the job against PS 7, not PS 8.
>
> And when I look at "Build with parameters", I can only see the
> IMPALA_REPO_URL and IMPALA_LZO_BRANCH parameters, so I cannot really run
> this job against the my newest patch set.
>
> Anyway the change is already +2ed and I could run gerrit-verify-dryrun
> which almost certainly do these checks as well, but if it didn't I don't
> want to commit something that might break gerrit-core-review-checks for
> subsequent changes.
>
> Thanks,
> Zoltan
>
>
>
> On Wed, Aug 1, 2018 at 10:56 PM Tim Armstrong
> <ta...@cloudera.com.invalid> wrote:
>
> > The flake8 comments should now line up correctly with your diff. I got
> more
> > feedback that some of the comments were a little strict (and it's unclear
> > which ones will be enforced) so I posted a diff to disable some of the
> > checks:
> >
> > https://gerrit.cloudera.org/#/c/11102/
> >
> > On Tue, Jul 31, 2018 at 10:00 AM, Tim Armstrong <tarmstrong@cloudera.com
> >
> > wrote:
> >
> > > Todd pointed out a bug where it was posting flake8 comments that didn't
> > > align with the diff. I figured out the issue but will keep the job in a
> > > silent mode for a bit while I monitor it.
> > >
> > > On Tue, Jul 31, 2018 at 9:16 AM, Jim Apple
> <jbapple@cloudera.com.invalid
> > >
> > > wrote:
> > >
> > >> This is from my .emacs. The way it works is that you select a region
> and
> > >> then M-x pep8-region.
> > >>
> > >> (defun pep8-region (&optional b e)
> > >> (interactive "r")
> > >> (call-process-region b e
> > >> "/home/jbapple/.local/bin/autopep8" t t nil
> > >> "--indent-size=2" "--max-line-length=90" "-a"
> > "-a"
> > >> "-a" "-a" "-a" "-a" "-a" "-a" "--experimental"
> > >> "-"))
> > >>
> > >> It's . . . not great. For instance, it doesn't understand that if
> you're
> > >> already 4 spaces in at the start of the region, you don't want to
> revert
> > >> back to 0 or 2 spaces in. That said, it can still be helpful. I don't
> > >> think
> > >> this is as sophisticated as clang-format.el.
> > >>
> > >> On Tue, Jul 31, 2018 at 9:09 AM, Todd Lipcon
> <todd@cloudera.com.invalid
> > >
> > >> wrote:
> > >>
> > >> > On Tue, Jul 31, 2018 at 9:02 AM, Tim Armstrong <
> > >> > tarmstrong@cloudera.com.invalid> wrote:
> > >> >
> > >> > > I agree that it might be a little strict at the moment and
> disallow
> > >> some
> > >> > > reasonable formatting. The tool is controlled by the setup.cfg
> file
> > in
> > >> > the
> > >> > > repo so it's easy enough to change the behaviour.
> > >> > >
> > >> > > I think we have been a little sloppy with Python style in general
> > so I
> > >> > > think some of these would be good to change over time. I think the
> > >> main
> > >> > > thing I'd like is to align the tool's behaviour with code reviews
> -
> > if
> > >> > > we're going to be strict about PEP 8 compliance in code reviews we
> > >> should
> > >> > > keep the tool strict.
> > >> > >
> > >> > > > 109 : E125 continuation line with same indent as next logical
> > >> line
> > >> > > I think this formatting is hard to read and confusing, I'm in
> favour
> > >> of
> > >> > > leaving this enabled.
> > >> > >
> > >> > > > 110 : E701 multiple statements on one line (colon)
> > >> > > This if because of the one-line if statements we have in the
> code. I
> > >> > don't
> > >> > > feel strongly either way as long as we're consistent.
> > >> > >
> > >> > > > 129 : E231 missing whitespace after ','
> > >> > > > 185 : E251 unexpected spaces around keyword / parameter
> equals
> > >> > > > 288 : E502 the backslash is redundant between brackets
> > >> > > These seems like sloppy/inconsistent formatting to me, I'm in
> favour
> > >> of
> > >> > > keeping these enabled and fixing existing code as we go.
> > >> > >
> > >> > > > 368 : E302 expected 2 blank lines, found 1
> > >> > > Our Python code is very inconsistent here, would be nice to make
> it
> > >> more
> > >> > > consistent. I'm in favour of keeping it enabled and fixing as we
> go.
> > >> > >
> > >> > > > 187 : E121 continuation line under-indented for hanging
> indent
> > >> > > > 1295 : E128 continuation line under-indented for visual indent
> > >> > > On one hand it would be nice to follow the PEP 8 style here (
> > >> > > https://www.python.org/dev/peps/pep-0008/#indentation) but the
> > other
> > >> > > idioms
> > >> > > seem fine to me. I've been asked on Python code reviews to switch
> to
> > >> the
> > >> > > PEP 8 indentation style before so I think we should align the
> tools
> > >> > > behaviour with what we're going to ask for code reviews.
> > >> > >
> > >> >
> > >> >
> > >> > Alright, I agree with all of the above. One suggestion, though: is
> it
> > >> > possible to get something like 'autopep8' to run in a 'patch
> > formatting'
> > >> > mode where it only reformats changed lines? Then we could more
> easily
> > >> just
> > >> > run a single command to ensure that our patches are properly
> formatted
> > >> > before submitting to review. Or, at the very least, some
> instructions
> > >> for
> > >> > running the same flake8-against-only-my-changed-lines that gerrit
> is
> > >> > running?
> > >> >
> > >> > -Todd
> > >> >
> > >> >
> > >> > >
> > >> > >
> > >> > > On Mon, Jul 30, 2018 at 7:48 PM, Todd Lipcon
> > >> <to...@cloudera.com.invalid>
> > >> > > wrote:
> > >> > >
> > >> > > > It seems like flake8 might need some adjustment of its policies.
> > >> Here
> > >> > are
> > >> > > > the most common issues in the current test code:
> > >> > > >
> > >> > > > 109 : E125 continuation line with same indent as next
> logical
> > >> line
> > >> > > > 110 : E701 multiple statements on one line (colon)
> > >> > > > 129 : E231 missing whitespace after ','
> > >> > > > 185 : E251 unexpected spaces around keyword / parameter
> equals
> > >> > > > 187 : E121 continuation line under-indented for hanging
> indent
> > >> > > > 288 : E502 the backslash is redundant between brackets
> > >> > > > 368 : E302 expected 2 blank lines, found 1
> > >> > > > 1295 : E128 continuation line under-indented for visual
> indent
> > >> > > >
> > >> > > > Maybe worth just disabling some of the indentation-related ones
> to
> > >> > start?
> > >> > > >
> > >> > > >
> > >> > > > On Mon, Jul 30, 2018 at 4:09 PM, Tim Armstrong <
> > >> > > > tarmstrong@cloudera.com.invalid> wrote:
> > >> > > >
> > >> > > > > I have a few updates.
> > >> > > > >
> > >> > > > > I added an automatic build job for docs changes:
> > >> > > > > https://jenkins.impala.io/job/gerrit-docs-auto-test/
> > >> > > > >
> > >> > > > > I'm going to disable the "Build started" message for
> > >> > > > > gerrit-code-review-checks. It seems a bit too chatty. Let me
> > know
> > >> if
> > >> > > you
> > >> > > > > disagree.
> > >> > > > >
> > >> > > > > I'm adding a job that automatically does some checks on the
> diff
> > >> and
> > >> > > > posts
> > >> > > > > code review comments. I started off with Python flake8
> comments.
> > >> > > > >
> > >> > > > > Let me know if you see any problems or if it turns out to be
> too
> > >> > noisy.
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > > > > On Mon, Jul 23, 2018 at 11:55 AM, Tim Armstrong <
> > >> > > tarmstrong@cloudera.com
> > >> > > > >
> > >> > > > > wrote:
> > >> > > > >
> > >> > > > > > Hi All,
> > >> > > > > > I'm enabling an automatic precommit job for code reviews
> > >> uploaded
> > >> > > to
> > >> > > > > > gerrit that will run RAT, clang-tidy and a GCC debug
> > >> compilation.
> > >> > > This
> > >> > > > is
> > >> > > > > > to provide faster feedback on code reviews:
> > >> > > https://issues.apache.org/
> > >> > > > > > jira/browse/IMPALA-7317 . I'll add some more checks but I'm
> > >> wanting
> > >> > > to
> > >> > > > > > test the basic mechanism for a bit now.
> > >> > > > > >
> > >> > > > > > It excludes docs/ commits.
> > >> > > > > >
> > >> > > > > > Let me know if you see any problems with it.
> > >> > > > > >
> > >> > > > > > Thanks,
> > >> > > > > > Tim
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > > --
> > >> > > > Todd Lipcon
> > >> > > > Software Engineer, Cloudera
> > >> > > >
> > >> > >
> > >> >
> > >> >
> > >> >
> > >> > --
> > >> > Todd Lipcon
> > >> > Software Engineer, Cloudera
> > >> >
> > >>
> > >
> > >
> >
>
Re: Enabling automatic code review precommit job
Posted by Zoltan Borok-Nagy <bo...@cloudera.com>.
Hi,
I might have ran into a bug of gerrit-code-review-checks with this change:
https://gerrit.cloudera.org/#/c/11160/
The job failed for PS 7, because it didn't contain
bin/jenkins/build-only.sh.
So I rebased and uploaded PS 8, but that didn't trigger
gerrit-code-review-checks. So I went to jenkins and hit "Retrigger" on the
failed job, but it executed the job against PS 7, not PS 8.
And when I look at "Build with parameters", I can only see the
IMPALA_REPO_URL and IMPALA_LZO_BRANCH parameters, so I cannot really run
this job against the my newest patch set.
Anyway the change is already +2ed and I could run gerrit-verify-dryrun
which almost certainly do these checks as well, but if it didn't I don't
want to commit something that might break gerrit-core-review-checks for
subsequent changes.
Thanks,
Zoltan
On Wed, Aug 1, 2018 at 10:56 PM Tim Armstrong
<ta...@cloudera.com.invalid> wrote:
> The flake8 comments should now line up correctly with your diff. I got more
> feedback that some of the comments were a little strict (and it's unclear
> which ones will be enforced) so I posted a diff to disable some of the
> checks:
>
> https://gerrit.cloudera.org/#/c/11102/
>
> On Tue, Jul 31, 2018 at 10:00 AM, Tim Armstrong <ta...@cloudera.com>
> wrote:
>
> > Todd pointed out a bug where it was posting flake8 comments that didn't
> > align with the diff. I figured out the issue but will keep the job in a
> > silent mode for a bit while I monitor it.
> >
> > On Tue, Jul 31, 2018 at 9:16 AM, Jim Apple <jbapple@cloudera.com.invalid
> >
> > wrote:
> >
> >> This is from my .emacs. The way it works is that you select a region and
> >> then M-x pep8-region.
> >>
> >> (defun pep8-region (&optional b e)
> >> (interactive "r")
> >> (call-process-region b e
> >> "/home/jbapple/.local/bin/autopep8" t t nil
> >> "--indent-size=2" "--max-line-length=90" "-a"
> "-a"
> >> "-a" "-a" "-a" "-a" "-a" "-a" "--experimental"
> >> "-"))
> >>
> >> It's . . . not great. For instance, it doesn't understand that if you're
> >> already 4 spaces in at the start of the region, you don't want to revert
> >> back to 0 or 2 spaces in. That said, it can still be helpful. I don't
> >> think
> >> this is as sophisticated as clang-format.el.
> >>
> >> On Tue, Jul 31, 2018 at 9:09 AM, Todd Lipcon <todd@cloudera.com.invalid
> >
> >> wrote:
> >>
> >> > On Tue, Jul 31, 2018 at 9:02 AM, Tim Armstrong <
> >> > tarmstrong@cloudera.com.invalid> wrote:
> >> >
> >> > > I agree that it might be a little strict at the moment and disallow
> >> some
> >> > > reasonable formatting. The tool is controlled by the setup.cfg file
> in
> >> > the
> >> > > repo so it's easy enough to change the behaviour.
> >> > >
> >> > > I think we have been a little sloppy with Python style in general
> so I
> >> > > think some of these would be good to change over time. I think the
> >> main
> >> > > thing I'd like is to align the tool's behaviour with code reviews -
> if
> >> > > we're going to be strict about PEP 8 compliance in code reviews we
> >> should
> >> > > keep the tool strict.
> >> > >
> >> > > > 109 : E125 continuation line with same indent as next logical
> >> line
> >> > > I think this formatting is hard to read and confusing, I'm in favour
> >> of
> >> > > leaving this enabled.
> >> > >
> >> > > > 110 : E701 multiple statements on one line (colon)
> >> > > This if because of the one-line if statements we have in the code. I
> >> > don't
> >> > > feel strongly either way as long as we're consistent.
> >> > >
> >> > > > 129 : E231 missing whitespace after ','
> >> > > > 185 : E251 unexpected spaces around keyword / parameter equals
> >> > > > 288 : E502 the backslash is redundant between brackets
> >> > > These seems like sloppy/inconsistent formatting to me, I'm in favour
> >> of
> >> > > keeping these enabled and fixing existing code as we go.
> >> > >
> >> > > > 368 : E302 expected 2 blank lines, found 1
> >> > > Our Python code is very inconsistent here, would be nice to make it
> >> more
> >> > > consistent. I'm in favour of keeping it enabled and fixing as we go.
> >> > >
> >> > > > 187 : E121 continuation line under-indented for hanging indent
> >> > > > 1295 : E128 continuation line under-indented for visual indent
> >> > > On one hand it would be nice to follow the PEP 8 style here (
> >> > > https://www.python.org/dev/peps/pep-0008/#indentation) but the
> other
> >> > > idioms
> >> > > seem fine to me. I've been asked on Python code reviews to switch to
> >> the
> >> > > PEP 8 indentation style before so I think we should align the tools
> >> > > behaviour with what we're going to ask for code reviews.
> >> > >
> >> >
> >> >
> >> > Alright, I agree with all of the above. One suggestion, though: is it
> >> > possible to get something like 'autopep8' to run in a 'patch
> formatting'
> >> > mode where it only reformats changed lines? Then we could more easily
> >> just
> >> > run a single command to ensure that our patches are properly formatted
> >> > before submitting to review. Or, at the very least, some instructions
> >> for
> >> > running the same flake8-against-only-my-changed-lines that gerrit is
> >> > running?
> >> >
> >> > -Todd
> >> >
> >> >
> >> > >
> >> > >
> >> > > On Mon, Jul 30, 2018 at 7:48 PM, Todd Lipcon
> >> <to...@cloudera.com.invalid>
> >> > > wrote:
> >> > >
> >> > > > It seems like flake8 might need some adjustment of its policies.
> >> Here
> >> > are
> >> > > > the most common issues in the current test code:
> >> > > >
> >> > > > 109 : E125 continuation line with same indent as next logical
> >> line
> >> > > > 110 : E701 multiple statements on one line (colon)
> >> > > > 129 : E231 missing whitespace after ','
> >> > > > 185 : E251 unexpected spaces around keyword / parameter equals
> >> > > > 187 : E121 continuation line under-indented for hanging indent
> >> > > > 288 : E502 the backslash is redundant between brackets
> >> > > > 368 : E302 expected 2 blank lines, found 1
> >> > > > 1295 : E128 continuation line under-indented for visual indent
> >> > > >
> >> > > > Maybe worth just disabling some of the indentation-related ones to
> >> > start?
> >> > > >
> >> > > >
> >> > > > On Mon, Jul 30, 2018 at 4:09 PM, Tim Armstrong <
> >> > > > tarmstrong@cloudera.com.invalid> wrote:
> >> > > >
> >> > > > > I have a few updates.
> >> > > > >
> >> > > > > I added an automatic build job for docs changes:
> >> > > > > https://jenkins.impala.io/job/gerrit-docs-auto-test/
> >> > > > >
> >> > > > > I'm going to disable the "Build started" message for
> >> > > > > gerrit-code-review-checks. It seems a bit too chatty. Let me
> know
> >> if
> >> > > you
> >> > > > > disagree.
> >> > > > >
> >> > > > > I'm adding a job that automatically does some checks on the diff
> >> and
> >> > > > posts
> >> > > > > code review comments. I started off with Python flake8 comments.
> >> > > > >
> >> > > > > Let me know if you see any problems or if it turns out to be too
> >> > noisy.
> >> > > > >
> >> > > > >
> >> > > > >
> >> > > > > On Mon, Jul 23, 2018 at 11:55 AM, Tim Armstrong <
> >> > > tarmstrong@cloudera.com
> >> > > > >
> >> > > > > wrote:
> >> > > > >
> >> > > > > > Hi All,
> >> > > > > > I'm enabling an automatic precommit job for code reviews
> >> uploaded
> >> > > to
> >> > > > > > gerrit that will run RAT, clang-tidy and a GCC debug
> >> compilation.
> >> > > This
> >> > > > is
> >> > > > > > to provide faster feedback on code reviews:
> >> > > https://issues.apache.org/
> >> > > > > > jira/browse/IMPALA-7317 . I'll add some more checks but I'm
> >> wanting
> >> > > to
> >> > > > > > test the basic mechanism for a bit now.
> >> > > > > >
> >> > > > > > It excludes docs/ commits.
> >> > > > > >
> >> > > > > > Let me know if you see any problems with it.
> >> > > > > >
> >> > > > > > Thanks,
> >> > > > > > Tim
> >> > > > > >
> >> > > > >
> >> > > >
> >> > > >
> >> > > >
> >> > > > --
> >> > > > Todd Lipcon
> >> > > > Software Engineer, Cloudera
> >> > > >
> >> > >
> >> >
> >> >
> >> >
> >> > --
> >> > Todd Lipcon
> >> > Software Engineer, Cloudera
> >> >
> >>
> >
> >
>