Merge lp:~benji/landscape-client/try-unicode into lp:~landscape/landscape-client/trunk

Proposed by Benji York on 2016-06-02
Status: Rejected
Rejected by: Benji York on 2016-06-07
Proposed branch: lp:~benji/landscape-client/try-unicode
Merge into: lp:~landscape/landscape-client/trunk
Diff against target: 255 lines (+49/-13)
18 files modified
landscape/log.py (+3/-0)
landscape/manager/aptsources.py (+2/-0)
landscape/manager/config.py (+2/-0)
landscape/manager/customgraph.py (+13/-9)
landscape/manager/fakepackagemanager.py (+2/-0)
landscape/manager/hardwareinfo.py (+2/-0)
landscape/manager/haservice.py (+2/-0)
landscape/manager/keystonetoken.py (+2/-0)
landscape/manager/manager.py (+2/-0)
landscape/manager/packagemanager.py (+2/-0)
landscape/manager/plugin.py (+2/-0)
landscape/manager/processkiller.py (+2/-0)
landscape/manager/scriptexecution.py (+2/-2)
landscape/manager/service.py (+2/-0)
landscape/manager/shutdownmanager.py (+2/-0)
landscape/manager/store.py (+4/-1)
landscape/manager/tests/test_usermanager.py (+1/-1)
landscape/manager/usermanager.py (+2/-0)
To merge this branch: bzr merge lp:~benji/landscape-client/try-unicode
Reviewer Review Type Date Requested Status
Adam Collard (community) 2016-06-02 Disapprove on 2016-06-07
Chad Smith 2016-06-02 Pending
Review via email: mp+296360@code.launchpad.net

Description of the change

This branch begins chipping away at converting the client codebase to
using unicode and byte strings so as to be compatible with Python 3.

This branch converts all of the modules in the landscape/managers
directory. Many survived with only a one-line change to add a from
__future__ import, but others required more work.

Testing instructions:

make check
start the client and register it

To post a comment you must log in.
Adam Collard (adam-collard) wrote :

Hmm, toggling unicode_literals feels a bit dangerous.

There is some discussion about the pro's and con's at http://python-future.org/unicode_literals.html .

Can we put some words on paper (read: Google doc) as to why we took this approach, what the risks are etc.? Just make the case for unicode_literals as opposed to string-by-string u"" or b""

review: Needs Information
Free Ekanayaka (free.ekanayaka) wrote :

FWIW I think it's a safe approach because afaict landscape.schema will make sure that expectations on the data format that gets transferred on the wire are met.

Chad Smith (chad.smith) wrote :

Hrm, well something may be falling over I was unable to see package reports coming from a registered client with these changes against advicedog. I'll try to see if I can get a second registration against an OnPremises deployment in bstack to confirm.

Chad Smith (chad.smith) wrote :

Code changes themselves look clean. And I appreciate the Gdoc thank you.

Adam Collard (adam-collard) wrote :

