Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 7 months ago by nodir Modified:
6 years, 7 months ago Reviewers:
kmg, Vadim Sh. CC:
chromium-reviews, kjellander-cc_chromium.org, cmp-cc_chromium.org, ilevy-cc_chromium.org, stip+watch_chromium.org Visibility:
Public. |
DescriptionMade Gerrit tryjob message format less restrictive
Any Gerrit message containing !tryjob is considered a try job. Anything
after !tryjob till the EOL/EOF is considered job definition and should
be in JSON.
R=kmg@chromium.org, vadimsh@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=266996
Patch Set 1 #
Total comments: 2
Patch Set 2 : Any message may have !tryjob #
Created: 6 years, 7 months ago
Messages
Total messages: 10 (0 generated)
https://codereview.chromium.org/259323003/diff/1/scripts/master/try_job_gerri... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/259323003/diff/1/scripts/master/try_job_gerri... scripts/master/try_job_gerrit.py:58: MESSAGE_REGEX_TRYJOB = re.compile('Patch set \d+:( Tryjob\+1)?\s+!tryjob(.*)', Take care of Code-Review and Verified labels too then, since they also can be set in same message. Or just skip everything until you see !tryjob.
https://codereview.chromium.org/259323003/diff/1/scripts/master/try_job_gerri... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/259323003/diff/1/scripts/master/try_job_gerri... scripts/master/try_job_gerrit.py:58: MESSAGE_REGEX_TRYJOB = re.compile('Patch set \d+:( Tryjob\+1)?\s+!tryjob(.*)', On 2014/04/29 21:36:49, Vadim Sh. wrote: > Take care of Code-Review and Verified labels too then, since they also can be > set in same message. They shouldn't. > Or just skip everything until you see !tryjob. I thought of this. This allows anybody to submit a tryjob in any comment, like Hey John, here is my change. PTAL. I am going to try on Master: !tryjob ["Master"] But I'm not sure about this. What do you think?
On 2014/04/29 21:49:46, nodir wrote: > https://codereview.chromium.org/259323003/diff/1/scripts/master/try_job_gerri... > File scripts/master/try_job_gerrit.py (right): > > https://codereview.chromium.org/259323003/diff/1/scripts/master/try_job_gerri... > scripts/master/try_job_gerrit.py:58: MESSAGE_REGEX_TRYJOB = re.compile('Patch > set \d+:( Tryjob\+1)?\s+!tryjob(.*)', > On 2014/04/29 21:36:49, Vadim Sh. wrote: > > Take care of Code-Review and Verified labels too then, since they also can be > > set in same message. > > They shouldn't. > Why? What prevents me from setting Code-Review +1 and TryJob +1 in a same message? > > Or just skip everything until you see !tryjob. > > I thought of this. This allows anybody to submit a tryjob in any comment, like > > Hey John, here is my change. PTAL. I am going to try on Master: !tryjob > ["Master"] > > But I'm not sure about this. What do you think? What prevents you from "faking" TryJob +1 message? It's all nasty text parsing.
On 2014/04/29 21:49:46, nodir wrote: > https://codereview.chromium.org/259323003/diff/1/scripts/master/try_job_gerri... > File scripts/master/try_job_gerrit.py (right): > > https://codereview.chromium.org/259323003/diff/1/scripts/master/try_job_gerri... > scripts/master/try_job_gerrit.py:58: MESSAGE_REGEX_TRYJOB = re.compile('Patch > set \d+:( Tryjob\+1)?\s+!tryjob(.*)', > On 2014/04/29 21:36:49, Vadim Sh. wrote: > > Take care of Code-Review and Verified labels too then, since they also can be > > set in same message. > > They shouldn't. Clarification: git-try will set these labels, but I agree checking for Verified, etc is more general. This is the question of how strict this check should be. > > Or just skip everything until you see !tryjob. > > I thought of this. This allows anybody to submit a tryjob in any comment, like > > Hey John, here is my change. PTAL. I am going to try on Master: !tryjob > ["Master"] > > But I'm not sure about this. What do you think? Actually this is not entirely true. Currently TryJobGerritPoller expects the tryjob definition be form !tryjob to EOF. We could check if !tryjob is followed with a parenthesis and find a matching one, but this is more complicated than it should be
PTAL. Updated description: Made Gerrit tryjob message format less restrictive Any Gerrit message containing !tryjob is considered a try job. Anything after !tryjob till the EOL/EOF is considered job definition and should be in JSON.
lgtm
The CQ bit was checked by nodir@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nodir@chromium.org/259323003/10002
Message was sent while issue was closed.
Change committed as 266996 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
