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 <borokna...@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 <tarmstr...@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 < > > borokna...@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 > > > <tarmstr...@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 < > > tarmstr...@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 > > > <jbap...@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 > > > <t...@cloudera.com.invalid > > > > > > > > > >> wrote: > > > > >> > > > > >> > On Tue, Jul 31, 2018 at 9:02 AM, Tim Armstrong < > > > > >> > tarmstr...@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 > > > > >> <t...@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 < > > > > >> > > > tarmstr...@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 < > > > > >> > > tarmstr...@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 > > > > >> > > > > > >> > > > > > > > > > > > > > > > > > > > >
