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

[Benji York]

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

[Adam Collard (adam-collard)]

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)]

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.

[Benji York (benji)]

I've posted the requested words at https://docs.google.com/document/d/1NT5VjG7CwN406ht2PVj5Dn4U5NgqBmI_Dh34xOW1UUs/edit

[Chad Smith (chad.smith)]

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)]

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

[Adam Collard (adam-collard)]

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

=== modified file 'landscape/log.py'
--- landscape/log.py	2008-06-10 10:56:01 +0000
+++ landscape/log.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import inspect
 import logging
 
@@ -31,6 +33,7 @@
         percent = 0.0
     return "%.02f%%" % float(percent)
 
+
 def rotate_logs():
     """
     This closes and reopens the underlying files in the logging module's

=== modified file 'landscape/manager/aptsources.py'
--- landscape/manager/aptsources.py	2013-04-01 09:25:31 +0000
+++ landscape/manager/aptsources.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import glob
 import os
 import pwd

=== modified file 'landscape/manager/config.py'
--- landscape/manager/config.py	2013-10-24 01:54:07 +0000
+++ landscape/manager/config.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import os
 
 from landscape.deployment import Configuration

=== modified file 'landscape/manager/customgraph.py'
--- landscape/manager/customgraph.py	2013-05-21 14:18:29 +0000
+++ landscape/manager/customgraph.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import os
 import time
 import logging
@@ -84,9 +86,9 @@
     def register(self, registry):
         super(CustomGraphPlugin, self).register(registry)
         registry.register_message(
-            "custom-graph-add", self._handle_custom_graph_add)
+            b"custom-graph-add", self._handle_custom_graph_add)
         registry.register_message(
-            "custom-graph-remove", self._handle_custom_graph_remove)
+            b"custom-graph-remove", self._handle_custom_graph_remove)
         self._persist = StoreProxy(self.registry.store)
         self._accumulate = Accumulator(self._persist, self.run_interval)
 
@@ -153,15 +155,17 @@
                 if os.path.isfile(filename):
                     script_hash = self._get_script_hash(filename)
                     self._data[graph_id] = {
-                        "values": [], "error": u"", "script-hash": script_hash}
+                        b"values": [],
+                        b"error": u"",
+                        b"script-hash": script_hash}
 
-        message = {"type": self.message_type, "data": self._data}
+        message = {b"type": self.message_type, b"data": self._data}
 
         new_data = {}
         for graph_id, item in self._data.iteritems():
-            script_hash = item["script-hash"]
+            script_hash = item[b"script-hash"]
             new_data[graph_id] = {
-                "values": [], "error": u"", "script-hash": script_hash}
+                b"values": [], b"error": u"", b"script-hash": script_hash}
         self._data = new_data
 
         self.registry.broker.send_message(message, self._session_id,
@@ -227,12 +231,12 @@
             if os.path.isfile(filename):
                 script_hash = self._get_script_hash(filename)
             else:
-                script_hash = ""
+                script_hash = b""
             if graph_id not in self._data:
                 self._data[graph_id] = {
-                    "values": [], "error": u"", "script-hash": script_hash}
+                    b"values": [], b"error": u"", b"script-hash": script_hash}
             else:
-                self._data[graph_id]["script-hash"] = script_hash
+                self._data[graph_id][b"script-hash"] = script_hash
             try:
                 uid, gid, path = get_user_info(user)
             except UnknownUserError, e:

=== modified file 'landscape/manager/fakepackagemanager.py'
--- landscape/manager/fakepackagemanager.py	2013-05-21 14:18:29 +0000
+++ landscape/manager/fakepackagemanager.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import random
 
 from landscape.manager.plugin import ManagerPlugin

=== modified file 'landscape/manager/hardwareinfo.py'
--- landscape/manager/hardwareinfo.py	2013-05-17 22:48:40 +0000
+++ landscape/manager/hardwareinfo.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import os
 
 from twisted.internet.utils import getProcessOutput

=== modified file 'landscape/manager/haservice.py'
--- landscape/manager/haservice.py	2013-07-23 10:00:52 +0000
+++ landscape/manager/haservice.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import logging
 import os
 

=== modified file 'landscape/manager/keystonetoken.py'
--- landscape/manager/keystonetoken.py	2013-07-11 20:27:10 +0000
+++ landscape/manager/keystonetoken.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import os
 import logging
 from ConfigParser import ConfigParser, NoOptionError

=== modified file 'landscape/manager/manager.py'
--- landscape/manager/manager.py	2010-04-23 14:42:21 +0000
+++ landscape/manager/manager.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 from landscape.manager.store import ManagerStore
 from landscape.broker.client import BrokerClient
 

=== modified file 'landscape/manager/packagemanager.py'
--- landscape/manager/packagemanager.py	2013-03-05 02:53:38 +0000
+++ landscape/manager/packagemanager.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import logging
 import os
 

=== modified file 'landscape/manager/plugin.py'
--- landscape/manager/plugin.py	2013-05-08 22:18:47 +0000
+++ landscape/manager/plugin.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 from twisted.internet.defer import maybeDeferred
 
 from landscape.lib.log import log_failure

=== modified file 'landscape/manager/processkiller.py'
--- landscape/manager/processkiller.py	2010-04-26 15:19:55 +0000
+++ landscape/manager/processkiller.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import os
 import signal
 import logging

=== modified file 'landscape/manager/scriptexecution.py'
--- landscape/manager/scriptexecution.py	2013-05-17 22:32:49 +0000
+++ landscape/manager/scriptexecution.py	2016-06-02 19:46:38 +0000
@@ -1,8 +1,8 @@
-"""
-Functionality for running arbitrary shell scripts.
+"""Functionality for running arbitrary shell scripts.
 
 @var ALL_USERS: A token indicating all users should be allowed.
 """
