policy_testserver.py: Change device policy input format to expect top-level message names |
|||
Issue description
[actually reported by kathrelkeld@]
Currently, policy_testserver.py's device policy input format is agnostic to top-level message names.
E.g. it's necessary to set
'google/chromeos/device' to {'timezone': 'America/Los_Angeles'}
in order to reach the field
ChromeDeviceSettingsProto.system_timezone.timezone
This is not clear and can collide with other fields (in this case, ChromeDeviceSettingsProto.device_off_hours.timezone).
The idea is to change the policy_testserver.py device policy input format to fully qualify the field, i.e.
{'system_timezone': {'timezone': 'America/Los_Angeles'}
The logic lives in GatherDevicePolicySettings[1] and we'd need to perform the policy matching/setting in the outer for loop (and get rid of the inner for loop).
Note that we'll have to update the clients in chromium which use policy_testserver.py to set device policy + external clients (autotest).
We could preserve backwards compatibility by falling back to the old logic if nothing has been matched using the new logic.
[1] https://cs.chromium.org/chromium/src/chrome/browser/policy/test/policy_testserver.py?rcl=8ad2991b5fc4aa9b0da2ca62b4d05bef63f7c3ea&l=792
,
Nov 14 2017
,
Dec 2 2017
Is there someone free to take on this bug?
,
Dec 5 2017
I'll try to do this during the next few days.
,
Dec 6 2017
Funnily, the CL r809004 passes the CQ just fine :-) maybe no chromium-side code uses that json-policy-setting thing. So I'm not sure I've done the right thing. Katherine, is there a good way to run autotests with the CL to see if they break (they should)? Also, would it be better to: Strategy 1: - submit a chromium CL, let autotests using old device policy setting format break - Migrate autotests to new device policy setting format Or Strategy 2: - submit a chromium CL, allowing both formats (fallback to old one) - Migrate autotests without a rush ? Also, the chrome tests seem to use another feature of policy_testserver.py: writing a serialized proto into policy_google_chromeos_device.bin through LocalPolicyTestServer::UpdatePolicy[1], which the policy_testserver.py will pick up preferably thorugh ReadPolicyFromDataDir[2]. See e.g. [3]. Maybe this is something that would be even better for autotests? [1] https://cs.chromium.org/chromium/src/chrome/browser/policy/test/local_policy_test_server.cc?l=145&gs=kythe%253A%252F%252Fchromium%253Flang%253Dc%25252B%25252B%253Fpath%253Dsrc%252Fchrome%252Fbrowser%252Fpolicy%252Ftest%252Flocal_policy_test_server.cc%2523ZpEWNNv2AvBIAPNgTgwjOxG%25252FHu3oQxxTOugr7Hh0lZA%25253D&gsn=UpdatePolicy&ct=xref_usages [2] https://cs.chromium.org/chromium/src/chrome/browser/policy/test/policy_testserver.py?rcl=3009675f23c9d898168e015c9bed2c9c6f27a099&l=911 [3] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc?l=155&gs=kythe%253A%252F%252Fchromium%253Flang%253Dc%25252B%25252B%253Fpath%253Dsrc%252Fchrome%252Fbrowser%252Fchromeos%252Fpolicy%252Fdevice_cloud_policy_browsertest.cc%2523OJ8sBrXJOiy7De6FJkIzk45erkHWkyoBm4sQmf0zptA%25253D&gsn=UpdateServedTestPolicy&ct=xref_usages
,
Dec 8 2017
Thankfully we don't need any migration strategy on the Chrome OS side, since it was getting Autotests set up to use device policies which made me notice this bug. When it lands we'll update the Autotest code to use it, but until then it shouldn't break anything not still in-progress. As for the serialized proto, can you elaborate on what that would better?
,
Dec 11 2017
Re: Serialized proto: Well, it may be easier for the test to just specify the proto it wants the policy_testserver to send (instead of using a json-based format to tell it which policies to set to which value). I don't mind either way, just wanted to say that there's also this option. Alright, then I'll add unit test to be confident that what I'm changing actually works because no one seems to be using it at the moment and we can push this change :-) |
|||
►
Sign in to add a comment |
|||
Comment 1 by pmarko@chromium.org
, Nov 9 2017