Merge lp:~bac/charmworldlib/check-constraints into lp:charmworldlib/0.2

[Brad Crittenden]

Add parse_constraints and check_constraints.

Also, removes the dependency on ez_setup since it caused the package to not
build and was unnecessary. There is a real system dependency on
python-virtualenv which install python-setuptools, so setuptools will always
be available and ez_setup not required.

https://codereview.appspot.com/69430043/

[Brad Crittenden (bac)]

Reviewers: mp+208669_code.launchpad.net,

Message:
Please take a look.

Description:
Add parse_constraints and check_constraints.

Also, removes the dependency on ez_setup since it caused the package to
not
build and was unnecessary. There is a real system dependency on
python-virtualenv which install python-setuptools, so setuptools will
always
be available and ez_setup not required.

https://code.launchpad.net/~bac/charmworldlib/check-constraints/+merge/208669

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/69430043/

Affected files (+166, -370 lines):
   M Makefile
   A [revision details]
   M charmworldlib/bundle.py
   D ez_setup.py
   M setup.py
   M tests/test_bundle.py

[lp:~bac/charmworldlib/check-constraints]

33. By Brad Crittenden on 2014-02-28

Move constraint processing to utils.

34. By Brad Crittenden on 2014-02-28

Add new utils files

[Brad Crittenden (bac)]

Please take a look.

https://codereview.appspot.com/69430043/

[lp:~bac/charmworldlib/check-constraints]

35. By Brad Crittenden on 2014-02-28

Use forked version number for now.

[Brad Crittenden (bac)]

Please take a look.

https://codereview.appspot.com/69430043/

[Richard Harding (rharding)]

LGTM thanks for the update and sanity checking.

https://codereview.appspot.com/69430043/diff/40001/setup.py
File setup.py (right):

https://codereview.appspot.com/69430043/diff/40001/setup.py#newcode13
setup.py:13: version="0.2.4-2-bac",
this makes constraint changes. Should it be 0.2.5-1~bac?

https://codereview.appspot.com/69430043/

[lp:~bac/charmworldlib/check-constraints]

36. By Brad Crittenden on 2014-03-04

Update constraint parsing to be more robust.

[Brad Crittenden (bac)]

Please take a look.

https://codereview.appspot.com/69430043/diff/40001/setup.py
File setup.py (right):

https://codereview.appspot.com/69430043/diff/40001/setup.py#newcode13
setup.py:13: version="0.2.4-2-bac",
On 2014/02/28 14:42:11, rharding wrote:
> this makes constraint changes. Should it be 0.2.5-1~bac?

Done.

https://codereview.appspot.com/69430043/

[Marco Ceppi (marcoceppi)]

LGTM

https://codereview.appspot.com/69430043/

=== modified file 'Makefile'
--- Makefile	2014-02-04 15:21:40 +0000
+++ Makefile	2014-03-04 13:42:34 +0000
@@ -1,3 +1,4 @@
+SYSDEPS = python-virtualenv
 VENVS = .venv2 .venv3
 VENV2 = $(word 1, $(VENVS))
 VENV3 = $(word 2, $(VENVS))
@@ -30,6 +31,9 @@
 
 setup: $(VENVS)
 
+sysdeps:
+	sudo apt-get install --yes $(SYSDEPS)
+
 test: setup
 	$(VENV3)/bin/nosetests -s --verbosity=2
 	$(VENV2)/bin/nosetests -s --verbosity=2 --with-coverage --cover-package=charmworldlib
@@ -48,6 +52,6 @@
 clean-all: clean
 	rm -rf .pip-cache
 
-.PHONY: setup test lint clean clean-all install install_2 install_3
+.PHONY: setup sysdeps test lint clean clean-all install install_2 install_3
 
 .DEFAULT_GOAL := all

=== modified file 'charmworldlib/bundle.py'
--- charmworldlib/bundle.py	2014-01-16 09:35:29 +0000
+++ charmworldlib/bundle.py	2014-03-04 13:42:34 +0000
@@ -1,5 +1,5 @@
-
 import yaml
+
 from . import api
 
 