+
 import os
 import pwd
 import tempfile

=== modified file 'landscape/manager/service.py'
--- landscape/manager/service.py	2013-05-07 22:47:06 +0000
+++ landscape/manager/service.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 from twisted.python.reflect import namedClass
 
 from landscape.service import LandscapeService, run_landscape_service

=== modified file 'landscape/manager/shutdownmanager.py'
--- landscape/manager/shutdownmanager.py	2013-05-17 22:45:29 +0000
+++ landscape/manager/shutdownmanager.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import logging
 
 from twisted.internet.defer import Deferred

=== modified file 'landscape/manager/store.py'
--- landscape/manager/store.py	2008-10-22 16:54:38 +0000
+++ landscape/manager/store.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 try:
     import sqlite3
 except ImportError:
@@ -35,7 +37,8 @@
                 (filename, user, graph_id))
         else:
             cursor.execute(
-                "INSERT INTO graph (graph_id, filename, user) VALUES (?, ?, ?)",
+                "INSERT INTO graph (graph_id, filename, user) "
+                    "VALUES (?, ?, ?)",
                 (graph_id, filename, user))
 
     @with_cursor

=== modified file 'landscape/manager/tests/test_usermanager.py'
--- landscape/manager/tests/test_usermanager.py	2013-07-19 09:12:16 +0000
+++ landscape/manager/tests/test_usermanager.py	2016-06-02 19:46:38 +0000
@@ -175,7 +175,7 @@
             self.assertMessages(messages,
                                 [{"type": "operation-result", "status": FAILED,
                                   "operation-id": 123, "timestamp": 0,
-                                  "result-text": "KeyError: 'username'"}])
+                                  "result-text": "KeyError: u'username'"}])
 
         self.setup_environment([], [], None)
 

=== modified file 'landscape/manager/usermanager.py'
--- landscape/manager/usermanager.py	2013-05-07 23:06:56 +0000
+++ landscape/manager/usermanager.py	2016-06-02 19:46:38 +0000
@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import logging
 
 from landscape.lib.encoding import encode_dict_if_needed