Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1063073003:
Retrieval of information by release channel (Closed)
|
Created:
5 years, 8 months ago by Michael Hablich Modified:
5 years, 8 months ago Reviewers:
Michael Achenbach CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionRetrieval of information by release channel
Polls omahaproxy for data about Chrome releases
BUG=
NOTRY=true
Committed: https://crrev.com/6198bbc56d5fc1c7ccb79ce7530b64958f6c5f19
Cr-Commit-Position: refs/heads/master@{#27841}
Patch Set 1 #Patch Set 2 : Data is now dumped into JSON #Patch Set 3 : Chrome release data now dumped into JSON #
Total comments: 25
Patch Set 4 : Fixed broken test #
Total comments: 7
Patch Set 5 : Added test data and fixed nits #
Total comments: 7
Patch Set 6 : Removed empty lines #Messages
Total messages: 12 (2 generated)
hablich@chromium.org changed reviewers: + machenbach@chromium.org
Some comments - mostly nits: https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.py File tools/release/releases.py (right): https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:499: RietrieveChromiumBranches, nit: could you clean up my old spelling mistake: Retrieve... https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:502: WriteOutput nit: , in the end are generally recommended in case the ending brackets are in a new line - makes the diffs easier to read when new lines are added https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:505: Please move class up so that the order of steps matches the order of classes (at least that was the convention so far). https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:512: resultRaw = self.ReadURL('http://omahaproxy.appspot.com/all.json', params, wait_plan=[5, 20]) nit: 80 chars https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:512: resultRaw = self.ReadURL('http://omahaproxy.appspot.com/all.json', params, wait_plan=[5, 20]) nit (style): Use _ for local variables not camel case: result_raw recent_releases current_os current_canary current_version etc https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:512: resultRaw = self.ReadURL('http://omahaproxy.appspot.com/all.json', params, wait_plan=[5, 20]) nit: All v8-side scripts use " instead of ' due to stupid historical reasons. Please stick to one or change them all :) https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:514: print recentReleases Can the print go away in the final version? Or pretty print: print json.dumps(recentReleases, indent=2, sort_keys=True) https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:516: allCanaries = [] Call just canaries? https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:520: currentCandidate = {'version':currentVersion['version'], Move under the condition below? Or are there coming more channels? https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:520: currentCandidate = {'version':currentVersion['version'], Please qualify 'version' since we have more now, e.g. chrome_version. Above in this script, version referred to the v8 version (should have been qualified there as well). https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:521: 'os':currentVersion['os'], nit: indentation - either align all keys or use a new line (better): currentCandidate = { 'version':currentVersion['version'], 'os':currentVersion['os'], ... } https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:523: 'v8_version':currentVersion['v8_version']} nit: always one space after : also below
https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... File tools/release/test_scripts.py (right): https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... tools/release/test_scripts.py:1423: "{}"), Looks good, could you provide some dummy test data? https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... tools/release/test_scripts.py:1441: expected_json = {'chrome_releases':{'canaries':[]}, nit: Always space after : https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... tools/release/test_scripts.py:1441: expected_json = {'chrome_releases':{'canaries':[]}, nit: please keep indentation levels at a minimum - makes it easier to read. When there are loads of data please follow the scheme: ............. = { { { bla, }, }, } https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... tools/release/test_scripts.py:1499: }] nit: }, ]
https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.py File tools/release/releases.py (right): https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:499: RietrieveChromiumBranches, On 2015/04/15 09:27:53, Michael Achenbach wrote: > nit: could you clean up my old spelling mistake: Retrieve... Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:502: WriteOutput On 2015/04/15 09:27:53, Michael Achenbach wrote: > nit: , in the end are generally recommended in case the ending brackets are in a > new line - makes the diffs easier to read when new lines are added Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:505: On 2015/04/15 09:27:53, Michael Achenbach wrote: > Please move class up so that the order of steps matches the order of classes (at > least that was the convention so far). Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:512: resultRaw = self.ReadURL('http://omahaproxy.appspot.com/all.json', params, wait_plan=[5, 20]) On 2015/04/15 09:27:53, Michael Achenbach wrote: > nit (style): Use _ for local variables not camel case: > result_raw > recent_releases > current_os > current_canary > current_version > etc Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:512: resultRaw = self.ReadURL('http://omahaproxy.appspot.com/all.json', params, wait_plan=[5, 20]) On 2015/04/15 09:27:53, Michael Achenbach wrote: > nit: All v8-side scripts use " instead of ' due to stupid historical reasons. > Please stick to one or change them all :) Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:512: resultRaw = self.ReadURL('http://omahaproxy.appspot.com/all.json', params, wait_plan=[5, 20]) On 2015/04/15 09:27:53, Michael Achenbach wrote: > nit (style): Use _ for local variables not camel case: > result_raw > recent_releases > current_os > current_canary > current_version > etc Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:512: resultRaw = self.ReadURL('http://omahaproxy.appspot.com/all.json', params, wait_plan=[5, 20]) On 2015/04/15 09:27:53, Michael Achenbach wrote: > nit: 80 chars Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:514: print recentReleases On 2015/04/15 09:27:53, Michael Achenbach wrote: > Can the print go away in the final version? Or pretty print: > print json.dumps(recentReleases, indent=2, sort_keys=True) Will go away. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:516: allCanaries = [] On 2015/04/15 09:27:53, Michael Achenbach wrote: > Call just canaries? Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:520: currentCandidate = {'version':currentVersion['version'], On 2015/04/15 09:27:53, Michael Achenbach wrote: > Move under the condition below? Or are there coming more channels? More channels coming. Wanted to finish Canary first (show on gui). At least Dev will follow shortly. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:520: currentCandidate = {'version':currentVersion['version'], On 2015/04/15 09:27:53, Michael Achenbach wrote: > Please qualify 'version' since we have more now, e.g. chrome_version. Above in > this script, version referred to the v8 version (should have been qualified > there as well). Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:521: 'os':currentVersion['os'], On 2015/04/15 09:27:53, Michael Achenbach wrote: > nit: indentation - either align all keys or use a new line (better): > currentCandidate = { > 'version':currentVersion['version'], > 'os':currentVersion['os'], > ... > } Acknowledged. https://codereview.chromium.org/1063073003/diff/40001/tools/release/releases.... tools/release/releases.py:523: 'v8_version':currentVersion['v8_version']} On 2015/04/15 09:27:53, Michael Achenbach wrote: > nit: always one space after : also below Acknowledged.
https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... File tools/release/test_scripts.py (right): https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... tools/release/test_scripts.py:1423: "{}"), On 2015/04/15 09:35:39, Michael Achenbach wrote: > Looks good, could you provide some dummy test data? Yep. Wanted to make the tests pass first. https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... tools/release/test_scripts.py:1441: expected_json = {'chrome_releases':{'canaries':[]}, On 2015/04/15 09:35:38, Michael Achenbach wrote: > nit: please keep indentation levels at a minimum - makes it easier to read. When > there are loads of data please follow the scheme: > ............. = { > { > { > bla, > }, > }, > } Acknowledged. https://codereview.chromium.org/1063073003/diff/60001/tools/release/test_scri... tools/release/test_scripts.py:1499: }] On 2015/04/15 09:35:39, Michael Achenbach wrote: > nit: > }, > ] Acknowledged.
Generally lg. Some more nits. https://codereview.chromium.org/1063073003/diff/80001/tools/release/releases.py File tools/release/releases.py (right): https://codereview.chromium.org/1063073003/diff/80001/tools/release/releases.... tools/release/releases.py:443: "http://omahaproxy.appspot.com/all.json", nit: Convention is either: method(first_param, second_param) or: method( first_param, second_param) or: method( first_param, second_param, ) and sometimes in case of lots of small parameters: method(first_param, second_param, third_param, etc...) Generally we use 4 spaces for indentation of method params and 2 spaces for indentation of dict and list content. https://codereview.chromium.org/1063073003/diff/80001/tools/release/releases.... tools/release/releases.py:453: current_candidate = { nit: indentation https://codereview.chromium.org/1063073003/diff/80001/tools/release/releases.... tools/release/releases.py:480: "releases": self["releases"], nit: indentation - see comment on last patchset about dict and list structures https://codereview.chromium.org/1063073003/diff/80001/tools/release/releases.... tools/release/releases.py:539: nit: two empty lines on toplevel https://codereview.chromium.org/1063073003/diff/80001/tools/release/test_scri... File tools/release/test_scripts.py (right): https://codereview.chromium.org/1063073003/diff/80001/tools/release/test_scri... tools/release/test_scripts.py:1430: }] nit: 2 spaces less on closing brackets https://codereview.chromium.org/1063073003/diff/80001/tools/release/test_scri... tools/release/test_scripts.py:1449: expected_json = {"chrome_releases":{ nit: indentation - please move to same level as the stuff below https://codereview.chromium.org/1063073003/diff/80001/tools/release/test_scri... tools/release/test_scripts.py:1514: },], nit: All of the above should get 2 spaces of indentation to match the newly introduced level. Then this bracket should go to a new line...
lgtm - as long as remaining indentation issues are resolved in a follow up
Note, please keep the CL description's first line and the summary line in sync. For the commit message, the first line will be used. I recommend to add NOTRY=true for the CQ.
The CQ bit was checked by hablich@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1063073003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6198bbc56d5fc1c7ccb79ce7530b64958f6c5f19 Cr-Commit-Position: refs/heads/master@{#27841} |
