Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1063103002:
git cl try uses buildbucket via OAuth2 (Closed)
Descriptiongit cl try uses buildbucket via OAuth2
TEST=local run on Linux desktop and Macbook
NOPRESUBMIT=true(due to https://code.google.com/p/chromium/issues/detail?id=487172)
BUG=461614
R=maruel@chromium.org, nodir@chromium.org, vadimsh@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=295236
Patch Set 1 #
Total comments: 17
Patch Set 2 : use new auth module #Patch Set 3 : fix merging #Patch Set 4 : upgrade 3rd party package and fix imports #Patch Set 5 : fix imports #Patch Set 6 : fix #
Total comments: 22
Patch Set 7 : address comments #Patch Set 8 : use rietveld url from settings #Patch Set 9 : rebase #
Total comments: 2
Patch Set 10 : rebase #Patch Set 11 : use put_batch #Patch Set 12 : remove debugging #
Total comments: 10
Patch Set 13 : nits fix #Patch Set 14 : remove use_oauth2 #Patch Set 15 : add option use_buildbucket #Patch Set 16 : rebase #
Total comments: 30
Patch Set 17 : address comments #
Total comments: 12
Patch Set 18 : address more comments #
Total comments: 10
Patch Set 19 : address comments #Messages
Total messages: 54 (13 generated)
sheyang@chromium.org changed reviewers: + nodir@chromium.org, vadimsh@chromium.org
https://codereview.chromium.org/1063103002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode41 git_cl.py:41: from third_party.google_api_python_client import apiclient it is not vendored properly: Traceback (most recent call last): File "/Users/vadimsh/depot_tools/git_cl.py", line 36, in <module> from third_party import upload File "/Users/vadimsh/depot_tools/third_party/upload.py", line 75, in <module> import auth File "/Users/vadimsh/depot_tools/auth.py", line 26, in <module> from third_party.google_api_python_client import apiclient File "/Users/vadimsh/depot_tools/third_party/google_api_python_client/apiclient/__init__.py", line 3, in <module> import googleapiclient ImportError: No module named googleapiclient I suspect you have it installed in your global site-packages and it is imported from there. I personally think apiclient.discovery is not worth the trouble of adding apiclient dependency (e.g. it is simpler to use httplib2 directly to send HTTP requests to JSON endpoints). But as you prefer. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode48 git_cl.py:48: import oauth2 this file has been replaced by auth.py in https://codereview.chromium.org/1074673002/ (will commit it tomorrow). auth.py is used by "git cl upload" and all other depot_tools script. See comment below for how to use it for accessing buildbucket. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode74 git_cl.py:74: DEFAULT_SCOPES = 'email' not used https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode239 git_cl.py:239: def _get_buildbucket(credentials): with auth.py it would be: def _get_buildbucket(rietveld_url, auth_config): authenticator = auth.get_authenticator_for_host(rietveld_url, auth_config) http = httplib2.Http() authenticator.authorize(http) return ... auth_config is already extracted from command line options CMDtry. rietveld_url is changelist.GetRietveldServer(). https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode253 git_cl.py:253: requester = creds_props['id_token']['email'] this is currently not exposed via auth.py, though I can add it. But I think it is a wrong approach security-wise, see comment below. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode283 git_cl.py:283: 'requester': requester, the server should derive it from oauth credentials itself, or at least verify if corresponds to email associated with oauth token. Otherwise it is trivial to trick server and request jobs on somebody else's behalf. +nodir to answer this question.
https://codereview.chromium.org/1063103002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode283 git_cl.py:283: 'requester': requester, On 2015/04/10 02:24:29, Vadim Sh. wrote: > the server should derive it from oauth credentials itself, or at least verify if > corresponds to email associated with oauth token. Otherwise it is trivial to > trick server and request jobs on somebody else's behalf. +nodir to answer this > question. buidbucket does not read this property. It is interpreted by a recipe, that does not verify identity. Buildbot trusts buildbucket that its buckets are secure. BuildBucket trusts that authorized users won't put garbage to properties. requester is not always == created_by because CQ schedules builds on behalf of its users, so "requester" "enables" impersonation (in insecure way I guess)
https://codereview.chromium.org/1063103002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode41 git_cl.py:41: from third_party.google_api_python_client import apiclient On 2015/04/10 02:24:29, Vadim Sh. wrote: > it is not vendored properly: > > Traceback (most recent call last): > File "/Users/vadimsh/depot_tools/git_cl.py", line 36, in <module> > from third_party import upload > File "/Users/vadimsh/depot_tools/third_party/upload.py", line 75, in <module> > import auth > File "/Users/vadimsh/depot_tools/auth.py", line 26, in <module> > from third_party.google_api_python_client import apiclient > File > "/Users/vadimsh/depot_tools/third_party/google_api_python_client/apiclient/__init__.py", > line 3, in <module> > import googleapiclient > ImportError: No module named googleapiclient "import apiclient" should work, we use it in other places. If it doesn't PYTHONPATH needs to be updated https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode283 git_cl.py:283: 'requester': requester, > requester is not always == created_by because CQ schedules builds on behalf of > its users, so "requester" "enables" impersonation (in insecure way I guess) This part was wrong because CQ sets requester to "commit-bot@chromium.org", so requester should be read from buildbucket.build.created_by property. In order to not break the existing contract between buildbot and recipes, I think buildbot-buildbucket integration should copy created_by to requester property. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode292 git_cl.py:292: 'requester:%s' % requester] requester tag is not the user, but a tool. Maybe we should rename it to "requester_tool" https://cr-buildbucket.appspot.com/#docs/conventions CQ specifies "cq" here. git-cl-try should probably specify "git-cl-try". Rietveld will specify "rietveld"
PTAL. https://codereview.chromium.org/1063103002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode41 git_cl.py:41: from third_party.google_api_python_client import apiclient On 2015/04/13 16:04:53, nodir wrote: > On 2015/04/10 02:24:29, Vadim Sh. wrote: > > it is not vendored properly: > > > > Traceback (most recent call last): > > File "/Users/vadimsh/depot_tools/git_cl.py", line 36, in <module> > > from third_party import upload > > File "/Users/vadimsh/depot_tools/third_party/upload.py", line 75, in > <module> > > import auth > > File "/Users/vadimsh/depot_tools/auth.py", line 26, in <module> > > from third_party.google_api_python_client import apiclient > > File > > > "/Users/vadimsh/depot_tools/third_party/google_api_python_client/apiclient/__init__.py", > > line 3, in <module> > > import googleapiclient > > ImportError: No module named googleapiclient > > "import apiclient" should work, we use it in other places. If it doesn't > PYTHONPATH needs to be updated Per discussion with Nodir@ I added the directory to syspath to fix it. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode48 git_cl.py:48: import oauth2 On 2015/04/10 02:24:29, Vadim Sh. wrote: > this file has been replaced by auth.py in > https://codereview.chromium.org/1074673002/ (will commit it tomorrow). > > auth.py is used by "git cl upload" and all other depot_tools script. See comment > below for how to use it for accessing buildbucket. Done. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode74 git_cl.py:74: DEFAULT_SCOPES = 'email' On 2015/04/10 02:24:29, Vadim Sh. wrote: > not used Removed. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode239 git_cl.py:239: def _get_buildbucket(credentials): On 2015/04/10 02:24:29, Vadim Sh. wrote: > with auth.py it would be: > > def _get_buildbucket(rietveld_url, auth_config): > authenticator = auth.get_authenticator_for_host(rietveld_url, auth_config) > http = httplib2.Http() > authenticator.authorize(http) > return ... > > auth_config is already extracted from command line options CMDtry. rietveld_url > is changelist.GetRietveldServer(). Done. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode253 git_cl.py:253: requester = creds_props['id_token']['email'] On 2015/04/10 02:24:29, Vadim Sh. wrote: > this is currently not exposed via auth.py, though I can add it. But I think it > is a wrong approach security-wise, see comment below. This field will be populated from oauth2 credential. Replaced it with user_agent and the value is 'git+cl_try'. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode283 git_cl.py:283: 'requester': requester, On 2015/04/13 16:04:53, nodir wrote: > > requester is not always == created_by because CQ schedules builds on behalf of > > its users, so "requester" "enables" impersonation (in insecure way I guess) > > This part was wrong because CQ sets requester to mailto:"commit-bot@chromium.org", so > requester should be read from buildbucket.build.created_by property. In order to > not break the existing contract between buildbot and recipes, I think > buildbot-buildbucket integration should copy created_by to requester property. Acknowledged. https://codereview.chromium.org/1063103002/diff/1/git_cl.py#newcode292 git_cl.py:292: 'requester:%s' % requester] On 2015/04/13 16:04:53, nodir wrote: > requester tag is not the user, but a tool. Maybe we should rename it to > "requester_tool" > https://cr-buildbucket.appspot.com/#docs/conventions > CQ specifies "cq" here. git-cl-try should probably specify "git-cl-try". > Rietveld will specify "rietveld" Use 'user_agent' instead.
https://codereview.chromium.org/1063103002/diff/100001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode77 git_cl.py:77: '{api}/{apiVersion}/rest') Rename to BUILDBUCKET_DISCOVERY_URL https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode240 git_cl.py:240: rietveld_host = urlparse.urlparse(changelist.GetRietveldServer()).hostname We will need to make sure that all branches on all machines of all chromium devs have codereview.chromium.org value for `git config branch.<branch>.rietveldserver` or `git config rietveld.server`. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode248 git_cl.py:248: patch=str(patchset)) You don't need str calls here https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode249 git_cl.py:249: print 'Tried jobs on:' "Trying", as they were not tried yet https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode250 git_cl.py:250: for (master, builders_and_tests) in masters.iteritems(): Parens are unnecessary https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode254 git_cl.py:254: req = buildbucket.put(body={ In https://codereview.chromium.org/1058893004/ esprehn@ said we should send just one request to schedule many builds. I will add a new endpoint "put_batch" that schedules many builds https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode271 git_cl.py:271: 'rietveld': changelist.GetRietveldServer(), why not rietveld_host ? https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode294 git_cl.py:294: if status >= 500: Inverse this condition so you can unindent the ">=500" part https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode300 git_cl.py:300: raise Move these two lines before logging about transient errors and waiting https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode2983 git_cl.py:2983: e, stacktrace) doesn't logging.exception do the same with current exception info (with trace) for you?
Addressed comments. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode77 git_cl.py:77: '{api}/{apiVersion}/rest') On 2015/04/14 16:12:48, nodir wrote: > Rename to BUILDBUCKET_DISCOVERY_URL Done. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode240 git_cl.py:240: rietveld_host = urlparse.urlparse(changelist.GetRietveldServer()).hostname On 2015/04/14 16:12:48, nodir wrote: > We will need to make sure that all branches on all machines of all chromium devs > have http://codereview.chromium.org value for `git config > branch.<branch>.rietveldserver` or `git config rietveld.server`. As a fallback, rietveld_server will be fetched from codereview.settings, which should always be available. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode248 git_cl.py:248: patch=str(patchset)) On 2015/04/14 16:12:48, nodir wrote: > You don't need str calls here Done. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode249 git_cl.py:249: print 'Tried jobs on:' On 2015/04/14 16:12:48, nodir wrote: > "Trying", as they were not tried yet This is from the old code... Updated. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode250 git_cl.py:250: for (master, builders_and_tests) in masters.iteritems(): On 2015/04/14 16:12:48, nodir wrote: > Parens are unnecessary Done. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode254 git_cl.py:254: req = buildbucket.put(body={ On 2015/04/14 16:12:48, nodir wrote: > In https://codereview.chromium.org/1058893004/ esprehn@ said we should send just > one request to schedule many builds. I will add a new endpoint "put_batch" that > schedules many builds > Acknowledged. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode271 git_cl.py:271: 'rietveld': changelist.GetRietveldServer(), On 2015/04/14 16:12:48, nodir wrote: > why not rietveld_host ? I think it needs 'https' prefix... adding rietveld_url. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode294 git_cl.py:294: if status >= 500: On 2015/04/14 16:12:48, nodir wrote: > Inverse this condition so you can unindent the ">=500" part Done. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode294 git_cl.py:294: if status >= 500: On 2015/04/14 16:12:48, nodir wrote: > Inverse this condition so you can unindent the ">=500" part Done. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode300 git_cl.py:300: raise On 2015/04/14 16:12:48, nodir wrote: > Move these two lines before logging about transient errors and waiting Done. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode300 git_cl.py:300: raise On 2015/04/14 16:12:48, nodir wrote: > Move these two lines before logging about transient errors and waiting Done. https://codereview.chromium.org/1063103002/diff/100001/git_cl.py#newcode2983 git_cl.py:2983: e, stacktrace) On 2015/04/14 16:12:48, nodir wrote: > doesn't logging.exception do the same with current exception info (with trace) > for you? logging.exception only has traceback.format_exc().
Please extract third_part changes to a separate CL
Some changes: - use httplib2 instead of apiclient - use put_batch - remove requester, and use user_agent - rietveld url is from settings only, rather than local branch
nodir@chromium.org changed reviewers: + agable@chromium.org, maruel@chromium.org
+maruel and +agable for review. This is a serious change. https://codereview.chromium.org/1063103002/diff/160001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/160001/git_cl.py#newcode250 git_cl.py:250: print 'Tring jobs on:' Typo: trying https://codereview.chromium.org/1063103002/diff/160001/git_cl.py#newcode255 git_cl.py:255: req = buildbucket.put(body={ You might want to update this to use put_batch API https://codereview.chromium.org/1063103002/diff/220001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode221 git_cl.py:221: def trigger_distributed_try_jobs( I'd call something like "trigger_buildbucket_builds". "distributed" does not make sense to me in the new context https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode245 git_cl.py:245: { nit: I think it is fine to merge lines 244 and 245, and unindent keys and values https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode254 git_cl.py:254: 'clobber': options.clobber, As smut said in https://code.google.com/p/chromium/issues/detail?id=478354, the existing contract is "clobber build if 'clobber' property is present, regardless of the value" https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode279 git_cl.py:279: "PUT", use ' https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode281 git_cl.py:281: headers={'Content-type': 'application/json'}, nit: capital t: Content-Type https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode289 git_cl.py:289: # Buildbbucket could return an error even status==200. "even if" https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode289 git_cl.py:289: # Buildbbucket could return an error even status==200. typo: double b https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode309 git_cl.py:309: print '\n'.join(print_text) Move this before the loop. Otherwise it says "trying" after it already tried. If there is an exception, this will never be printed https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode2982 git_cl.py:2982: e, stacktrace) I think logging.exception('ERROR: Exception when trying to trigger tryjobs') will you give you the same without importing traceback. It prints current exception info
ignore comments for patchset9, they are old
https://codereview.chromium.org/1063103002/diff/220001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/220001/git_cl.py#newcode269 git_cl.py:269: 'user_agent:git-cl-try'] please sort
Major changes: Only set clobber if it's true.
I have only one suggestion: make this feature hidden behind a flag at first and enable it only when it is verified to work reliably. Or at least preserve previous behavior behind a flag. So that if buildbucket is misbehaving, users have some workaround.
On 2015/04/20 19:35:20, Vadim Sh. wrote: > I have only one suggestion: make this feature hidden behind a flag at first and > enable it only when it is verified to work reliably. > > Or at least preserve previous behavior behind a flag. So that if buildbucket is > misbehaving, users have some workaround. Good idea for safe transition. Will make it a hidden feature first.
Add option use_buildbucket.
lgtm, but I'm not an owner, and I do not know buildbucket related stuff https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode308 git_cl.py:308: time.sleep(wait) nit: no need to wait if it was the last attempt (i.e. try_count == 0).
https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode70 git_cl.py:70: 'https://cr-buildbucket.appspot.com/_ah/api/buildbucket/v1/builds/batch') That URL shouldn't be hardcoded there.
lgtm, please send a PSA, tell about --use_buildbucket=False option https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode70 git_cl.py:70: 'https://cr-buildbucket.appspot.com/_ah/api/buildbucket/v1/builds/batch') On 2015/05/11 22:21:28, M-A Ruel wrote: > That URL shouldn't be hardcoded there. I think hardcoding the default hostname is fine, but making it configurable (read from codereview.settings or something) would be good too
The CQ bit was checked by sheyang@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
COMMIT=false detected. CQ is abandoning the patch.
The CQ bit was checked by sheyang@chromium.org
https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode70 git_cl.py:70: 'https://cr-buildbucket.appspot.com/_ah/api/buildbucket/v1/builds/batch') On 2015/05/11 22:48:29, nodir wrote: > On 2015/05/11 22:21:28, M-A Ruel wrote: > > That URL shouldn't be hardcoded there. > > I think hardcoding the default hostname is fine, but making it configurable > (read from codereview.settings or something) would be good too Not lgtm. It's not optional nor nice to have. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode281 git_cl.py:281: 'PUT', If I understand correctly, the sole reason http is being pulled in is to allow PUT request?
The CQ bit was unchecked by maruel@chromium.org
When people are not responsive to code review, please ping repeatedly. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode238 git_cl.py:238: print_text.append('Tried jobs on:') Why not printing as it's happening? Why hang output until everything is complete? It feels much less interactive. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode239 git_cl.py:239: for master, builders_and_tests in masters.iteritems(): Please sort. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode278 git_cl.py:278: try_count -= 1 try_count-- https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode308 git_cl.py:308: time.sleep(wait) On 2015/05/11 22:14:01, Vadim Sh. wrote: > nit: no need to wait if it was the last attempt (i.e. try_count == 0). Please address this.
https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode278 git_cl.py:278: try_count -= 1 On 2015/05/11 23:13:59, M-A Ruel wrote: > try_count-- this is invalid Python syntax https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode281 git_cl.py:281: 'PUT', On 2015/05/11 23:01:27, M-A Ruel wrote: > If I understand correctly, the sole reason http is being pulled in is to allow > PUT request? to authenticate the request.
https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode278 git_cl.py:278: try_count -= 1 On 2015/05/11 23:19:36, nodir wrote: > On 2015/05/11 23:13:59, M-A Ruel wrote: > > try_count-- > > this is invalid Python syntax Eh, damn python.
https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode69 git_cl.py:69: BUILDBUCKET_PUT_URL = ( Please only code the host here. Host != API, so that it's shouldn't be an endpoint here, same for BUILDSET_STR, which is not worth making a constant. So BUILDBUCKET_HOST = 'cr-buildbucket.appspot.com' then create the URL in the function below. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode213 git_cl.py:213: def _prefix_master(master): Why? https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode241 git_cl.py:241: bucket = _prefix_master(master) This looks like black magic, explain the context. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode242 git_cl.py:242: for builder, tests in builders_and_tests.iteritems(): sort this too. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode267 git_cl.py:267: 'parameters_json': json.dumps(parameters), What. You are encoding json inside json? Why? Because it's passed without understanding to buildbot?
PTAL. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode69 git_cl.py:69: BUILDBUCKET_PUT_URL = ( On 2015/05/12 00:46:21, M-A Ruel wrote: > Please only code the host here. Host != API, so that it's shouldn't be an > endpoint here, same for BUILDSET_STR, which is not worth making a constant. > > So > > BUILDBUCKET_HOST = 'cr-buildbucket.appspot.com' > > then create the URL in the function below. Done. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode70 git_cl.py:70: 'https://cr-buildbucket.appspot.com/_ah/api/buildbucket/v1/builds/batch') On 2015/05/11 23:01:27, M-A Ruel wrote: > On 2015/05/11 22:48:29, nodir wrote: > > On 2015/05/11 22:21:28, M-A Ruel wrote: > > > That URL shouldn't be hardcoded there. > > > > I think hardcoding the default hostname is fine, but making it configurable > > (read from codereview.settings or something) would be good too > > Not lgtm. > > It's not optional nor nice to have. Done. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode213 git_cl.py:213: def _prefix_master(master): On 2015/05/12 00:46:21, M-A Ruel wrote: > Why? See the comments about 'black magic' below. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode238 git_cl.py:238: print_text.append('Tried jobs on:') On 2015/05/11 23:13:59, M-A Ruel wrote: > Why not printing as it's happening? Why hang output until everything is > complete? It feels much less interactive. Here we're only preparing parameters for the PUT call - nothing is happening until the one request gets sent. This is also consistent with the old way - send request first, and report error or tryjob results based on the exception. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode239 git_cl.py:239: for master, builders_and_tests in masters.iteritems(): On 2015/05/11 23:13:59, M-A Ruel wrote: > Please sort. Done. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode241 git_cl.py:241: bucket = _prefix_master(master) On 2015/05/12 00:46:21, M-A Ruel wrote: > This looks like black magic, explain the context. Buildbucket uses full master name as bucket name 'master.tryserver.chromium.linux', while the developers always use 'tryserver.chromium.linux' to schedule tryjobs by stripping off the prefix 'master.'. This "black magic" here is to convert user-specified master name to formal master name behind the scene, so this migration will not cause any end user workflow change. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode242 git_cl.py:242: for builder, tests in builders_and_tests.iteritems(): On 2015/05/12 00:46:21, M-A Ruel wrote: > sort this too. Done. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode267 git_cl.py:267: 'parameters_json': json.dumps(parameters), On 2015/05/12 00:46:21, M-A Ruel wrote: > What. You are encoding json inside json? Why? Because it's passed without > understanding to buildbot? Yes this parameter property is for buildbot about which Buildbucket has no knowledge, so it ends up with a big json string... Nodir@ should know all details about the design. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode278 git_cl.py:278: try_count -= 1 On 2015/05/11 23:20:22, M-A Ruel wrote: > On 2015/05/11 23:19:36, nodir wrote: > > On 2015/05/11 23:13:59, M-A Ruel wrote: > > > try_count-- > > > > this is invalid Python syntax > > Eh, damn python. Acknowledged. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode281 git_cl.py:281: 'PUT', On 2015/05/11 23:19:36, nodir wrote: > On 2015/05/11 23:01:27, M-A Ruel wrote: > > If I understand correctly, the sole reason http is being pulled in is to allow > > PUT request? > > to authenticate the request. Discussed with maruel@ offline and the recommended request is N/A in depot_tools. https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode308 git_cl.py:308: time.sleep(wait) On 2015/05/11 23:13:59, M-A Ruel wrote: > On 2015/05/11 22:14:01, Vadim Sh. wrote: > > nit: no need to wait if it was the last attempt (i.e. try_count == 0). > > Please address this. if try_count <=0, we'll raise an exception above so this block will not get hit.
There is JSON field parameters_json in a proto message. In general, it is an arbitrary JSON, in buildbot it is builder name, properties, etc. The message is encoded as JSON, so it leads to JSON inside JSON On Mon, May 11, 2015, 6:37 PM <reply@chromiumcodereview-hr.appspotmail.com> wrote: > PTAL. > > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode69 > git_cl.py:69: BUILDBUCKET_PUT_URL = ( > On 2015/05/12 00:46:21, M-A Ruel wrote: > > Please only code the host here. Host != API, so that it's shouldn't be > an > > endpoint here, same for BUILDSET_STR, which is not worth making a > constant. > > > So > > > BUILDBUCKET_HOST = 'cr-buildbucket.appspot.com' > > > then create the URL in the function below. > > Done. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode70 > git_cl.py:70: > 'https://cr-buildbucket.appspot.com/_ah/api/buildbucket/v1/builds/batch') > On 2015/05/11 23:01:27, M-A Ruel wrote: > > On 2015/05/11 22:48:29, nodir wrote: > > > On 2015/05/11 22:21:28, M-A Ruel wrote: > > > > That URL shouldn't be hardcoded there. > > > > > > I think hardcoding the default hostname is fine, but making it > configurable > > > (read from codereview.settings or something) would be good too > > > Not lgtm. > > > It's not optional nor nice to have. > > Done. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode213 > git_cl.py:213: def _prefix_master(master): > On 2015/05/12 00:46:21, M-A Ruel wrote: > > Why? > > See the comments about 'black magic' below. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode238 > git_cl.py:238: print_text.append('Tried jobs on:') > On 2015/05/11 23:13:59, M-A Ruel wrote: > > Why not printing as it's happening? Why hang output until everything > is > > complete? It feels much less interactive. > > Here we're only preparing parameters for the PUT call - nothing is > happening until the one request gets sent. > > This is also consistent with the old way - send request first, and > report error or tryjob results based on the exception. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode239 > git_cl.py:239: for master, builders_and_tests in masters.iteritems(): > On 2015/05/11 23:13:59, M-A Ruel wrote: > > Please sort. > > Done. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode241 > git_cl.py:241: bucket = _prefix_master(master) > On 2015/05/12 00:46:21, M-A Ruel wrote: > > This looks like black magic, explain the context. > > Buildbucket uses full master name as bucket name > 'master.tryserver.chromium.linux', while the developers always use > 'tryserver.chromium.linux' to schedule tryjobs by stripping off the > prefix 'master.'. This "black magic" here is to convert user-specified > master name to formal master name behind the scene, so this migration > will not cause any end user workflow change. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode242 > git_cl.py:242: for builder, tests in builders_and_tests.iteritems(): > On 2015/05/12 00:46:21, M-A Ruel wrote: > > sort this too. > > Done. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode267 > git_cl.py:267: 'parameters_json': json.dumps(parameters), > On 2015/05/12 00:46:21, M-A Ruel wrote: > > What. You are encoding json inside json? Why? Because it's passed > without > > understanding to buildbot? > Yes this parameter property is for buildbot about which Buildbucket has > no knowledge, so it ends up with a big json string... Nodir@ should know > all details about the design. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode278 > git_cl.py:278: try_count -= 1 > On 2015/05/11 23:20:22, M-A Ruel wrote: > > On 2015/05/11 23:19:36, nodir wrote: > > > On 2015/05/11 23:13:59, M-A Ruel wrote: > > > > try_count-- > > > > > > this is invalid Python syntax > > > Eh, damn python. > > Acknowledged. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode281 > git_cl.py:281: 'PUT', > On 2015/05/11 23:19:36, nodir wrote: > > On 2015/05/11 23:01:27, M-A Ruel wrote: > > > If I understand correctly, the sole reason http is being pulled in > is to allow > > > PUT request? > > > to authenticate the request. > > Discussed with maruel@ offline and the recommended request is N/A in > depot_tools. > > https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode308 > git_cl.py:308: time.sleep(wait) > On 2015/05/11 23:13:59, M-A Ruel wrote: > > On 2015/05/11 22:14:01, Vadim Sh. wrote: > > > nit: no need to wait if it was the last attempt (i.e. try_count == > 0). > > > Please address this. > > if try_count <=0, we'll raise an exception above so this block will not > get hit. > > https://codereview.chromium.org/1063103002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode241 git_cl.py:241: bucket = _prefix_master(master) On 2015/05/12 01:37:56, sheyang wrote: > On 2015/05/12 00:46:21, M-A Ruel wrote: > > This looks like black magic, explain the context. > > Buildbucket uses full master name as bucket name > 'master.tryserver.chromium.linux', while the developers always use > 'tryserver.chromium.linux' to schedule tryjobs by stripping off the prefix > 'master.'. This "black magic" here is to convert user-specified master name to > formal master name behind the scene, so this migration will not cause any end > user workflow change. Please add this to _prefix_master docstring then. https://codereview.chromium.org/1063103002/diff/320001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode215 git_cl.py:215: else: No need for else, just return https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode232 git_cl.py:232: buildset_template = 'patch/rietveld/{hostname}/{issue}/{patch}' I don't see the value to use named variable for these, they are part of the API and this whole function is part of the API client code, so: buildset = 'patch/rietveld/{hostname}/{issue}/{patch}'.format( ....) https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode249 git_cl.py:249: 'builder_name': builder, FYI, I always use +2 indentation for structures in new code. https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode250 git_cl.py:250: 'changes':[ 'changes': [ https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode279 git_cl.py:279: wait = 1 In general we do: for try_count in xrange(3): then at line you do something like: time.sleep(0.5 + i*1.5) - you have one variable instead of 2 - the relation between try count and sleep duration is clear, it's a formula - you save 4 lines of code leading to more readable loop. https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode3008 git_cl.py:3008: for (master, builders) in masters.iteritems(): sort here while at it, it always bothered me that the ordering was random.
https://codereview.chromium.org/1063103002/diff/300001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/300001/git_cl.py#newcode241 git_cl.py:241: bucket = _prefix_master(master) On 2015/05/12 13:05:27, M-A Ruel wrote: > On 2015/05/12 01:37:56, sheyang wrote: > > On 2015/05/12 00:46:21, M-A Ruel wrote: > > > This looks like black magic, explain the context. > > > > Buildbucket uses full master name as bucket name > > 'master.tryserver.chromium.linux', while the developers always use > > 'tryserver.chromium.linux' to schedule tryjobs by stripping off the prefix > > 'master.'. This "black magic" here is to convert user-specified master name to > > formal master name behind the scene, so this migration will not cause any end > > user workflow change. > > Please add this to _prefix_master docstring then. Done. https://codereview.chromium.org/1063103002/diff/320001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode215 git_cl.py:215: else: On 2015/05/12 13:05:27, M-A Ruel wrote: > No need for else, just return Done. https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode232 git_cl.py:232: buildset_template = 'patch/rietveld/{hostname}/{issue}/{patch}' On 2015/05/12 13:05:28, M-A Ruel wrote: > I don't see the value to use named variable for these, they are part of the API > and this whole function is part of the API client code, so: > > buildset = 'patch/rietveld/{hostname}/{issue}/{patch}'.format( > ....) Done. https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode249 git_cl.py:249: 'builder_name': builder, On 2015/05/12 13:05:27, M-A Ruel wrote: > FYI, I always use +2 indentation for structures in new code. I thought +4 is for line break? https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode250 git_cl.py:250: 'changes':[ On 2015/05/12 13:05:27, M-A Ruel wrote: > 'changes': [ Eagle eyes...Done. https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode279 git_cl.py:279: wait = 1 On 2015/05/12 13:05:27, M-A Ruel wrote: > In general we do: > > for try_count in xrange(3): > > > then at line you do something like: > > time.sleep(0.5 + i*1.5) > > - you have one variable instead of 2 > - the relation between try count and sleep duration is clear, it's a formula > - you save 4 lines of code leading to more readable loop. Done. https://codereview.chromium.org/1063103002/diff/320001/git_cl.py#newcode3008 git_cl.py:3008: for (master, builders) in masters.iteritems(): On 2015/05/12 13:05:28, M-A Ruel wrote: > sort here while at it, it always bothered me that the ordering was random. Done.
https://codereview.chromium.org/1063103002/diff/340001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode225 git_cl.py:225: def trigger_try_jobs( This fits a single line, it's 75 cols. https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode307 git_cl.py:307: raise httplib2.HttpLib2Error(content) What about: - Endpoints returns HTTP 200 - Endpoints return invalid JSON Look at the behavior of this loop in that situation. https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode2883 git_cl.py:2883: "--use-buildbucket", action="store_true", default=False, default=False is not needed, I don't know why people keep on adding these. The default None works just fine. https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode2984 git_cl.py:2984: trigger_try_jobs( This fits 80 cols, please do not wrap unless needed. https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode2999 git_cl.py:2999: except urllib2.HTTPError, e: as e
https://codereview.chromium.org/1063103002/diff/340001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode225 git_cl.py:225: def trigger_try_jobs( On 2015/05/12 17:48:58, M-A Ruel wrote: > This fits a single line, it's 75 cols. Done. https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode307 git_cl.py:307: raise httplib2.HttpLib2Error(content) On 2015/05/12 17:48:58, M-A Ruel wrote: > What about: > - Endpoints returns HTTP 200 > - Endpoints return invalid JSON > > Look at the behavior of this loop in that situation. In this case, it's a bug in Buildbucket. Adding handling code. https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode2883 git_cl.py:2883: "--use-buildbucket", action="store_true", default=False, On 2015/05/12 17:48:59, M-A Ruel wrote: > default=False is not needed, I don't know why people keep on adding these. The > default None works just fine. I think it makes the default value for a boolean flag more obvious and understandable. https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode2984 git_cl.py:2984: trigger_try_jobs( On 2015/05/12 17:48:58, M-A Ruel wrote: > This fits 80 cols, please do not wrap unless needed. Done. https://codereview.chromium.org/1063103002/diff/340001/git_cl.py#newcode2999 git_cl.py:2999: except urllib2.HTTPError, e: On 2015/05/12 17:48:58, M-A Ruel wrote: > as e Done.
lgtm
The CQ bit was checked by sheyang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org, nodir@chromium.org Link to the patchset: https://codereview.chromium.org/1063103002/#ps360001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1063103002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 1063103002-360001 failed and returned exit status 1. Running presubmit commit checks ... Initialized empty Git repository in /b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/_rietveld/.git/ Updating origin From https://chromium.googlesource.com/infra/infra * [new branch] deployed -> origin/deployed * [new branch] master -> origin/master From https://chromium.googlesource.com/infra/infra * branch master -> FETCH_HEAD 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 ** tests/git_rebase_update_test.py (13.24s) failed .Switched to branch foobar. Switched to branch sub_K. Switched to branch old_branch. Switched to branch empty_branch. Switched to branch empty_branch2. FSwitched to branch foobar. . ====================================================================== FAIL: testRebaseUpdate (__main__.GitRebaseUpdateTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_rebase_update_test.py", line 207, in testRebaseUpdate 'branch_L', 'old_branch', 'foobar'}) AssertionError: Items in the first set but not the second: 'origin/master)' ---------------------------------------------------------------------- Ran 3 tests in 12.823s FAILED (failures=1) Name Stmts Miss Cover Missing --------------------------------------------------- git_new_branch 28 0 100% git_rebase_update 144 0 100% git_rename_branch 29 0 100% git_reparent_branch 43 0 100% --------------------------------------------------- TOTAL 244 0 100% tests/git_common_test.py (6.39s) failed ...x....F.....FF........uCannot foo with a dirty tree. You must commit locally first. Uncommitted files: (git diff-index --name-status HEAD) A test.file ....... ====================================================================== FAIL: testGetBranchTree (__main__.GitMutableStructuredTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 536, in testGetBranchTree self.assertEqual(skipped, {'master', 'root_X', 'branch_DOG', 'root_CAT'}) AssertionError: Items in the first set but not the second: '78df7e3)' ====================================================================== FAIL: testTooManyBranches (__main__.GitMutableStructuredTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 464, in testTooManyBranches self.assertEqual(38, len(self.repo.run(list, self.gc.branches()))) AssertionError: 38 != 39 ====================================================================== FAIL: testBranches (__main__.GitReadOnlyFunctionsTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 235, in testBranches {'master', 'branch_D', 'root_A'}) AssertionError: Items in the first set but not the second: '696a838)' ---------------------------------------------------------------------- Ran 32 tests in 6.159s FAILED (failures=3, expected failures=1, unexpected successes=1) Name Stmts Miss Cover Missing ------------------------------------------ git_common 388 19 95% 319, 683-706 FATAL: not at 100% coverage. Presubmit checks took 175.1s to calculate.
On 2015/05/12 18:56:43, I haz the power (commit-bot) wrote: > Presubmit check for 1063103002-360001 failed and returned exit status 1. > > Running presubmit commit checks ... > Initialized empty Git repository in > /b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/_rietveld/.git/ > Updating origin > From https://chromium.googlesource.com/infra/infra > * [new branch] deployed -> origin/deployed > * [new branch] master -> origin/master > From https://chromium.googlesource.com/infra/infra > * branch master -> FETCH_HEAD > 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 ** > tests/git_rebase_update_test.py (13.24s) failed > .Switched to branch foobar. > Switched to branch sub_K. > Switched to branch old_branch. > Switched to branch empty_branch. > Switched to branch empty_branch2. > FSwitched to branch foobar. > . > ====================================================================== > FAIL: testRebaseUpdate (__main__.GitRebaseUpdateTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/git_rebase_update_test.py", line 207, in testRebaseUpdate > 'branch_L', 'old_branch', 'foobar'}) > AssertionError: Items in the first set but not the second: > 'origin/master)' > > ---------------------------------------------------------------------- > Ran 3 tests in 12.823s > > FAILED (failures=1) > Name Stmts Miss Cover Missing > --------------------------------------------------- > git_new_branch 28 0 100% > git_rebase_update 144 0 100% > git_rename_branch 29 0 100% > git_reparent_branch 43 0 100% > --------------------------------------------------- > TOTAL 244 0 100% > > > tests/git_common_test.py (6.39s) failed > ...x....F.....FF........uCannot foo with a dirty tree. You must commit locally > first. > Uncommitted files: (git diff-index --name-status HEAD) > A test.file > ....... > ====================================================================== > FAIL: testGetBranchTree (__main__.GitMutableStructuredTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/git_common_test.py", line 536, in testGetBranchTree > self.assertEqual(skipped, {'master', 'root_X', 'branch_DOG', 'root_CAT'}) > AssertionError: Items in the first set but not the second: > '78df7e3)' > > ====================================================================== > FAIL: testTooManyBranches (__main__.GitMutableStructuredTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/git_common_test.py", line 464, in testTooManyBranches > self.assertEqual(38, len(self.repo.run(list, self.gc.branches()))) > AssertionError: 38 != 39 > > ====================================================================== > FAIL: testBranches (__main__.GitReadOnlyFunctionsTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/git_common_test.py", line 235, in testBranches > {'master', 'branch_D', 'root_A'}) > AssertionError: Items in the first set but not the second: > '696a838)' > > ---------------------------------------------------------------------- > Ran 32 tests in 6.159s > > FAILED (failures=3, expected failures=1, unexpected successes=1) > Name Stmts Miss Cover Missing > ------------------------------------------ > git_common 388 19 95% 319, 683-706 > FATAL: not at 100% coverage. > > > Presubmit checks took 175.1s to calculate. I cannot reproduce the errors locally, and they don't look related to my changes... Retrying.
The CQ bit was checked by sheyang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1063103002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 1063103002-360001 failed and returned exit status 1. Running presubmit commit checks ... Initialized empty Git repository in /b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/_rietveld/.git/ Updating origin From https://chromium.googlesource.com/infra/infra * [new branch] deployed -> origin/deployed * [new branch] master -> origin/master From https://chromium.googlesource.com/infra/infra * branch master -> FETCH_HEAD 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 ** tests/git_rebase_update_test.py (12.62s) failed .Switched to branch foobar. Switched to branch sub_K. Switched to branch old_branch. Switched to branch empty_branch. Switched to branch empty_branch2. FSwitched to branch foobar. . ====================================================================== FAIL: testRebaseUpdate (__main__.GitRebaseUpdateTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_rebase_update_test.py", line 207, in testRebaseUpdate 'branch_L', 'old_branch', 'foobar'}) AssertionError: Items in the first set but not the second: 'origin/master)' ---------------------------------------------------------------------- Ran 3 tests in 12.415s FAILED (failures=1) Name Stmts Miss Cover Missing --------------------------------------------------- git_new_branch 28 0 100% git_rebase_update 144 0 100% git_rename_branch 29 0 100% git_reparent_branch 43 0 100% --------------------------------------------------- TOTAL 244 0 100% tests/git_common_test.py (6.34s) failed ...x....F.....FF........uCannot foo with a dirty tree. You must commit locally first. Uncommitted files: (git diff-index --name-status HEAD) A test.file ....... ====================================================================== FAIL: testGetBranchTree (__main__.GitMutableStructuredTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 536, in testGetBranchTree self.assertEqual(skipped, {'master', 'root_X', 'branch_DOG', 'root_CAT'}) AssertionError: Items in the first set but not the second: '78df7e3)' ====================================================================== FAIL: testTooManyBranches (__main__.GitMutableStructuredTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 464, in testTooManyBranches self.assertEqual(38, len(self.repo.run(list, self.gc.branches()))) AssertionError: 38 != 39 ====================================================================== FAIL: testBranches (__main__.GitReadOnlyFunctionsTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 235, in testBranches {'master', 'branch_D', 'root_A'}) AssertionError: Items in the first set but not the second: '696a838)' ---------------------------------------------------------------------- Ran 32 tests in 6.142s FAILED (failures=3, expected failures=1, unexpected successes=1) Name Stmts Miss Cover Missing ------------------------------------------ git_common 388 19 95% 319, 683-706 FATAL: not at 100% coverage. Presubmit checks took 178.2s to calculate.
On 2015/05/12 19:07:24, I haz the power (commit-bot) wrote: > Presubmit check for 1063103002-360001 failed and returned exit status 1. > > Running presubmit commit checks ... > Initialized empty Git repository in > /b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/_rietveld/.git/ > Updating origin > From https://chromium.googlesource.com/infra/infra > * [new branch] deployed -> origin/deployed > * [new branch] master -> origin/master > From https://chromium.googlesource.com/infra/infra > * branch master -> FETCH_HEAD > 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 ** > tests/git_rebase_update_test.py (12.62s) failed > .Switched to branch foobar. > Switched to branch sub_K. > Switched to branch old_branch. > Switched to branch empty_branch. > Switched to branch empty_branch2. > FSwitched to branch foobar. > . > ====================================================================== > FAIL: testRebaseUpdate (__main__.GitRebaseUpdateTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/git_rebase_update_test.py", line 207, in testRebaseUpdate > 'branch_L', 'old_branch', 'foobar'}) > AssertionError: Items in the first set but not the second: > 'origin/master)' > > ---------------------------------------------------------------------- > Ran 3 tests in 12.415s > > FAILED (failures=1) > Name Stmts Miss Cover Missing > --------------------------------------------------- > git_new_branch 28 0 100% > git_rebase_update 144 0 100% > git_rename_branch 29 0 100% > git_reparent_branch 43 0 100% > --------------------------------------------------- > TOTAL 244 0 100% > > > tests/git_common_test.py (6.34s) failed > ...x....F.....FF........uCannot foo with a dirty tree. You must commit locally > first. > Uncommitted files: (git diff-index --name-status HEAD) > A test.file > ....... > ====================================================================== > FAIL: testGetBranchTree (__main__.GitMutableStructuredTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/git_common_test.py", line 536, in testGetBranchTree > self.assertEqual(skipped, {'master', 'root_X', 'branch_DOG', 'root_CAT'}) > AssertionError: Items in the first set but not the second: > '78df7e3)' > > ====================================================================== > FAIL: testTooManyBranches (__main__.GitMutableStructuredTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/git_common_test.py", line 464, in testTooManyBranches > self.assertEqual(38, len(self.repo.run(list, self.gc.branches()))) > AssertionError: 38 != 39 > > ====================================================================== > FAIL: testBranches (__main__.GitReadOnlyFunctionsTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "tests/git_common_test.py", line 235, in testBranches > {'master', 'branch_D', 'root_A'}) > AssertionError: Items in the first set but not the second: > '696a838)' > > ---------------------------------------------------------------------- > Ran 32 tests in 6.142s > > FAILED (failures=3, expected failures=1, unexpected successes=1) > Name Stmts Miss Cover Missing > ------------------------------------------ > git_common 388 19 95% 319, 683-706 > FATAL: not at 100% coverage. > > > Presubmit checks took 178.2s to calculate. Okay, so these git related tests are broken due to git upgrade: https://code.google.com/p/chromium/issues/detail?id=487172 I'm skipping presubmit now.
The CQ bit was checked by sheyang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1063103002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 1063103002-360001 failed and returned exit status 1. Running presubmit commit checks ... Initialized empty Git repository in /b/infra_internal/commit_queue/workdir/tools/depot_tools/testing_support/_rietveld/.git/ Updating origin From https://chromium.googlesource.com/infra/infra * [new branch] deployed -> origin/deployed * [new branch] master -> origin/master From https://chromium.googlesource.com/infra/infra * branch master -> FETCH_HEAD 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 ** tests/git_rebase_update_test.py (12.17s) failed .Switched to branch foobar. Switched to branch sub_K. Switched to branch old_branch. Switched to branch empty_branch. Switched to branch empty_branch2. FSwitched to branch foobar. . ====================================================================== FAIL: testRebaseUpdate (__main__.GitRebaseUpdateTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_rebase_update_test.py", line 207, in testRebaseUpdate 'branch_L', 'old_branch', 'foobar'}) AssertionError: Items in the first set but not the second: 'origin/master)' ---------------------------------------------------------------------- Ran 3 tests in 11.943s FAILED (failures=1) Name Stmts Miss Cover Missing --------------------------------------------------- git_new_branch 28 0 100% git_rebase_update 144 0 100% git_rename_branch 29 0 100% git_reparent_branch 43 0 100% --------------------------------------------------- TOTAL 244 0 100% tests/git_common_test.py (6.20s) failed ...x....F.....FF........uCannot foo with a dirty tree. You must commit locally first. Uncommitted files: (git diff-index --name-status HEAD) A test.file ....... ====================================================================== FAIL: testGetBranchTree (__main__.GitMutableStructuredTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 536, in testGetBranchTree self.assertEqual(skipped, {'master', 'root_X', 'branch_DOG', 'root_CAT'}) AssertionError: Items in the first set but not the second: '78df7e3)' ====================================================================== FAIL: testTooManyBranches (__main__.GitMutableStructuredTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 464, in testTooManyBranches self.assertEqual(38, len(self.repo.run(list, self.gc.branches()))) AssertionError: 38 != 39 ====================================================================== FAIL: testBranches (__main__.GitReadOnlyFunctionsTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "tests/git_common_test.py", line 235, in testBranches {'master', 'branch_D', 'root_A'}) AssertionError: Items in the first set but not the second: '696a838)' ---------------------------------------------------------------------- Ran 32 tests in 5.979s FAILED (failures=3, expected failures=1, unexpected successes=1) Name Stmts Miss Cover Missing ------------------------------------------ git_common 388 19 95% 319, 683-706 FATAL: not at 100% coverage. Presubmit checks took 176.6s to calculate.
Message was sent while issue was closed.
Committed patchset #19 (id:360001) manually as 295236 (presubmit successful). |
