Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
447363002:
Auto find tryserver master for git cl try (Closed)
DescriptionAuto find tryserver master for git cl try
BUG=352461
TEST=local tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=289711
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 2
Patch Set 3 : A new function #
Total comments: 23
Patch Set 4 : #
Total comments: 12
Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 22 (2 generated)
+M-A for depot_tools OWNERS https://codereview.chromium.org/447363002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/1/git_cl.py#newcode2280 git_cl.py:2280: map_url = 'http://builders-map.appspot.com/' This should use HTTPS. https://codereview.chromium.org/447363002/diff/1/git_cl.py#newcode2285 git_cl.py:2285: err_msg = ('Fail to fetch builder-to-master map from %s. Error: %s.' % nit: Fail -> Failed https://codereview.chromium.org/447363002/diff/1/git_cl.py#newcode2287 git_cl.py:2287: logging.warn(err_msg) Why do we need this (here and below) if we actually record the error and print it using parser.error? https://codereview.chromium.org/447363002/diff/1/git_cl.py#newcode2292 git_cl.py:2292: if master_map: Have you tested the code path when we catch an exception? This seems likely to throw NameError: name 'master_map' is not defined I'd recommend initializing master_map to none above.
https://codereview.chromium.org/447363002/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/1/git_cl.py#newcode2280 git_cl.py:2280: map_url = 'http://builders-map.appspot.com/' On 2014/08/08 11:13:50, Paweł Hajdan Jr. wrote: > This should use HTTPS. Done. https://codereview.chromium.org/447363002/diff/1/git_cl.py#newcode2285 git_cl.py:2285: err_msg = ('Fail to fetch builder-to-master map from %s. Error: %s.' % On 2014/08/08 11:13:50, Paweł Hajdan Jr. wrote: > nit: Fail -> Failed Done. https://codereview.chromium.org/447363002/diff/1/git_cl.py#newcode2287 git_cl.py:2287: logging.warn(err_msg) On 2014/08/08 11:13:50, Paweł Hajdan Jr. wrote: > Why do we need this (here and below) if we actually record the error and print > it using parser.error? Removed. https://codereview.chromium.org/447363002/diff/1/git_cl.py#newcode2292 git_cl.py:2292: if master_map: On 2014/08/08 11:13:50, Paweł Hajdan Jr. wrote: > Have you tested the code path when we catch an exception? This seems likely to > throw NameError: name 'master_map' is not defined > > I'd recommend initializing master_map to none above. Done.
LGTM
https://codereview.chromium.org/447363002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/20001/git_cl.py#newcode2279 git_cl.py:2279: err_msg = '' Would you mind making this discovery part a function? It's very chromium infra specific. (git-cl is chromium infra specific but we use it on other projects too).
https://codereview.chromium.org/447363002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/20001/git_cl.py#newcode2279 git_cl.py:2279: err_msg = '' On 2014/08/13 12:47:07, M-A Ruel wrote: > Would you mind making this discovery part a function? It's very chromium infra > specific. (git-cl is chromium infra specific but we use it on other projects > too). Done.
https://codereview.chromium.org/447363002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2209 git_cl.py:2209: 2 lines https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2218 git_cl.py:2218: master_map = json.load(res) master_map = json.load(urllib2.urlopen(map_url)) https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2219 git_cl.py:2219: except urllib2.URLError as url_e: s/url_e/e/ https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2220 git_cl.py:2220: err_msg = ('Failed to fetch builder-to-master map from %s. Error: %s.' % Why not return right away? https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2222 git_cl.py:2222: except ValueError as val_e: s/val_e/e/ https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2225 git_cl.py:2225: if master_map: if not master_map: return None, err_msg https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2228 git_cl.py:2228: builder, _ = bot.split(':', 1) builder = bot.split(':', 1)[0] Remove line 2227, 2229-2230. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2233 git_cl.py:2233: err_msg = 'Cannot find a master for builder %s.' % builder It'd likely be simpler to return right away. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2236 git_cl.py:2236: err_msg = ('The builder name %s exists in multiple masters %s.' % Same https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2245 git_cl.py:2245: result_master = '' Return https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2248 git_cl.py:2248: 2 lines
https://codereview.chromium.org/447363002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2209 git_cl.py:2209: On 2014/08/14 18:42:02, M-A Ruel wrote: > 2 lines Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2218 git_cl.py:2218: master_map = json.load(res) On 2014/08/14 18:42:02, M-A Ruel wrote: > master_map = json.load(urllib2.urlopen(map_url)) Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2219 git_cl.py:2219: except urllib2.URLError as url_e: On 2014/08/14 18:42:02, M-A Ruel wrote: > s/url_e/e/ Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2220 git_cl.py:2220: err_msg = ('Failed to fetch builder-to-master map from %s. Error: %s.' % On 2014/08/14 18:42:02, M-A Ruel wrote: > Why not return right away? Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2222 git_cl.py:2222: except ValueError as val_e: On 2014/08/14 18:42:02, M-A Ruel wrote: > s/val_e/e/ Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2222 git_cl.py:2222: except ValueError as val_e: On 2014/08/14 18:42:02, M-A Ruel wrote: > s/val_e/e/ Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2225 git_cl.py:2225: if master_map: On 2014/08/14 18:42:02, M-A Ruel wrote: > if not master_map: > return None, err_msg Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2228 git_cl.py:2228: builder, _ = bot.split(':', 1) On 2014/08/14 18:42:02, M-A Ruel wrote: > builder = bot.split(':', 1)[0] > > Remove line 2227, 2229-2230. Removed. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2233 git_cl.py:2233: err_msg = 'Cannot find a master for builder %s.' % builder On 2014/08/14 18:42:02, M-A Ruel wrote: > It'd likely be simpler to return right away. Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2236 git_cl.py:2236: err_msg = ('The builder name %s exists in multiple masters %s.' % On 2014/08/14 18:42:02, M-A Ruel wrote: > Same Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2245 git_cl.py:2245: result_master = '' On 2014/08/14 18:42:02, M-A Ruel wrote: > Return Done. https://codereview.chromium.org/447363002/diff/40001/git_cl.py#newcode2248 git_cl.py:2248: On 2014/08/14 18:42:02, M-A Ruel wrote: > 2 lines Done.
https://codereview.chromium.org/447363002/diff/60001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2213 git_cl.py:2213: err_msg = '' remove https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2214 git_cl.py:2214: master_map = None Remove https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2215 git_cl.py:2215: result_master = '' Move to line 2229 https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2220 git_cl.py:2220: err_msg = ('Failed to fetch builder-to-master map from %s. Error: %s.' % return None, ('... https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2224 git_cl.py:2224: err_msg = 'Invalid json string from %s. Error: %s.' % (map_url, e) return None, '... and for the lines below. https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2247 git_cl.py:2247: return result_master, err_msg return result_master, None
https://codereview.chromium.org/447363002/diff/60001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2213 git_cl.py:2213: err_msg = '' On 2014/08/14 19:59:38, M-A Ruel wrote: > remove Done. https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2214 git_cl.py:2214: master_map = None On 2014/08/14 19:59:38, M-A Ruel wrote: > Remove Done. https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2215 git_cl.py:2215: result_master = '' On 2014/08/14 19:59:38, M-A Ruel wrote: > Move to line 2229 Done. https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2220 git_cl.py:2220: err_msg = ('Failed to fetch builder-to-master map from %s. Error: %s.' % On 2014/08/14 19:59:38, M-A Ruel wrote: > return None, ('... Do you want to get rid of the err_msg variable? https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2247 git_cl.py:2247: return result_master, err_msg On 2014/08/14 19:59:38, M-A Ruel wrote: > return result_master, None Done.
https://codereview.chromium.org/447363002/diff/60001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/447363002/diff/60001/git_cl.py#newcode2220 git_cl.py:2220: err_msg = ('Failed to fetch builder-to-master map from %s. Error: %s.' % On 2014/08/14 20:45:09, sheyang wrote: > On 2014/08/14 19:59:38, M-A Ruel wrote: > > return None, ('... > > Do you want to get rid of the err_msg variable? Exact, it serves no purpose.
lgtm
The CQ bit was checked by sheyang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheyang@chromium.org/447363002/100001
Message was sent while issue was closed.
Committed patchset #6 (100001) as 289711
Message was sent while issue was closed.
nodir@chromium.org changed reviewers: + nodir@chromium.org
Message was sent while issue was closed.
ugh, discovered this. builders-map.appspot.com is in experimental directory https://cs.chromium.org/chromium/infra/appengine/experimental/builders_map/bu... and chrome-troopers do not have access to this cloud project
Message was sent while issue was closed.
I thought Sergey had a plan to kill it. Is it done? Best regards, Sheng On Fri, Jul 29, 2016 at 12:29 PM, <nodir@chromium.org> wrote: > ugh, discovered this. builders-map.appspot.com is in experimental > directory > > https://cs.chromium.org/chromium/infra/appengine/experimental/builders_map/bu... > and chrome-troopers do not have access to this cloud project > > https://codereview.chromium.org/447363002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
No, I never got around to fixing that: http://crbug.com/570733 On Fri, Jul 29, 2016 at 1:44 PM Sheng Yang <sheyang@chromium.org> wrote: > I thought Sergey had a plan to kill it. Is it done? > > Best regards, > > Sheng > > On Fri, Jul 29, 2016 at 12:29 PM, <nodir@chromium.org> wrote: > >> ugh, discovered this. builders-map.appspot.com is in experimental >> directory >> >> https://cs.chromium.org/chromium/infra/appengine/experimental/builders_map/bu... >> and chrome-troopers do not have access to this cloud project >> >> https://codereview.chromium.org/447363002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
sergeyberezin@chromium.org changed reviewers: + sergeyberezin@chromium.org
Message was sent while issue was closed.
Also added chrome-troopers as Editor to the app. |