=== added file 'charmworldlib/utils.py'
--- charmworldlib/utils.py	1970-01-01 00:00:00 +0000
+++ charmworldlib/utils.py	2014-03-04 13:42:34 +0000
@@ -0,0 +1,76 @@
+import collections
+
+
+# Define a sequence of allowed constraints to be used in the process of
+# preparing the bundle object. See the _prepare_constraints function below.
+ALLOWED_CONSTRAINTS = (
+    'arch',
+    'container',
+    'cpu-cores',
+    'cpu-power',
+    'mem',
+    'root-disk',
+    # XXX: BradCrittenden 2014-02-12:
+    # tags are supported by MaaS only so they are not currently implemented.
+    # It is unclear whether the GUI should support them or not so they are
+    # being left out for now.
+    # Also, tags are a comma-separated, which would clash with the currently
+    # broken constraint parsing in the GUI.
+    # 'tags',
+)
+
+
+def parse_constraints(original_constraints):
+    """Parse the constraints and validate them.
+
+    constraints is a string of key=value pairs or a dict.  Due to
+    historical reasons, many bundles use a comma-separated list rather
+    than the space-separated list juju-core expects.  This method
+    handles both separators.
+
+    Returns a dict of validated constraints.
+    Raises ValueError if one or more constraints is invalid.
+    """
+
+    constraints = original_constraints
+    if not isinstance(constraints, collections.Mapping):
+        constraints = constraints.strip()
+        if not constraints:
+            return {}
+        #pairs = CONSTRAINTS_REGEX.findall(constraints)
+        num_equals = constraints.count('=')
+        # Comma separation is supported but deprecated.  Attempt splitting on
+        # it first as it yields better results if a mix of commas and spaces
+        # is used.
+        pairs = constraints.split(',')
+        if num_equals != len(pairs):
+            pairs = constraints.split(' ')
+        if num_equals != len(pairs):
+            raise ValueError('invalid constraints: {}'.format(
+                original_constraints))
+
+        constraints = {}
+        for item in pairs:
+            k, v = item.split('=')
+            if v.find(',') != -1:
+                raise ValueError('invalid constraints: {}'.format(
+                    original_constraints))
+
+            constraints[k.strip()] = v.strip()
+
+    unsupported = set(constraints).difference(ALLOWED_CONSTRAINTS)
+    if unsupported:
+        msg = 'unsupported constraints: {}'.format(
+            ', '.join(sorted(unsupported)))
+        raise ValueError(msg)
+    return constraints
+
+
+def check_constraints(original_constraints):
+    """Check to see that constraints are space-separated and valid."""
+    try:
+        parsed = parse_constraints(original_constraints)
+    except ValueError:
+        return False
+    tokens = original_constraints.strip().split()
+    return len(parsed) == len(tokens)

=== removed file 'ez_setup.py'
--- ez_setup.py	2014-02-19 13:03:36 +0000
+++ ez_setup.py	1970-01-01 00:00:00 +0000
@@ -1,361 +0,0 @@
-#!/usr/bin/env python
-"""Bootstrap setuptools installation
-
-To use setuptools in your package's setup.py, include this
-file in the same directory and add this to the top of your setup.py::
-
-    from ez_setup import use_setuptools
-    use_setuptools()
-
-To require a specific version of setuptools, set a download
-mirror, or use an alternate download directory, simply supply
-the appropriate options to ``use_setuptools()``.
-
-This file can also be run as a script to install or upgrade setuptools.
-"""
-import os
-import shutil
-import sys
-import tempfile
-import tarfile
-import optparse
-import subprocess
-import platform
-import textwrap
-
-from distutils import log
-
-try:
-    from site import USER_SITE
-except ImportError:
-    USER_SITE = None
-
-DEFAULT_VERSION = "0.9.8"
-DEFAULT_URL = "https://pypi.python.org/packages/source/s/setuptools/"
-
-def _python_cmd(*args):
-    args = (sys.executable,) + args
-    return subprocess.call(args) == 0
-
-def _install(tarball, install_args=()):
-    # extracting the tarball
-    tmpdir = tempfile.mkdtemp()
-    log.warn('Extracting in %s', tmpdir)
-    old_wd = os.getcwd()
-    try:
-        os.chdir(tmpdir)
-        tar = tarfile.open(tarball)
-        _extractall(tar)
-        tar.close()
-
-        # going in the directory
-        subdir = os.path.join(tmpdir, os.listdir(tmpdir)[0])
-        os.chdir(subdir)
-        log.warn('Now working in %s', subdir)
-
-        # installing
-        log.warn('Installing Setuptools')
-        if not _python_cmd('setup.py', 'install', *install_args):
-            log.warn('Something went wrong during the installation.')
-            log.warn('See the error message above.')
-            # exitcode will be 2
-            return 2
-    finally:
-        os.chdir(old_wd)
-        shutil.rmtree(tmpdir)
-
-
-def _build_egg(egg, tarball, to_dir):
-    # extracting the tarball
-    tmpdir = tempfile.mkdtemp()
-    log.warn('Extracting in %s', tmpdir)
-    old_wd = os.getcwd()
-    try:
-        os.chdir(tmpdir)
-        tar = tarfile.open(tarball)
-        _extractall(tar)
-        tar.close()
-
-        # going in the directory
-        subdir = os.path.join(tmpdir, os.listdir(tmpdir)[0])
-        os.chdir(subdir)
-        log.warn('Now working in %s', subdir)
-
-        # building an egg
-        log.warn('Building a Setuptools egg in %s', to_dir)
-        _python_cmd('setup.py', '-q', 'bdist_egg', '--dist-dir', to_dir)
-
-    finally:
-        os.chdir(old_wd)
-        shutil.rmtree(tmpdir)
-    # returning the result
-    log.warn(egg)
-    if not os.path.exists(egg):
-        raise IOError('Could not build the egg.')
-
-
-def _do_download(version, download_base, to_dir, download_delay):
-    egg = os.path.join(to_dir, 'setuptools-%s-py%d.%d.egg'
-                       % (version, sys.version_info[0], sys.version_info[1]))
-    if not os.path.exists(egg):
-        tarball = download_setuptools(version, download_base,
-                                      to_dir, download_delay)
-        _build_egg(egg, tarball, to_dir)
-    sys.path.insert(0, egg)
-
-    # Remove previously-imported pkg_resources if present (see
-    # https://bitbucket.org/pypa/setuptools/pull-request/7/ for details).
-    if 'pkg_resources' in sys.modules:
-        del sys.modules['pkg_resources']
-
-    import setuptools
-    setuptools.bootstrap_install_from = egg
-
-
-def use_setuptools(version=DEFAULT_VERSION, download_base=DEFAULT_URL,
-                   to_dir=os.curdir, download_delay=15):
-    to_dir = os.path.abspath(to_dir)
-    rep_modules = 'pkg_resources', 'setuptools'
-    imported = set(sys.modules).intersection(rep_modules)
-    try:
-        import pkg_resources
-    except ImportError:
-        return _do_download(version, download_base, to_dir, download_delay)
-    try:
-        pkg_resources.require("setuptools>=" + version)
-        return
-    except pkg_resources.DistributionNotFound:
-        return _do_download(version, download_base, to_dir, download_delay)
-    except pkg_resources.VersionConflict as VC_err:
-        if imported:
-            msg = textwrap.dedent("""
-                The required version of setuptools (>={version}) is not available,
-                and can't be installed while this script is running. Please
-                install a more recent version first, using
-                'easy_install -U setuptools'.
-
-                (Currently using {VC_err.args[0]!r})
-                """).format(VC_err=VC_err, version=version)
-            sys.stderr.write(msg)
-            sys.exit(2)
-
-        # otherwise, reload ok
-        del pkg_resources, sys.modules['pkg_resources']
-        return _do_download(version, download_base, to_dir, download_delay)
-
-def _clean_check(cmd, target):
-    """
-    Run the command to download target. If the command fails, clean up before
-    re-raising the error.
-    """
-    try:
-        subprocess.check_call(cmd)
-    except subprocess.CalledProcessError:
-        if os.access(target, os.F_OK):
-            os.unlink(target)
-        raise
-
-def download_file_powershell(url, target):
-    """
-    Download the file at url to target using Powershell (which will validate
-    trust). Raise an exception if the command cannot complete.
-    """
-    target = os.path.abspath(target)
-    cmd = [
-        'powershell',
-        '-Command',
-        "(new-object System.Net.WebClient).DownloadFile(%(url)r, %(target)r)" % vars(),
-    ]
-    _clean_check(cmd, target)
-
-def has_powershell():
-    if platform.system() != 'Windows':
-        return False
-    cmd = ['powershell', '-Command', 'echo test']
-    devnull = open(os.path.devnull, 'wb')
-    try:
-        try:
-            subprocess.check_call(cmd, stdout=devnull, stderr=devnull)
-        except:
-            return False
-    finally:
-        devnull.close()
-    return True
-
-download_file_powershell.viable = has_powershell
-
-def download_file_curl(url, target):
-    cmd = ['curl', url, '--silent', '--output', target]
-    _clean_check(cmd, target)
-
-def has_curl():
-    cmd = ['curl', '--version']
-    devnull = open(os.path.devnull, 'wb')
-    try:
-        try:
-            subprocess.check_call(cmd, stdout=devnull, stderr=devnull)
-        except:
-            return False
-    finally:
-        devnull.close()
-    return True
-
-download_file_curl.viable = has_curl
-
-def download_file_wget(url, target):
-    cmd = ['wget', url, '--quiet', '--output-document', target]
-    _clean_check(cmd, target)
-
-def has_wget():
-    cmd = ['wget', '--version']
-    devnull = open(os.path.devnull, 'wb')
-    try:
-        try:
-            subprocess.check_call(cmd, stdout=devnull, stderr=devnull)
-        except:
-            return False
-    finally:
-        devnull.close()
-    return True
-
-download_file_wget.viable = has_wget
-
-def download_file_insecure(url, target):
-    """
-    Use Python to download the file, even though it cannot authenticate the
-    connection.
-    """
-    try:
-        from urllib.request import urlopen
-    except ImportError:
-        from urllib2 import urlopen
-    src = dst = None
-    try:
-        src = urlopen(url)
-        # Read/write all in one block, so we don't create a corrupt file
-        # if the download is interrupted.
-        data = src.read()
-        dst = open(target, "wb")
-        dst.write(data)
-    finally:
-        if src:
-            src.close()
-        if dst:
-            dst.close()
-
-download_file_insecure.viable = lambda: True
-
-def get_best_downloader():
-    downloaders = [
-        download_file_powershell,
-        download_file_curl,
-        download_file_wget,
-        download_file_insecure,
-    ]
-
-    for dl in downloaders:
-        if dl.viable():
-            return dl
-
-def download_setuptools(version=DEFAULT_VERSION, download_base=DEFAULT_URL,
-                        to_dir=os.curdir, delay=15,
-                        downloader_factory=get_best_downloader):
-    """Download setuptools from a specified location and return its filename
-
-    `version` should be a valid setuptools version number that is available
-    as an egg for download under the `download_base` URL (which should end
-    with a '/'). `to_dir` is the directory where the egg will be downloaded.
-    `delay` is the number of seconds to pause before an actual download
-    attempt.
-
-    ``downloader_factory`` should be a function taking no arguments and
-    returning a function for downloading a URL to a target.
-    """
-    # making sure we use the absolute path
-    to_dir = os.path.abspath(to_dir)
-    tgz_name = "setuptools-%s.tar.gz" % version
-    url = download_base + tgz_name
-    saveto = os.path.join(to_dir, tgz_name)
-    if not os.path.exists(saveto):  # Avoid repeated downloads
-        log.warn("Downloading %s", url)
-        downloader = downloader_factory()
-        downloader(url, saveto)
-    return os.path.realpath(saveto)
-
-
-def _extractall(self, path=".", members=None):
-    """Extract all members from the archive to the current working
-       directory and set owner, modification time and permissions on
-       directories afterwards. `path' specifies a different directory
-       to extract to. `members' is optional and must be a subset of the
-       list returned by getmembers().
-    """
-    import copy
-    import operator
-    from tarfile import ExtractError
-    directories = []
-
-    if members is None:
-        members = self
-
-    for tarinfo in members:
-        if tarinfo.isdir():
-            # Extract directories with a safe mode.
-            directories.append(tarinfo)
-            tarinfo = copy.copy(tarinfo)
-            tarinfo.mode = 448  # decimal for oct 0700
-        self.extract(tarinfo, path)
-
-    # Reverse sort directories.
-    directories.sort(key=operator.attrgetter('name'), reverse=True)
-
-    # Set correct owner, mtime and filemode on directories.
-    for tarinfo in directories:
-        dirpath = os.path.join(path, tarinfo.name)
-        try:
-            self.chown(tarinfo, dirpath)
-            self.utime(tarinfo, dirpath)
-            self.chmod(tarinfo, dirpath)
-        except ExtractError as e:
-            if self.errorlevel > 1:
-                raise
-            else:
-                self._dbg(1, "tarfile: %s" % e)
-
-
-def _build_install_args(options):
-    """
-    Build the arguments to 'python setup.py install' on the setuptools package
-    """
-    return ['--user'] if options.user_install else []
-
-def _parse_args():
-    """
-    Parse the command line for options
-    """
-    parser = optparse.OptionParser()
-    parser.add_option(
-        '--user', dest='user_install', action='store_true', default=False,
-        help='install in user site package (requires Python 2.6 or later)')
-    parser.add_option(
-        '--download-base', dest='download_base', metavar="URL",
-        default=DEFAULT_URL,
-        help='alternative URL from where to download the setuptools package')
-    parser.add_option(
-        '--insecure', dest='downloader_factory', action='store_const',
-        const=lambda: download_file_insecure, default=get_best_downloader,
-        help='Use internal, non-validating downloader'
-    )
-    options, args = parser.parse_args()
-    # positional arguments are ignored
-    return options
-
-def main(version=DEFAULT_VERSION):
-    """Install or upgrade setuptools and EasyInstall"""
-    options = _parse_args()
-    tarball = download_setuptools(download_base=options.download_base,
-        downloader_factory=options.downloader_factory)
-    return _install(tarball, _build_install_args(options))
-
-if __name__ == '__main__':
-    sys.exit(main())

