Merge lp:~kissiel/checkbox/session-storage-renames into lp:checkbox

[Maciej Kisielewski]

This MR brings changes to how sessions are stored in the filesystem.

Instead of using mkdtemp, the directory name that's used for storing session is now generated from two variables - session title and timestamp.

Title is determined by (in order of importance):

1) command line argument (--title)
2) basename of the launcher name
3) front-end name (e.g. checkbox-cli

To test it run checkbox-cli in different ways and observe ~/.config/plainbox/*

8a01902 plainbox:runner: fix typo in docstring
4573b73 plainbox:session: allow custom prefixes for session storage
265e26a plainbox:session: use timestamp in session storage path
4ed79a2 checkbox-ng: use launcher basename as sesion title
530faee checkbox-ng: add --title option for naming sessions

[Pierre Equoy (pieq)]

Apart from the small typo (see below), it looks good to me!

I tried to brutalize it by passing weird characters but it slugifies everything, so it's OK (well, Chinese words end up like a bunch of underscores but I guess the point is to be able to quickly name a session when testing, so it's not a big deal)

review: Needs Fixing

[lp:~kissiel/checkbox/session-storage-renames]

4463. By Launchpad Translations on behalf of checkbox-dev on 2016-08-06

Launchpad automatic translations update.

4464. By Maciej Kisielewski on 2016-08-08

plainbox:runner: fix typo in docstring

Signed-off-by: Maciej Kisielewski <email address hidden>

4465. By Maciej Kisielewski on 2016-08-08

plainbox:session: allow custom prefixes for session storage

Signed-off-by: Maciej Kisielewski <email address hidden>

4466. By Maciej Kisielewski on 2016-08-08

plainbox:session: use timestamp in session storage path

Instead of using mkdtemp that gives somewhat enigmatic session dir names, this
patch makes plainbox use timestamp instead.

session directory is now in the format {prefix}{timestamp}{suffix}, where
prefix is determined by the session title, timestamp is in ISO8601 compliant
format (%Y-%m-%dT%H.%M.%S), and suffix is '.session'.

In the rare case where requested directory would already exist, _({num}) is
inserted between timestamp and suffix. {num} starts at 1.

Signed-off-by: Maciej Kisielewski <email address hidden>

4467. By Maciej Kisielewski on 2016-08-08

checkbox-ng: use launcher basename as sesion title

Signed-off-by: Maciej Kisielewski <email address hidden>

4468. By Maciej Kisielewski on 2016-08-08

checkbox-ng: add --title option for naming sessions

Signed-off-by: Maciej Kisielewski <email address hidden>

[Maciej Kisielewski (kissiel)]

> Apart from the small typo (see below), it looks good to me!
>
> I tried to brutalize it by passing weird characters but it slugifies
> everything, so it's OK (well, Chinese words end up like a bunch of underscores
> but I guess the point is to be able to quickly name a session when testing, so
> it's not a big deal)

All in all there was too much 'all' :)
Fixed and repushed

[Paul Larson (pwlars)]

It looks like you rebased, so it's hard to tell for sure, but I think the only thing that changed since the last push was the typo that epierre mentioned. So +1

review: Approve

[Pierre Equoy (pieq)]

LGTM!

review: Approve

=== modified file 'checkbox-ng/launchers/checkbox-cli'
--- checkbox-ng/launchers/checkbox-cli	2016-08-03 18:27:47 +0000
+++ checkbox-ng/launchers/checkbox-cli	2016-08-08 08:25:58 +0000
@@ -339,6 +339,10 @@
     def _start_new_session(self):
         print(_("Preparing..."))
         title = self.launcher.app_id
+        if self.ctx.args.title:
+            title = self.ctx.args.title
+        elif self.ctx.args.launcher:
+            title = os.path.basename(self.ctx.args.launcher)
         if self.launcher.app_version:
             title += ' {}'.format(self.launcher.app_version)
         self.ctx.sa.start_new_session(title)
@@ -813,6 +817,9 @@
         parser.add_argument(
             '--verify', action='store_true',
             help=_('only validate the launcher'))
+        parser.add_argument(
+            '--title', action='store', metavar='SESSION_NAME',
+            help=_('title of the session to use'))
 
 
 if __name__ == '__main__':

=== modified file 'plainbox/plainbox/impl/runner.py'
--- plainbox/plainbox/impl/runner.py	2016-07-12 11:47:54 +0000
+++ plainbox/plainbox/impl/runner.py	2016-08-08 08:25:58 +0000
@@ -51,7 +51,7 @@
 
 
 def slugify(_string):
-    """Transform any string to onet that can be used in filenames."""
+    """Transform any string to one that can be used in filenames."""
     valid_chars = frozenset(
         "-_.{}{}".format(string.ascii_letters, string.digits))
     return ''.join(c if c in valid_chars else '_' for c in _string)

=== modified file 'plainbox/plainbox/impl/session/assistant.py'
--- plainbox/plainbox/impl/session/assistant.py	2016-07-04 13:37:03 +0000
+++ plainbox/plainbox/impl/session/assistant.py	2016-08-08 08:25:58 +0000
@@ -561,7 +561,7 @@
         methods to see if session should be resumed instead.
         """
         UsageExpectation.of(self).enforce()
-        self._manager = SessionManager.create(self._repo)
+        self._manager = SessionManager.create(self._repo, prefix=title + '-')
         self._context = self._manager.add_local_device_context()
         for provider in self._selected_providers:
             if provider.problem_list:

=== modified file 'plainbox/plainbox/impl/session/manager.py'
--- plainbox/plainbox/impl/session/manager.py	2016-07-25 17:55:34 +0000
+++ plainbox/plainbox/impl/session/manager.py	2016-08-08 08:25:58 +0000
@@ -187,7 +187,7 @@
             return self.default_device_context.state
 
     @classmethod
-    def create(cls, repo=None, legacy_mode=False):
+    def create(cls, repo=None, legacy_mode=False, prefix='pbox-'):
         """
         Create an empty session manager.
 
@@ -217,7 +217,7 @@
         logger.debug("SessionManager.create()")
         if repo is None:
             repo = SessionStorageRepository()
-        storage = SessionStorage.create(repo.location, legacy_mode)
+        storage = SessionStorage.create(repo.location, legacy_mode, prefix)
         WellKnownDirsHelper(storage).populate()
         return cls([], storage)
 

=== modified file 'plainbox/plainbox/impl/session/storage.py'
--- plainbox/plainbox/impl/session/storage.py	2015-08-28 10:25:58 +0000
+++ plainbox/plainbox/impl/session/storage.py	2016-08-08 08:25:58 +0000
@@ -28,6 +28,7 @@
 associated with a particular session.
 """
 
+import datetime
 import errno
 import logging
 import os
@@ -37,6 +38,7 @@
 import tempfile
 
 from plainbox.i18n import gettext as _, ngettext
+from plainbox.impl.runner import slugify
 
 logger = logging.getLogger("plainbox.session.storage")
 
@@ -249,7 +251,7 @@
         return os.path.join(self._location, self._SESSION_FILE)
 
     @classmethod
-    def create(cls, base_dir, legacy_mode=False):
+    def create(cls, base_dir, legacy_mode=False, prefix='pbox-'):
         """
         Create a new :class:`SessionStorage` in a random subdirectory
         of the specified base directory. The base directory is also
@@ -264,6 +266,10 @@
             If False (defaults to True) then the caller is expected to
             handle multiple sessions by itself.
 
+        :param prefix:
+            String which should prefix all session filenames. The prefix is
+            sluggified before use.
+
         .. note::
             Legacy mode is where applications using PlainBox API can only
             handle one session. Creating another session replaces whatever was
@@ -278,8 +284,18 @@
         """
         if not os.path.exists(base_dir):
             os.makedirs(base_dir)
-        location = tempfile.mkdtemp(
-            prefix='pbox-', suffix='.session', dir=base_dir)
+        isoformat = "%Y-%m-%dT%H.%M.%S"
+        timestamp = datetime.datetime.utcnow().strftime(isoformat)
+        location = os.path.join(base_dir, "{prefix}{timestamp}{suffix}".format(
+            prefix=slugify(prefix), timestamp=timestamp, suffix='.session'))
+        uniq = 1
+        while os.path.exists(location):
+            location = os.path.join(
+                base_dir, "{prefix}{timestamp}_({uniq}){suffix}".format(
+                    prefix=slugify(prefix), timestamp=timestamp,
+                    uniq=uniq, suffix='.session'))
+            uniq += 1
+        os.mkdir(location)
         logger.debug(_("Created new storage in %r"), location)
         self = cls(location)
         if legacy_mode:

=== modified file 'plainbox/plainbox/impl/session/test_manager.py'
--- plainbox/plainbox/impl/session/test_manager.py	2015-06-09 18:25:56 +0000
+++ plainbox/plainbox/impl/session/test_manager.py	2016-08-08 08:25:58 +0000
@@ -148,7 +148,8 @@
         repo = mocks['SessionStorageRepository']()
         # Ensure that a storage was created, with repository location and
         # without legacy mode turned on
-        mocks['SessionStorage'].create.assert_called_with(repo.location, False)
+        mocks['SessionStorage'].create.assert_called_with(
+            repo.location, False, 'pbox-')
         storage = mocks['SessionStorage'].create()
         # Ensure that a default directories were created
         mocks['WellKnownDirsHelper'].assert_called_with(storage)