trychange.py: Gerrit protocol

trychange.py: Gerrit protocol

This change implements Gerrit protocol for posting try jobs. The
"protocol" is described here:
https://code.google.com/p/chromium/issues/detail?id=366099#c10

CLs for Buildbot:
https://codereview.chromium.org/250983003/
https://codereview.chromium.org/259323003/

R=vadimsh@chromium.org, maruel@chromium.org, kmg@chromium.org
BUG=366099, 364639, 364631
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=267379

[Vadim Sh., 2014-04-30 00:40:47]

https://codereview.chromium.org/254133007/diff/20001/trychange.py
File trychange.py (right):

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode735
trychange.py:735: def get_change_id():
looks like this file is using GetChangeId naming style

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode741
trychange.py:741: raise Error('Change-Id was not found in the HEAD commit.')
It would be helpful to explain what to do in that case. For example explain that
this try mechanism works only Gerrit projects that have commit hooks in place.

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode743
trychange.py:743: assert change_id and change_id.startswith('I')
I think this assert is redundant since your regexp is pretty strict already.

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode756
trychange.py:756: msg = '%s %s' % (msg, json.dumps(job_def))
json.dumps(job_def, sort_keys=True)

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode757
trychange.py:757: return msg
Does Gerrit append '\n' for you here? I think it's better to explicitly append
it, since your regexp on buildbot side expects it.

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode761
trychange.py:761: change_id = get_change_id()
I think it would be cleaner to pass |head_sha| to get_change_id() and extract
Change-Id from |head_sha| commit, rather then requery the history again.

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode762
trychange.py:762: assert change_id
I think this is redundant too..

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode766
trychange.py:766: assert len(changes) in (0, 1), 'Multiple changes with id %s' %
change_id
hm.. len(changes) <= 1?

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode772
trychange.py:772: raise Error('Some changes were made since last upload to
Gerrit, '
I'd rephrase it somehow. Maybe "Please reupload your latest changes to Gerrit"
or something like that. English is hard :(

You should also probably check that index and working directory are clean? I.e.
both 'git diff' and 'git diff --cached' return empty strings.

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode779
trychange.py:779: # Post a message and set TryJob=1 label
nit: append '.' at the end of a sentence :)

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode781
trychange.py:781: labels={'Tryjob': 1})
What happens here if project doesn't have Tryjob label configured?

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode1124
trychange.py:1124: can_anything = can_http or can_svn or can_git or can_gerrit
I'm far from english expert, but shouldn't it be "can_something" ? :)

[nodir, 2014-04-30 18:57:27]

Addressed your comments. Please, take a look

https://codereview.chromium.org/254133007/diff/20001/trychange.py
File trychange.py (right):

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode757
trychange.py:757: return msg
On 2014/04/30 00:40:48, Vadim Sh. wrote:
> Does Gerrit append '\n' for you here? I think it's better to explicitly append
> it, since your regexp on buildbot side expects it.

The regexp expects EOL or EOF  after job definition. I've tested that:

re.search('!tryjob(.*)$', 'aaa\n!tryjob xxx', re.I).group() == '!tryjob xxx'

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode761
trychange.py:761: change_id = get_change_id()
On 2014/04/30 00:40:48, Vadim Sh. wrote:
> I think it would be cleaner to pass |head_sha| to get_change_id() and extract
> Change-Id from |head_sha| commit, rather then requery the history again.

Done.

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode766
trychange.py:766: assert len(changes) in (0, 1), 'Multiple changes with id %s' %
change_id
On 2014/04/30 00:40:48, Vadim Sh. wrote:
> hm.. len(changes) <= 1?

Wanted to be more explicit, it's probably looks weird

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode772
trychange.py:772: raise Error('Some changes were made since last upload to
Gerrit, '
On 2014/04/30 00:40:48, Vadim Sh. wrote:
> I'd rephrase it somehow. Maybe "Please reupload your latest changes to Gerrit"
> or something like that. English is hard :(

Modified the string a bit.

> You should also probably check that index and working directory are clean?
I.e.
> both 'git diff' and 'git diff --cached' return empty strings.

This is done by other part of trychange.py

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode781
trychange.py:781: labels={'Tryjob': 1})
On 2014/04/30 00:40:48, Vadim Sh. wrote:
> What happens here if project doesn't have Tryjob label configured?

It will report:

Bad Request: label "Tryjb" is not a configured label

https://codereview.chromium.org/254133007/diff/20001/trychange.py#newcode1124
trychange.py:1124: can_anything = can_http or can_svn or can_git or can_gerrit
On 2014/04/30 00:40:48, Vadim Sh. wrote:
> I'm far from english expert, but shouldn't it be "can_something" ? :)

Done

[Vadim Sh., 2014-04-30 22:06:26]

https://codereview.chromium.org/254133007/diff/40001/trychange.py
File trychange.py (right):

https://codereview.chromium.org/254133007/diff/40001/trychange.py#newcode739
trychange.py:739: def getChangeId(commmitish):
mm.. GetChangeId, not getChangeId.

[nodir, 2014-04-30 22:43:39]

Done

https://codereview.chromium.org/254133007/diff/40001/trychange.py
File trychange.py (right):

https://codereview.chromium.org/254133007/diff/40001/trychange.py#newcode739
trychange.py:739: def getChangeId(commmitish):
On 2014/04/30 22:06:26, Vadim Sh. wrote:
> mm.. GetChangeId, not getChangeId.

Done.

[Vadim Sh., 2014-04-30 22:45:40]

https://codereview.chromium.org/254133007/diff/60001/trychange.py
File trychange.py (right):

https://codereview.chromium.org/254133007/diff/60001/trychange.py#newcode765
trychange.py:765: def postTryjob(message):
missed that one :)

[nodir, 2014-04-30 22:49:06]

Done

[Vadim Sh., 2014-04-30 22:56:57]

lgtm

[nodir, 2014-04-30 22:59:02]

The CQ bit was checked by nodir@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:59:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nodir@chromium.org/254133007/80001

[commit-bot: I haz the power, 2014-04-30 23:01:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 23:01:02]

Presubmit check for 254133007-80001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    depot_tools/gerrit_util.py
    depot_tools/tests/trychange_unittest.py
    depot_tools/trychange.py

Presubmit checks took 92.3s to calculate.

[Vadim Sh., 2014-04-30 23:02:10]

On 2014/04/30 23:01:02, I haz the power (commit-bot) wrote:
> Presubmit check for 254133007-80001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> Checking out rietveld...
> Running save-description-on-failure.sh
> Running push-basic.sh
> Running upstream.sh
> Running submit-from-new-dir.sh
> Running abandon.sh
> Running submodule-merge-test.sh
> Running upload-local-tracking-branch.sh
> Running hooks.sh
> Running post-dcommit-hook-test.sh
> Running upload-stale.sh
> Running patch.sh
> Running basic.sh
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     depot_tools/gerrit_util.py
>     depot_tools/tests/trychange_unittest.py
>     depot_tools/trychange.py
> 
> Presubmit checks took 92.3s to calculate.

Ah, right. I'm not an OWNER for depot_tools (thankfully). So you still need to
bug M-A to review.

[M-A Ruel, 2014-04-30 23:13:23]

lgtm

[nodir, 2014-04-30 23:38:42]

The CQ bit was checked by nodir@chromium.org

[commit-bot: I haz the power, 2014-04-30 23:39:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nodir@chromium.org/254133007/80001

[commit-bot: I haz the power, 2014-04-30 23:40:56]

Change committed as 267379