The Savilerow project

Merge lp:~savilerow-team/savilerow/fix_lp1279119_with_pics into lp:~savilerow-team/savilerow/trunk-1.0

  1. fix_lp1279119_with_pics
  2. Merge into trunk-1.0
Proposed by Chris Wayne
Status: Rejected
Rejected by: Kyle Nitzsche
Proposed branch: lp:~savilerow-team/savilerow/fix_lp1279119_with_pics
Merge into: lp:~savilerow-team/savilerow/trunk-1.0
Diff against target: 76 lines (+33/-6)
2 files modified
src/system/custom/etc/dconf_source/db/custom.d/locks/custom.lock (+0/-1)
tests/api/test_dconf.py (+33/-5)
To merge this branch: bzr merge lp:~savilerow-team/savilerow/fix_lp1279119_with_pics
Reviewer Review Type Date Requested Status
Alex Chiang (community) Needs Fixing
Review via email: mp+206035@code.launchpad.net

This proposal supersedes a proposal from 2014-02-12.

Description of the change

Remove background from list of locked dconf keys, and add test for dconf locks

To post a comment you must log in.
Revision history for this message
Alex Chiang (achiang) wrote : Posted in a previous version of this proposal

Please drop r71, as that MP is still outstanding. This will probably mean rebasing r72 and r73 on trunk.

176 - self.asserttrue(False)
177 + self.assertTrue(False)

I do see you fixed the assertTrue, but it also looks like you might have included a tab instead of spaces. Please double-check.

185 - dval = subprocess.check_output(['dconf', 'read', key])
186 + dval = check_output(['dconf', 'read', key])

Also double-check tab v spaces here.

197 + In some rare cases, downstreams may wish to make a customization
198 + unchangable. This is done by setting a dconf lock.

Suggested alternate wording (for clarity). Note also, one space between period. :)

In some rare cases, downstreams may wish to ship a customization and prevent end users from ever changing it. This is done by setting a dconf lock.

199 +
200 + This test verifies that the lock file exists, and that the contained
201 + keys cannot be changed.

"This test verifies that the dconf lock file exists..."

Lines 203--212: all good, but please insert space between comment char and first letter of comment. Also, please capitalize the comment as if it were a sentence.

Overall: good MP, but a question - should we continue to lock the sounds too? Is there something more benign we could lock, so that people experimenting with savvy don't get surprised? Absolute best would be to lock a dconf key that we're not even using, so the lock itself becomes a nop...

Thoughts?

review: Needs Fixing
Revision history for this message
Alex Chiang (achiang) wrote : Posted in a previous version of this proposal

176 - self.asserttrue(False)
177 + self.assertTrue(False)
...
185 - dval = subprocess.check_output(['dconf', 'read', key])
186 + dval = check_output(['dconf', 'read', key])

I checked the lines myself in an editor, and they were fine. Must have been my mail client making the spaces look funny.

So ignore those comments, but please address the others.

Thanks.

Revision history for this message
Chris Wayne (cwayne) wrote : Posted in a previous version of this proposal

Tested the lock fix by flashing to a clean N4 running devel, then making sure the custom background is shown, and then changing it.

Revision history for this message
Alex Chiang (achiang) wrote :

69 + #iterate through list of locked keys and try to change them

Missed fixing that comment.

This still needs a response:

Overall: good MP, but a question - should we continue to lock the sounds too? Is there something more benign we could lock, so that people experimenting with savvy don't get surprised? Absolute best would be to lock a dconf key that we're not even using, so the lock itself becomes a nop...

Thoughts?

review: Needs Fixing
Revision history for this message
Chris Wayne (cwayne) wrote :

Yes, apologies for not addressing the question:

I think you're right, so I've been trying to look for a good dconf key to lock. I was thinking maybe disabled scopes? I'm not sure if we can lock a dconf key we're not touching, I need to continue investigating this

Unmerged revisions

71. By Chris Wayne

Unlock background and fix dconf_test

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/system/custom/etc/dconf_source/db/custom.d/locks/custom.lock'
2--- src/system/custom/etc/dconf_source/db/custom.d/locks/custom.lock 2013-09-05 19:24:47 +0000
3+++ src/system/custom/etc/dconf_source/db/custom.d/locks/custom.lock 2014-02-12 19:29:13 +0000
4@@ -1,3 +1,2 @@
5-/org/gnome/desktop/background/picture-uri
6 /com/ubuntu/touch/sound/incoming-call-sound
7 /com/ubuntu/touch/sound/incoming-message-sound
8
9=== modified file 'tests/api/test_dconf.py'
10--- tests/api/test_dconf.py 2014-02-05 16:26:04 +0000
11+++ tests/api/test_dconf.py 2014-02-12 19:29:13 +0000
12@@ -1,11 +1,14 @@
13 from autopilot.testcase import AutopilotTestCase
14+from subprocess import check_output
15 import os
16-import subprocess
17 import re
18
19 #: Path to pre-seeded custom dconf database
20 custom_dconf_key = '/custom/etc/dconf/db/custom.d/custom.key'
21
22+#: Path to the list of locked dconf keys
23+custom_lock = '/custom/etc/dconf/db/custom.d/locks/custom.lock'
24+
25 class DconfVerificationTests(AutopilotTestCase):
26
27 os.environ['DCONF_DIR'] = '/custom/etc/dconf'
28@@ -24,7 +27,7 @@
29 ck = open(custom_dconf_key, 'r')
30 except IOError:
31 print "File not found, has this image not been customized?"
32- self.asserttrue(False)
33+ self.assertTrue(False)
34
35 dconf_keys = {}
36
37@@ -48,11 +51,36 @@
38 for key, value in dconf_keys.iteritems():
39 # Properly installed dconf keys should be visible via
40 # `dconf read`
41- dval = subprocess.check_output(['dconf', 'read', key])
42+ dval = check_output(['dconf', 'read', key])
43 try:
44 self.assertEqual(dval, value)
45 except AssertionError:
46- print ""
47- print "ERROR"
48 print "%s does not match. FAILING" % key
49 self.asserttrue(False)
50+
51+ def test_dconf_lockfile(self):
52+ """
53+ In some rare cases, downstreams may wish to ship a customization
54+ and prevent end users from ever changing it. This is done by setting
55+ a dconf lock.
56+
57+ This test verifies that the dconf lock file exists, and that
58+ the contained keys cannot be changed.
59+ """
60+ # Ensure the lockfile exists first
61+ self.assertTrue(os.path.exists(custom_lock))
62+ # The error that should appear when trying to write a locked key
63+ fail_string = "error: The operation attempted to modify "\
64+ "one or more non-writable keys"
65+ # It doesn't matter what the key is set as, it should fail because
66+ # of the lock, so setting the key as a blank string
67+ locked_keyval = "' '"
68+ with open(custom_lock, 'r') as lockfile:
69+ #iterate through list of locked keys and try to change them
70+ for line in lockfile.readlines():
71+ lock_out = check_output(['dconf', 'write', line, locked_keyval])
72+ self.assertIn(fail_string, lock_out)
73+
74+
75+
76+

Subscribers

People subscribed via source and target branches

to all changes: