Use JSON as pagekite action-script arguments

This allows to safe some conversions

actions/pagekite

@@ -25,12 +25,13 @@ Unfortunately there is no python3 package for augeas yet
 
 import argparse
 import augeas
+import json
 import os
 import subprocess
 
 import util
-from pagekite_util import SERVICE_PARAMS, convert_to_service, \
+from pagekite_util import SERVICE_PARAMS, convert_service_to_string, \
-    convert_service_to_string, get_augeas_servicefile_path, CONF_PATH
+    get_augeas_servicefile_path, load_service, CONF_PATH
 
 aug = augeas.Augeas()
 
@@ -75,11 +76,10 @@ def parse_arguments():
     subparsers.add_parser('get-services', help='Get list of enabled services')
     add_service = subparsers.add_parser('add-service',
                                         help='Add a pagekite service')
-    add_service.add_argument('--service', help='":"-separated service string')
+    add_service.add_argument('--service', help='json service dictionary')
     remove_service = subparsers.add_parser('remove-service',
                                            help='Remove a pagekite service')
-    remove_service.add_argument('--service', help='":"-separated service \
+    remove_service.add_argument('--service', help='json service dictionary')
-                                string')
 
     return parser.parse_args()
 
@@ -150,17 +150,18 @@ def enable_pagekitenet_frontend():
 def subcommand_get_services(arguments):
     """ lists all available (enabled) services """
     for match in aug.match(PATHS['service_on']):
-        print ":".join([aug.get(os.path.join(match, param)) for param in
+        service = dict([(param, aug.get(os.path.join(match, param)))
-                        SERVICE_PARAMS])
+                        for param in SERVICE_PARAMS])
+        print json.dumps(service)
 
 
 def subcommand_remove_service(arguments):
     """Searches and removes the service(s) that match all given parameters"""
-    service = convert_to_service(arguments.service)
+    service = load_service(arguments.service)
     paths = get_existing_service_paths(service)
     # TODO: theoretically, everything to do here is:
     # [aug.remove(path) for path in paths]
-    # but augeas won't let you save the changed files, and won't tell you why
+    # but augeas won't let you save the changed files and doesn't say why
     for path in paths:
         filepath = convert_augeas_path_to_filepath(path)
         service_found = False
@@ -185,14 +186,14 @@ def get_existing_service_paths(service):
     # construct an augeas query path with patterns like:
     #     */service_on/*[protocol='http']
     path = PATHS['service_on']
-    for key, value in service.items():
+    for param, value in service.items():
-        path += "[%s='%s']" % (key, value)
+        path += "[%s='%s']" % (param, value)
     return aug.match(path)
 
 
 def subcommand_add_service(arguments):
     """Add one service"""
-    service = convert_to_service(arguments.service)
+    service = load_service(arguments.service)
     if get_existing_service_paths(service):
         msg = "Service with the parameters %s already exists"
         raise RuntimeError(msg % service)

actions/pagekite_util.py

@@ -24,34 +24,20 @@ Utilities for configuring PageKite.
 # Once python-augeas is available for python3 import the following things
 # from plinth.modules.pagekite.util (instead of having a copy in here):
 #
-# SERVICE_PARAMS, convert_service_to_string, convert_to_service
+# SERVICE_PARAMS, convert_service_to_string
 #
 # until then, this file is python2 and python3 compatible for the unittests
 
 import os
+import json
+
 CONF_PATH = '/files/etc/pagekite.d'
 
+# parameters that get stored in configuration service_on entries
 SERVICE_PARAMS = ['protocol', 'kitename', 'backend_host', 'backend_port',
                   'secret']
 
 
-def convert_to_service(service_string):
-    """ Convert a service string into a service parameter dictionary
-    >>> convert_to_service('https/443:@kitename:localhost:443:@kitesecret')
-    {'kitename': '@kitename', 'backend_host': 'localhost', \
-'secret': '@kitesecret', 'protocol': 'https/443', 'backend_port': '443'}
-    """
-    try:
-        params = dict(zip(SERVICE_PARAMS, service_string.split(':')))
-    except Exception:
-        msg = """params are expected to be a ':'-separated string containing
-              values for: %s , for example:\n"--params
-              http/8000:@kitename:localhost:8000:@kitesecret"
-              """
-        raise ValueError(msg % ", ".join(SERVICE_PARAMS))
-    return params
-
-
 def convert_service_to_string(service):
     """ Convert service dict into a ":"-separated parameter string
 
@@ -61,17 +47,27 @@ def convert_service_to_string(service):
     'https/443:@kitename:localhost:443:@kitesecret'
     """
     try:
-        service_string = ":".join([str(service[param]) for param in
+        service_string = ":".join([service[param] for param in SERVICE_PARAMS])
-                                  SERVICE_PARAMS])
     except KeyError:
         raise ValueError("Could not parse params: %s " % service)
     return service_string
 
 
+def load_service(json_service):
+    """ create a service out of json command-line argument
+
+    1) parse json
+    2) only use the parameters that we need (SERVICE_PARAMS)
+    3) convert unicode to strings
+    """
+    service = json.loads(json_service)
+    return dict((str(key), str(service[key])) for key in SERVICE_PARAMS)
+
+
 def get_augeas_servicefile_path(protocol):
     """Get the augeas path where a service for a protocol should be stored
 
-    TODO: Use doctests instead of unittests until we can use python3.
+    TODO: Once we use python3 switch from doctests to unittests
 
     >>> get_augeas_servicefile_path('http')
     '/files/etc/pagekite.d/80_http.rc/service_on'

plinth/modules/pagekite/forms.py

@@ -15,17 +15,18 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+import copy
 from gettext import gettext as _
+import json
 import logging
 
-import copy
 from django import forms
 from django.contrib import messages
 from django.core import validators
 
 from plinth.errors import ActionError
-from .util import _run, convert_service_to_string, get_kite_details, \
+from .util import _run, get_kite_details, BACKEND_HOST, KITE_NAME, \
-    BACKEND_HOST, KITE_NAME, KITE_SECRET, PREDEFINED_SERVICES
+    KITE_SECRET, PREDEFINED_SERVICES
 
 LOGGER = logging.getLogger(__name__)
 
@@ -71,6 +72,7 @@ for your account if no secret is set on the kite'))
         if old != new:
 
             config_changed = False
+
             if old['kite_name'] != new['kite_name'] or \
                     old['kite_secret'] != new['kite_secret']:
                 _run(['set-kite', '--kite-name', new['kite_name'],
@@ -92,7 +94,7 @@ for your account if no secret is set on the kite'))
                     messages.success(request, _('PageKite disabled'))
 
             # Restart the service if the config was changed while the service
-            # was running, so the changes take effect.
+            # was running, so changes take effect immediately.
             elif config_changed and new['enabled']:
                 _run(['restart'])
 
@@ -118,13 +120,13 @@ class DefaultServiceForm(forms.Form):
         for service_name in PREDEFINED_SERVICES.keys():
             if self.initial[service_name] != formdata[service_name]:
                 service = PREDEFINED_SERVICES[service_name]['params']
-                service_string = convert_service_to_string(service)
+                service = json.dumps(service)
                 if formdata[service_name]:
-                    _run(['add-service', '--service', service_string])
+                    _run(['add-service', '--service', service])
                     messages.success(request, _('Service enabled: {name}')
                                      .format(name=service_name))
                 else:
-                    _run(['remove-service', '--service', service_string])
+                    _run(['remove-service', '--service', service])
                     messages.success(request, _('Service disabled: {name}')
                                      .format(name=service_name))
 
@@ -170,8 +172,7 @@ class DeleteCustomServiceForm(BaseCustomServiceForm):
 
     def delete(self, request):
         service = self.convert_formdata_to_service(self.cleaned_data)
-        service_string = convert_service_to_string(service)
+        _run(['remove-service', '--service', json.dumps(service)])
-        _run(['remove-service', '--service', service_string])
         messages.success(request, _('Deleted custom service'))
 
 
@@ -213,9 +214,8 @@ class AddCustomServiceForm(BaseCustomServiceForm):
 
     def save(self, request):
         service = self.convert_formdata_to_service(self.cleaned_data)
-        service_string = convert_service_to_string(service)
         try:
-            _run(['add-service', '--service', service_string])
+            _run(['add-service', '--service', json.dumps(service)])
             messages.success(request, _('Added custom service'))
         except ActionError as exception:
             if "already exists" in str(exception):

plinth/modules/pagekite/util.py

@@ -16,6 +16,7 @@
 #
 
 from gettext import gettext as _
+import json
 import logging
 
 from plinth import actions
@@ -70,44 +71,6 @@ PREDEFINED_SERVICES = {
 }
 
 
-def convert_to_service(service_string):
-    """ Convert a service string into a service parameter dictionary
-    >>> input = 'https/443:@kitename:localhost:443:@kitesecret'
-    >>> output = convert_to_service(input)
-    >>> expected_output = {'secret': '@kitesecret',
-    ...     'backend_host': 'localhost', 'kitename': '@kitename',
-    ...     'backend_port': '443', 'protocol': 'https/443'}
-    ...
-    >>> output == expected_output
-    True
-    """
-    try:
-        params = dict(zip(SERVICE_PARAMS, service_string.split(':')))
-    except Exception:
-        msg = """params are expected to be a ':'-separated string containing
-              values for: %s , for example:\n"--params
-              http/8000:@kitename:localhost:8000:@kitesecret"
-              """
-        raise ValueError(msg % ", ".join(SERVICE_PARAMS))
-    return params
-
-
-def convert_service_to_string(service):
-    """ Convert service dict into a ":"-separated parameter string
-
-    >>> convert_service_to_string({'kitename': '@kitename', \
-'backend_host': 'localhost', 'secret': '@kitesecret', \
-'protocol': 'https/443', 'backend_port': '443'})
-    'https/443:@kitename:localhost:443:@kitesecret'
-    """
-    try:
-        service_string = ":".join([str(service[param]) for param in
-                                  SERVICE_PARAMS])
-    except KeyError:
-        raise ValueError("Could not parse params: %s " % service)
-    return service_string
-
-
 def get_kite_details():
     output = _run(['get-kite'])
     kite_details = output.split()
@@ -147,8 +110,11 @@ def get_pagekite_services():
     # set all predefined services to 'disabled' by default
     [predefined.update({proto: False}) for proto in PREDEFINED_SERVICES.keys()]
     # now, search for the enabled ones
-    for serviceline in _run(['get-services']).split():
+    for serviceline in _run(['get-services']).split('\n'):
-        service = convert_to_service(serviceline)
+        if not serviceline:  # skip empty lines
+            continue
+
+        service = json.loads(serviceline)
         for name, predefined_service in PREDEFINED_SERVICES.items():
             if service == predefined_service['params']:
                 predefined[name] = True
@@ -179,8 +145,3 @@ def _run(arguments, superuser=True):
         return actions.superuser_run(command, arguments)
     else:
         return actions.run(command, arguments)
-
-
-if __name__ == "__main__":
-    import doctest
-    doctest.testmod()