=== modified file 'setup.py'
--- setup.py	2014-02-19 13:04:04 +0000
+++ setup.py	2014-03-04 13:42:34 +0000
@@ -3,17 +3,12 @@
 # Copyright 2013 Marco Ceppi.  This software is licensed under the
 # GNU General Public License version 3 (see the file LICENSE).
 
-import ez_setup
-
-
-ez_setup.use_setuptools()
-
 from setuptools import setup
 
 
 setup(
     name='charmworldlib',
-    version="0.2.4-1",
+    version="0.2.5-1",
     packages=['charmworldlib'],
     maintainer='Marco Ceppi',
     maintainer_email='[e-mail]',

=== modified file 'tests/test_bundle.py'
--- tests/test_bundle.py	2014-01-16 09:35:29 +0000
+++ tests/test_bundle.py	2014-03-04 13:42:34 +0000
@@ -3,7 +3,10 @@
 import unittest
 
 from mock import patch
-from charmworldlib.bundle import Bundles, Bundle
+from charmworldlib.bundle import (
+    Bundle,
+    Bundles,
+)
 
 
 class BundlesTest(unittest.TestCase):

=== added file 'tests/test_utils.py'
--- tests/test_utils.py	1970-01-01 00:00:00 +0000
+++ tests/test_utils.py	2014-03-04 13:42:34 +0000
@@ -0,0 +1,124 @@
+"""Unit tests for utils."""
+
+import unittest
+
+from charmworldlib.utils import (
+    check_constraints,
+    parse_constraints,
+)
+
+
+class TestParseConstraints(unittest.TestCase):
+
+    def test_valid_constraints(self):
+        # Valid constraints are returned as they are.
+        constraints = {
+            'arch': 'i386',
+            'cpu-cores': 4,
+            'cpu-power': 2,
+            'mem': 2000,
+            'root-disk': '1G',
+            'container': 'lxc',
+        }
+        self.assertEqual(constraints, parse_constraints(constraints))
+
+    def test_valid_constraints_subset(self):
+        # A subset of valid constraints is returned as it is.
+        constraints = {'cpu-cores': '4', 'cpu-power': 2}
+        self.assertEqual(constraints, parse_constraints(constraints))
+
+    def test_invalid_constraints(self):
+        # A ValueError is raised if unsupported constraints are found.
+        with self.assertRaises(ValueError) as context_manager:
+            parse_constraints({'arch': 'i386', 'not-valid': 'bang!'})
+        self.assertEqual(
+            'unsupported constraints: not-valid',
+            str(context_manager.exception))
+
+    def test_string_constraints_space_separated(self):
+        # String constraints are converted to a dict.
+        constraints = 'arch=i386 cpu-cores=4 cpu-power=2 mem=2000'
+        expected = {
+            'arch': 'i386',
+            'cpu-cores': '4',
+            'cpu-power': '2',
+            'mem': '2000',
+        }
+        self.assertEqual(expected, parse_constraints(constraints))
+
+    def test_string_constraints_comma_separated(self):
+        # String constraints are converted to a dict.
+        constraints = 'arch=i386,cpu-cores=4,cpu-power=2,mem=2000'
+        expected = {
+            'arch': 'i386',
+            'cpu-cores': '4',
+            'cpu-power': '2',
+            'mem': '2000',
+        }
+        self.assertEqual(expected, parse_constraints(constraints))
+
+    def test_string_constraints_mixed_separated(self):
+        # String constraints are converted to a dict.
+        constraints = 'arch=i386,  cpu-cores=4, cpu-power=2,mem=2000'
+        expected = {
+            'arch': 'i386',
+            'cpu-cores': '4',
+            'cpu-power': '2',
+            'mem': '2000',
+        }
+        self.assertEqual(expected, parse_constraints(constraints))
+
+    def test_unsupported_string_constraints(self):
+        # A ValueError is raised if unsupported string constraints are found.
+        with self.assertRaises(ValueError) as context_manager:
+            parse_constraints('cpu-cores=4 invalid1=1 invalid2=2')
+        self.assertEqual(
+            'unsupported constraints: invalid1, invalid2',
+            str(context_manager.exception))
+
+    def test_invalid_string_constraints(self):
+        # A ValueError is raised if an invalid string is passed.
+        with self.assertRaises(ValueError) as context_manager:
+            parse_constraints('arch=,cpu-cores=,')
+        self.assertEqual(
+            'invalid constraints: arch=,cpu-cores=,',
+            str(context_manager.exception))
+
+    def test_invalid_no_pairs(self):
+        # A ValueError is raised if an invalid string is passed.
+        with self.assertRaises(ValueError) as context_manager:
+            parse_constraints('yo')
+        self.assertEqual(
+            'invalid constraints: yo',
+            str(context_manager.exception))
+
+    def test_not_key_value_pairs_constraints(self):
+        # A ValueError is raised if a string doesn't have the same number of
+        # tokens as '=' characters.
+        with self.assertRaises(ValueError) as context_manager:
+            parse_constraints('arch=1,cpu-cores')
+        self.assertEqual(
+            'invalid constraints: arch=1,cpu-cores',
+            str(context_manager.exception))
+
+    def test_empty_constraints_return_empty_dict(self):
+        constraints = parse_constraints('')
+        self.assertEqual({}, constraints)
+
+
+class TestCheckConstraints(unittest.TestCase):
+
+    def test_space_separated(self):
+        constraints = 'cpu-cores=4 mem=2000 root-disk=1'
+        result = check_constraints(constraints)
+        self.assertTrue(result)
+
+    def test_comma_separated(self):
+        constraints = 'cpu-cores=4,mem=2000,root-disk=1'
+        result = check_constraints(constraints)
+        self.assertFalse(result)
+
+    def test_invalid_separated(self):
+        constraints = 'invalid=4,mem=2000,root-disk=1'
+        result = check_constraints(constraints)
+        self.assertFalse(result)