check_reviewers can't deal with list of TBR emails |
|
Issue descriptionIf I TBR multiple people (eg TBR: a@chromium.org, b@chromium.org), depot_tools fails while doing git cl upload: File "/usr/local/google/home/mattcary/lib/depot_tools/owners.py", line 188, in _check_reviewers assert all(self.email_regexp.match(r) for r in reviewers), reviewers AssertionError: set(['mattcary@chromium.org', 'twellington@chromium.org, mdjones@chromium.org']) I think that email_regexp is too fussy and expects only single email, not a list.
,
Dec 4 2017
I'm not 100% sure the following is the original commit message, but it's been failing consistently enough that I'm pretty sure this will reproduce the issue (it's coming up a lot while sheriffing and it really slows down my flow disabling tests ;) Take the text between >>> and <<<: >>> Disable flaking WebRtcEventLogManagerTest.LocalLogMayNotBeStartedTwice Bug: 787809 TBR: eladalon@chromium.org, guidou@chromium.org <<< More precisely, the above is the text of my local git commit, and then I run the following command and get this full stack trace (of which the error mentioned above is the last line): mattcary@mattcary:/clank/sheriff/src$ git cl upload Running presubmit upload checks ... Traceback (most recent call last): File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 6193, in <module> sys.exit(main(sys.argv[1:])) File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 6175, in main return dispatcher.execute(OptionParser(), argv) File "/usr/local/google/home/mattcary/lib/depot_tools/subcommand.py", line 252, in execute return command(parser, args[1:]) File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 5036, in CMDupload return cl.CMDUpload(options, args, orig_args) File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 1642, in CMDUpload change=change) File "/usr/local/google/home/mattcary/lib/depot_tools/git_cl.py", line 1574, in RunHook gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit()) File "/usr/local/google/home/mattcary/lib/depot_tools/presubmit_support.py", line 1412, in DoPresubmitChecks results += executer.ExecPresubmitScript(presubmit_script, filename) File "/usr/local/google/home/mattcary/lib/depot_tools/presubmit_support.py", line 1319, in ExecPresubmitScript result = eval(function_name + '(*__args)', context) File "<string>", line 1, in <module> File "<string>", line 2764, in CheckChangeOnUpload File "<string>", line 2480, in _CommonChecks File "/usr/local/google/home/mattcary/lib/depot_tools/presubmit_canned_checks.py", line 1161, in PanProjectChecks input_api, output_api, source_file_filter=None)) File "/usr/local/google/home/mattcary/lib/depot_tools/presubmit_canned_checks.py", line 974, in CheckOwners reviewers_plus_owner) File "/usr/local/google/home/mattcary/lib/depot_tools/owners.py", line 174, in files_not_covered_by self._check_reviewers(reviewers) File "/usr/local/google/home/mattcary/lib/depot_tools/owners.py", line 188, in _check_reviewers assert all(self.email_regexp.match(r) for r in reviewers), reviewers AssertionError: set(['mattcary@chromium.org', 'eladalon@chromium.org, guidou@chromium.org'])
,
Dec 4 2017
Ah. Hmm, no tooling should be producing commit messages which look like that. This is clearly a bad thing and I should work on improving this interaction but for now, you can get around this in one of two ways: 1) Format the commit message like this: """ Disable flaking SomeSuite.ThatOneAnnoyingTest TBR=someone@foo.tld, someone@bar.tld Bug: 123456 """ or 2) Don't put the TBR in the commit message at all, and upload the change with "git cl upload --tbrs=someone@foo.tld,someone@bar.tld".
,
Dec 5 2017
No tooling created that, I wrote the commit message myself. Ah, so TBR uses "=", but Bug uses ":" now? With rietveld, Bug used "=" but I noticed it switched to ":" with gerrit. I assumed the other annotations used a colon as well. IMHO, I shouldn't have to think too hard about what punctuation to use. It's not too hard to make the parsing intelligent; I'd be happy to make the changes to depot_tools if you all our resource constrainted.
,
Dec 5 2017
You're absolutely right, which is why I said I need to work on fixing this and didn't close the bug :) I do suggest using "--tbrs=foo,bar" in the mean time, however.
,
Dec 5 2017
(And unfortunately, it *is* hard to make the parsing cleaner in a sane way. :()
,
Dec 6 2017
Sorry! I misread your comment. Thank you for staying on the bug, it's very much appreciated, creating reliable input processing is very fiddly. |
|
►
Sign in to add a comment |
|
Comment 1 by aga...@chromium.org
, Dec 4 2017