This branch makes me super uncomfortable. I'm doubling down on my "this feels a bit dangerous". We are changing the code without touching the tests: which means that our tests are using bytes (AKA Python2's str) but our real code is using unicode (AKA Python3's str).

The code in landscape-client was not written with the expectation that string literals are Unicode. This doesn't just affect the on-the-wire format (as Free mentions above) but all interactions with the filesystem, sub-processes, etc.

I think we have to bite the bullet and just accept that we have to take the time to identify what is textual data and what is binary (and mark up literals with u"" and b"" as necessary)

review: Disapprove

Unmerged revisions

844. By Benji York on 2016-06-02

fix lint

843. By Benji York on 2016-06-02

checkpoint - tests passing

842. By Benji York on 2016-06-02

checkpoint - tests passing

841. By Benji York on 2016-06-02

checkpoint - tests passing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'landscape/log.py'
2--- landscape/log.py 2008-06-10 10:56:01 +0000
3+++ landscape/log.py 2016-06-02 19:46:38 +0000
4@@ -1,3 +1,5 @@
5+from __future__ import unicode_literals
6+
7 import inspect
8 import logging
9
10@@ -31,6 +33,7 @@
11 percent = 0.0
12 return "%.02f%%" % float(percent)
13
14+
15 def rotate_logs():
16 """
17 This closes and reopens the underlying files in the logging module's
18
19=== modified file 'landscape/manager/aptsources.py'
20--- landscape/manager/aptsources.py 2013-04-01 09:25:31 +0000
21+++ landscape/manager/aptsources.py 2016-06-02 19:46:38 +0000
22@@ -1,3 +1,5 @@
23+from __future__ import unicode_literals
24+
25 import glob
26 import os
27 import pwd
28
29=== modified file 'landscape/manager/config.py'
30--- landscape/manager/config.py 2013-10-24 01:54:07 +0000
31+++ landscape/manager/config.py 2016-06-02 19:46:38 +0000
32@@ -1,3 +1,5 @@
33+from __future__ import unicode_literals
34+
35 import os
36
37 from landscape.deployment import Configuration
38
39=== modified file 'landscape/manager/customgraph.py'
40--- landscape/manager/customgraph.py 2013-05-21 14:18:29 +0000
41+++ landscape/manager/customgraph.py 2016-06-02 19:46:38 +0000
42@@ -1,3 +1,5 @@
43+from __future__ import unicode_literals
44+
45 import os
46 import time
47 import logging
48@@ -84,9 +86,9 @@
49 def register(self, registry):
50 super(CustomGraphPlugin, self).register(registry)
51 registry.register_message(
52- "custom-graph-add", self._handle_custom_graph_add)
53+ b"custom-graph-add", self._handle_custom_graph_add)
54 registry.register_message(
55- "custom-graph-remove", self._handle_custom_graph_remove)
56+ b"custom-graph-remove", self._handle_custom_graph_remove)
57 self._persist = StoreProxy(self.registry.store)
58 self._accumulate = Accumulator(self._persist, self.run_interval)
59
60@@ -153,15 +155,17 @@
61 if os.path.isfile(filename):
62 script_hash = self._get_script_hash(filename)
63 self._data[graph_id] = {
64- "values": [], "error": u"", "script-hash": script_hash}
65+ b"values": [],
66+ b"error": u"",
67+ b"script-hash": script_hash}
68
69- message = {"type": self.message_type, "data": self._data}
70+ message = {b"type": self.message_type, b"data": self._data}
71
72 new_data = {}
73 for graph_id, item in self._data.iteritems():
74- script_hash = item["script-hash"]
75+ script_hash = item[b"script-hash"]
76 new_data[graph_id] = {
77- "values": [], "error": u"", "script-hash": script_hash}
78+ b"values": [], b"error": u"", b"script-hash": script_hash}
79 self._data = new_data
80
81 self.registry.broker.send_message(message, self._session_id,
82@@ -227,12 +231,12 @@
83 if os.path.isfile(filename):
84 script_hash = self._get_script_hash(filename)
85 else:
86- script_hash = ""
87+ script_hash = b""
88 if graph_id not in self._data:
89 self._data[graph_id] = {
90- "values": [], "error": u"", "script-hash": script_hash}
91+ b"values": [], b"error": u"", b"script-hash": script_hash}
92 else:
93- self._data[graph_id]["script-hash"] = script_hash
94+ self._data[graph_id][b"script-hash"] = script_hash
95 try:
96 uid, gid, path = get_user_info(user)
97 except UnknownUserError, e:
98
99=== modified file 'landscape/manager/fakepackagemanager.py'
100--- landscape/manager/fakepackagemanager.py 2013-05-21 14:18:29 +0000
101+++ landscape/manager/fakepackagemanager.py 2016-06-02 19:46:38 +0000
102@@ -1,3 +1,5 @@
103+from __future__ import unicode_literals
104+
105 import random
106
107 from landscape.manager.plugin import ManagerPlugin
108
109=== modified file 'landscape/manager/hardwareinfo.py'
110--- landscape/manager/hardwareinfo.py 2013-05-17 22:48:40 +0000
111+++ landscape/manager/hardwareinfo.py 2016-06-02 19:46:38 +0000
112@@ -1,3 +1,5 @@
113+from __future__ import unicode_literals
114+
115 import os
116
117 from twisted.internet.utils import getProcessOutput
118
119=== modified file 'landscape/manager/haservice.py'
120--- landscape/manager/haservice.py 2013-07-23 10:00:52 +0000
121+++ landscape/manager/haservice.py 2016-06-02 19:46:38 +0000
122@@ -1,3 +1,5 @@
123+from __future__ import unicode_literals
124+
125 import logging
126 import os
127
128
129=== modified file 'landscape/manager/keystonetoken.py'
130--- landscape/manager/keystonetoken.py 2013-07-11 20:27:10 +0000
131+++ landscape/manager/keystonetoken.py 2016-06-02 19:46:38 +0000
132@@ -1,3 +1,5 @@
133+from __future__ import unicode_literals
134+
135 import os
136 import logging
137 from ConfigParser import ConfigParser, NoOptionError
138
139=== modified file 'landscape/manager/manager.py'
140--- landscape/manager/manager.py 2010-04-23 14:42:21 +0000
141+++ landscape/manager/manager.py 2016-06-02 19:46:38 +0000
142@@ -1,3 +1,5 @@
143+from __future__ import unicode_literals
144+
145 from landscape.manager.store import ManagerStore
146 from landscape.broker.client import BrokerClient
147
148
149=== modified file 'landscape/manager/packagemanager.py'
150--- landscape/manager/packagemanager.py 2013-03-05 02:53:38 +0000
151+++ landscape/manager/packagemanager.py 2016-06-02 19:46:38 +0000
152@@ -1,3 +1,5 @@
153+from __future__ import unicode_literals
154+
155 import logging
156 import os
157
158
159=== modified file 'landscape/manager/plugin.py'
160--- landscape/manager/plugin.py 2013-05-08 22:18:47 +0000
161+++ landscape/manager/plugin.py 2016-06-02 19:46:38 +0000
162@@ -1,3 +1,5 @@
163+from __future__ import unicode_literals
164+
165 from twisted.internet.defer import maybeDeferred
166
167 from landscape.lib.log import log_failure
168
169=== modified file 'landscape/manager/processkiller.py'
170--- landscape/manager/processkiller.py 2010-04-26 15:19:55 +0000
171+++ landscape/manager/processkiller.py 2016-06-02 19:46:38 +0000
172@@ -1,3 +1,5 @@
173+from __future__ import unicode_literals
174+
175 import os
176 import signal
177 import logging
178
179=== modified file 'landscape/manager/scriptexecution.py'
180--- landscape/manager/scriptexecution.py 2013-05-17 22:32:49 +0000
181+++ landscape/manager/scriptexecution.py 2016-06-02 19:46:38 +0000
182@@ -1,8 +1,8 @@
183-"""
184-Functionality for running arbitrary shell scripts.
185+"""Functionality for running arbitrary shell scripts.
186
187 @var ALL_USERS: A token indicating all users should be allowed.
188 """
189+
190 import os
191 import pwd
192 import tempfile
193
194=== modified file 'landscape/manager/service.py'
195--- landscape/manager/service.py 2013-05-07 22:47:06 +0000
196+++ landscape/manager/service.py 2016-06-02 19:46:38 +0000
197@@ -1,3 +1,5 @@
198+from __future__ import unicode_literals
199+
200 from twisted.python.reflect import namedClass
201
202 from landscape.service import LandscapeService, run_landscape_service
203
204=== modified file 'landscape/manager/shutdownmanager.py'
205--- landscape/manager/shutdownmanager.py 2013-05-17 22:45:29 +0000
206+++ landscape/manager/shutdownmanager.py 2016-06-02 19:46:38 +0000
207@@ -1,3 +1,5 @@
208+from __future__ import unicode_literals
209+
210 import logging
211
212 from twisted.internet.defer import Deferred
213
214=== modified file 'landscape/manager/store.py'
215--- landscape/manager/store.py 2008-10-22 16:54:38 +0000
216+++ landscape/manager/store.py 2016-06-02 19:46:38 +0000
217@@ -1,3 +1,5 @@
218+from __future__ import unicode_literals
219+
220 try:
221 import sqlite3
222 except ImportError:
223@@ -35,7 +37,8 @@
224 (filename, user, graph_id))
225 else:
226 cursor.execute(
227- "INSERT INTO graph (graph_id, filename, user) VALUES (?, ?, ?)",
228+ "INSERT INTO graph (graph_id, filename, user) "
229+ "VALUES (?, ?, ?)",
230 (graph_id, filename, user))
231
232 @with_cursor
233
234=== modified file 'landscape/manager/tests/test_usermanager.py'
235--- landscape/manager/tests/test_usermanager.py 2013-07-19 09:12:16 +0000
236+++ landscape/manager/tests/test_usermanager.py 2016-06-02 19:46:38 +0000
237@@ -175,7 +175,7 @@
238 self.assertMessages(messages,
239 [{"type": "operation-result", "status": FAILED,
240 "operation-id": 123, "timestamp": 0,
241- "result-text": "KeyError: 'username'"}])
242+ "result-text": "KeyError: u'username'"}])
243
244 self.setup_environment([], [], None)
245
246
247=== modified file 'landscape/manager/usermanager.py'
248--- landscape/manager/usermanager.py 2013-05-07 23:06:56 +0000
249+++ landscape/manager/usermanager.py 2016-06-02 19:46:38 +0000
250@@ -1,3 +1,5 @@
251+from __future__ import unicode_literals
252+
253 import logging
254
255 from landscape.lib.encoding import encode_dict_if_needed

Subscribers

People subscribed via source and target branches

to all changes: