Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885153005:
Policy definition for encryptfs to ext4 migration strategy (Closed)
DescriptionPolicy definition for encryptfs to ext4 migration strategy
This is just the proto definition for the policy. It should allow
development on server side without waiting for client implementation.
BUG=722371
Review-Url: https://codereview.chromium.org/2885153005
Cr-Commit-Position: refs/heads/master@{#472487}
Committed: https://chromium.googlesource.com/chromium/src/+/3f2e3a76424261598c4033dcb06e49f9c3205808
Patch Set 1 #
Total comments: 1
Patch Set 2 : Nit #
Total comments: 1
Patch Set 3 : Removed default #Messages
Total messages: 29 (12 generated)
igorcov@chromium.org changed reviewers: + atwilson@chromium.org, bartfab@chromium.org
bartfab@google.com changed reviewers: + bartfab@google.com
lgtm https://codereview.chromium.org/2885153005/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/2885153005/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:782: // Migration strategy for the case when ARC(N) needs the ext4 encryption while Nit: s/N/N+/
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@google.com Link to the patchset: https://codereview.chromium.org/2885153005/#ps20001 (title: "Nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
lgtm
lgtm
The CQ bit was checked by bartfab@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
On 2017/05/17 14:47:32, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started once the patch has received an L-G-T-M from a full > committer. > Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a > full super star committer. > Committers are members of the group "project-chromium-committers". > Note that this has nothing to do with OWNERS files. lgtm because bartfab@google.com is not an OWNER :)
On 2017/05/17 14:47:32, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started once the patch has received an L-G-T-M from a full > committer. > Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a > full super star committer. > Committers are members of the group "project-chromium-committers". > Note that this has nothing to do with OWNERS files. lgtm because bartfab@google.com is not an OWNER :)
atwilson@google.com changed reviewers: + atwilson@google.com
https://codereview.chromium.org/2885153005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/2885153005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:792: optional MigrationStrategy migration_strategy = 1 [default = DISALLOW_ARC]; Typically we don't specify defaults in this file, but instead hard-code default behavior in the code.
On 2017/05/17 14:55:56, Andrew Wilson wrote: > https://codereview.chromium.org/2885153005/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): > > https://codereview.chromium.org/2885153005/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:792: optional > MigrationStrategy migration_strategy = 1 [default = DISALLOW_ARC]; > Typically we don't specify defaults in this file, but instead hard-code default > behavior in the code. LGTM from correct acct this time, but please take the default out.
On 2017/05/17 14:57:46, Andrew T Wilson (Slow) wrote: > On 2017/05/17 14:55:56, Andrew Wilson wrote: > > > https://codereview.chromium.org/2885153005/diff/20001/chrome/browser/chromeos... > > File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): > > > > > https://codereview.chromium.org/2885153005/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:792: optional > > MigrationStrategy migration_strategy = 1 [default = DISALLOW_ARC]; > > Typically we don't specify defaults in this file, but instead hard-code > default > > behavior in the code. > > LGTM from correct acct this time, but please take the default out. This file is peppered with defaults. You are right that we hard-code things on the client. And the proto file serves as documentation of these defaults for the server-side team. And... why am I not an owner? :(
On 2017/05/17 15:01:21, bartfab1 wrote: > On 2017/05/17 14:57:46, Andrew T Wilson (Slow) wrote: > > On 2017/05/17 14:55:56, Andrew Wilson wrote: > > > > > > https://codereview.chromium.org/2885153005/diff/20001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto > (right): > > > > > > > > > https://codereview.chromium.org/2885153005/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:792: > optional > > > MigrationStrategy migration_strategy = 1 [default = DISALLOW_ARC]; > > > Typically we don't specify defaults in this file, but instead hard-code > > default > > > behavior in the code. > > > > LGTM from correct acct this time, but please take the default out. > > This file is peppered with defaults. You are right that we hard-code things on > the client. And the proto file serves as documentation of these defaults for the > server-side team. > > And... why am I not an owner? :( Oh, I see, I am signed in with my google.com account. FAIL.
LGTM, just for my peace of mind :).
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, bartfab@google.com, atwilson@chromium.org, atwilson@google.com Link to the patchset: https://codereview.chromium.org/2885153005/#ps40001 (title: "Removed default")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495038771953480,
"parent_rev": "27a1d93030604b777013fafab121a4e6e0ead70f", "commit_rev":
"3f2e3a76424261598c4033dcb06e49f9c3205808"}
Message was sent while issue was closed.
Description was changed from ========== Policy definition for encryptfs to ext4 migration strategy This is just the proto definition for the policy. It should allow development on server side without waiting for client implementation. BUG=722371 ========== to ========== Policy definition for encryptfs to ext4 migration strategy This is just the proto definition for the policy. It should allow development on server side without waiting for client implementation. BUG=722371 Review-Url: https://codereview.chromium.org/2885153005 Cr-Commit-Position: refs/heads/master@{#472487} Committed: https://chromium.googlesource.com/chromium/src/+/3f2e3a76424261598c4033dcb06e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3f2e3a76424261598c4033dcb06e... |
