Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1140333002:
Added log messages for cases of failure of open files in policy_testserver.py. (Closed)
DescriptionAdded log messages for cases of failure of open files in policy_testserver.py.
Improved logging in chrome code in case of error in policy fetching.
BUG=489230
Committed: https://crrev.com/cfdf46013b37da38c878632b87fa24f1b7997f3d
Cr-Commit-Position: refs/heads/master@{#330350}
Committed: https://crrev.com/f70b4312aa651ecb598a0287ee942efbc7c1e15b
Cr-Commit-Position: refs/heads/master@{#334574}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Removed cloud_policy_client.cc from the CL #
Total comments: 2
Patch Set 4 : #
Total comments: 2
Patch Set 5 : Merge #Patch Set 6 : Log to crash #Messages
Total messages: 20 (6 generated)
Hi Philipp, please review our changes ;)
peletskyi@chromium.org changed reviewers: + pneubeck@chromium.org
Hi Philipp, please review our changes ;)
chrome/browser/policy/test/policy_testserver.py lgtm please put the other change into a separate CL and let mnissler review it.
The CQ bit was checked by peletskyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pneubeck@chromium.org Link to the patchset: https://codereview.chromium.org/1140333002/#ps40001 (title: "Removed cloud_policy_client.cc from the CL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140333002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cfdf46013b37da38c878632b87fa24f1b7997f3d Cr-Commit-Position: refs/heads/master@{#330350}
Message was sent while issue was closed.
mnissler@chromium.org changed reviewers: + mnissler@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1140333002/diff/40001/chrome/browser/policy/t... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/1140333002/diff/40001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:82: logging.error('Could not import json') I think it's meanwhile fine to just import json unconditionally. This code dates back to a time when we couldn't assume the bots to have a json module available. I suggest you remove the try-except here entirely and move the import json statement to the other imports above. If the trybots like it, it's fine and this hack can go away. https://codereview.chromium.org/1140333002/diff/40001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:95: logging.error('Could not import chrome_extension_policy_pb2') This will add log spew I guess. A better way of doing this might be to move the imports to the point where these protobufs actually get used?
On 2015/05/18 13:29:17, Mattias Nissler wrote: > https://codereview.chromium.org/1140333002/diff/40001/chrome/browser/policy/t... > File chrome/browser/policy/test/policy_testserver.py (right): > > https://codereview.chromium.org/1140333002/diff/40001/chrome/browser/policy/t... > chrome/browser/policy/test/policy_testserver.py:82: logging.error('Could not > import json') > I think it's meanwhile fine to just import json unconditionally. This code dates > back to a time when we couldn't assume the bots to have a json module available. > I suggest you remove the try-except here entirely and move the import json > statement to the other imports above. If the trybots like it, it's fine and this > hack can go away. > > https://codereview.chromium.org/1140333002/diff/40001/chrome/browser/policy/t... > chrome/browser/policy/test/policy_testserver.py:95: logging.error('Could not > import chrome_extension_policy_pb2') > This will add log spew I guess. A better way of doing this might be to move the > imports to the point where these protobufs actually get used? Mattias, ptal
Please create a separate CL out of this as the original code has already landed? https://codereview.chromium.org/1140333002/diff/60001/chrome/browser/policy/t... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/1140333002/diff/60001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:74: import json alphabetize https://codereview.chromium.org/1140333002/diff/60001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:756: logging.error('chrome_device_policy_pb2 does not exist') Why not just drop the ep is None and dp is None checks above? The test server will noisily crash then, which seems appropriate?
On 2015/06/15 07:47:18, Mattias Nissler wrote: > Please create a separate CL out of this as the original code has already landed? > > https://codereview.chromium.org/1140333002/diff/60001/chrome/browser/policy/t... > File chrome/browser/policy/test/policy_testserver.py (right): > > https://codereview.chromium.org/1140333002/diff/60001/chrome/browser/policy/t... > chrome/browser/policy/test/policy_testserver.py:74: import json > alphabetize > > https://codereview.chromium.org/1140333002/diff/60001/chrome/browser/policy/t... > chrome/browser/policy/test/policy_testserver.py:756: > logging.error('chrome_device_policy_pb2 does not exist') > Why not just drop the ep is None and dp is None checks above? The test server > will noisily crash then, which seems appropriate? Done
LGTM
The CQ bit was checked by peletskyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pneubeck@chromium.org Link to the patchset: https://codereview.chromium.org/1140333002/#ps100001 (title: "Log to crash")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140333002/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/f70b4312aa651ecb598a0287ee942efbc7c1e15b Cr-Commit-Position: refs/heads/master@{#334574} |
