Merge lp:~thumper/charms/trusty/python-django/support-1.7 into lp:charms/python-django

[Tim Penhey]

This branch introduces a few tweaks, and support for Django 1.7 and above.

Some renaming was done in the code for my own sanity.
  sanitized_unit_name
became
  sanitized_service_name

The unit_name variable has been removed. Assigning service_name into unit_name was just confusing.

In order to handle checking the version number of django, a new pip package was added semantic_version.
Support for installing this package was added to the install and upgrade hooks.

The install hook didn't install the CHARM_PACKAGES (now called CHARM_DEB_PACKAGES) if the extra_deb_pkgs value was empty.

I haven't managed to work out how to run the unit tests inside the hooks dir.

If someone can help point that out, I'll add some unit tests for the 1.7 checking code.

[Charles Butler (lazypower)]

Tim,

Thank you for this contribution! It's great to see the python-django charm getting some love as its one of our few framework charms. I feel like your contributions have made a great impact in cleaning up some of the nomenclature and functionality of the charm for future contributors.

I deployed the charm and gave the charm a deploy test along with stressing the existing charm before pulling in your changes.

There is a minor linting issue that cropped up on this contribution:

FAIL: python-django::make lint
[/usr/bin/make -s lint exit 2]
Lint check (flake8)
hooks/hooks.py:44:19: E128 continuation line under-indented for visual indent
hooks/hooks.py:251:22: F821 undefined name 'semantic_version'
hooks/hooks.py:254:40: F821 undefined name 'semantic_version'
make: *** [lint] Error 1

If you could get those cleaned up in a subsequent patch, that would be +1 from me.

Thank you for taking the time to submit this fix for the charm store. We appreciate your work. I've merged this branch and it should be available in the charm store after the next ingestion. I look forward to the forthcoming updates to the python-django ecosystem from you :)

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

review: Approve

[Tim Penhey (thumper)]

On 23/05/15 07:54, Charles Butler wrote:
> Review: Approve
>
> Tim,
>
> Thank you for this contribution! It's great to see the python-django charm getting some love as its one of our few framework charms. I feel like your contributions have made a great impact in cleaning up some of the nomenclature and functionality of the charm for future contributors.
>
> I deployed the charm and gave the charm a deploy test along with stressing the existing charm before pulling in your changes.
>
> There is a minor linting issue that cropped up on this contribution:
>
> FAIL: python-django::make lint
> [/usr/bin/make -s lint exit 2]
> Lint check (flake8)
> hooks/hooks.py:44:19: E128 continuation line under-indented for visual indent
> hooks/hooks.py:251:22: F821 undefined name 'semantic_version'
> hooks/hooks.py:254:40: F821 undefined name 'semantic_version'
> make: *** [lint] Error 1
>
> If you could get those cleaned up in a subsequent patch, that would be +1 from me.
>
>
> Thank you for taking the time to submit this fix for the charm store. We appreciate your work. I've merged this branch and it should be available in the charm store after the next ingestion. I look forward to the forthcoming updates to the python-django ecosystem from you :)
>
>
> If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

I think we need to look at the process for checking that the charm works.

The lint issues identified above show that the code is clearly broken.
I have submitted another merge proposal that not only fixes this lint,
but adds tests to make sure it works as expected.

Tim

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-04-22 09:12:36 +0000
+++ hooks/hooks.py	2015-05-22 05:41:12 +0000
@@ -40,9 +40,11 @@
 
 hooks = Hooks()
 
-CHARM_PACKAGES = ["python-pip", "python-jinja2", "mercurial", "git-core",
+CHARM_DEB_PACKAGES = ["python-pip", "python-jinja2", "mercurial", "git-core",
                   "subversion", "bzr", "gettext"]
 
+CHARM_PIP_PACKAGES = ["semantic_version"]
+
 INJECTED_WARNING = """
 #------------------------------------------------------------------------------
 # The following is the import code for the settings directory injected by Juju
@@ -245,17 +247,22 @@
     applications.
     """
     django_admin_cmd = find_django_admin_cmd()
-    # TODO: only run syncdb if Django version is < 1.7
-    try:
-        run("%s syncdb --noinput --settings=%s" %
-            (django_admin_cmd, settings_module),
-            exit_on_error=exit_on_error, cwd=working_dir)
-    except:
-        if not swallow_exceptions:
-            raise
-
-    # TODO: check should be (django_south or Django version > 1.7)
-    if django_south:
+
+    django_version = semantic_version.Version(
+        subprocess.check_output([django_admin_cmd, '--version']),
+        partial=True)
+    django_migrate = django_version >= semantic_version.Version('1.7.0')
+
+    if not django_migrate:
+        try:
+            run("%s syncdb --noinput --settings=%s" %
+                (django_admin_cmd, settings_module),
+                exit_on_error=exit_on_error, cwd=working_dir)
+        except:
+            if not swallow_exceptions:
+                raise
+
+    if django_migrate or django_south:
         try:
             run("%s migrate --settings=%s" %
                 (django_admin_cmd, settings_module),
@@ -270,16 +277,16 @@
 ###############################################################################
 @hooks.hook()
 def install():
-    log("Installing {}".format(unit_name))
+    log("Installing {}".format(service_name()))
     apt_update()
 
-    packages = []
+    deb_packages = list(CHARM_DEB_PACKAGES)
     if extra_deb_pkgs:
-        packages = extra_deb_pkgs.split(',') + CHARM_PACKAGES
+        deb_packages.extend(extra_deb_pkgs.split(','))
 
     for retry in range(0, 24):
         try:
-            apt_install(packages)
+            apt_install(deb_packages)
         except subprocess.CalledProcessError as e:
             log("Error ({}) running  {}. Output: {}".format(
                 e.returncode, e.cmd, e.output))
@@ -288,11 +295,13 @@
 
         break
 
+    pip_packages = list(CHARM_PIP_PACKAGES)
     if extra_pip_pkgs:
-        for package in extra_pip_pkgs.split(','):
-            pip_install(package, upgrade=True)
+        pip_packages.extend(extra_pip_pkgs.split(','))
+    for package in pip_packages:
+        pip_install(package, upgrade=True)
 
-    configure_and_install(django_version)
+    configure_and_install(config_data['django_version'])
 
     if django_south:
         configure_and_install(django_south_version, distro="python-django-south", pip='South')
@@ -306,9 +315,9 @@
         if project_template_extension:
             cmd = " ".join([cmd, '--extension', project_template_extension])
         try:
-            run('%s %s %s' % (cmd, sanitized_unit_name, install_root), exit_on_error=False)
+            run('%s %s %s' % (cmd, sanitized_service_name, install_root), exit_on_error=False)
         except subprocess.CalledProcessError:
-            run('%s %s' % (cmd, sanitized_unit_name), cwd=install_root)
+            run('%s %s' % (cmd, sanitized_service_name), cwd=install_root)
     elif vcs == 'hg' or vcs == 'mercurial':
         run('hg clone %s %s' % (repos_url, vcs_clone_dir))
     elif vcs == 'git' or vcs == 'git-core':
@@ -340,21 +349,21 @@
 
     wsgi_py_path = os.path.join(working_dir, 'wsgi.py')
     if not os.path.exists(wsgi_py_path):
-        process_template('wsgi.py.tmpl', {'project_name': sanitized_unit_name,
+        process_template('wsgi.py.tmpl', {'project_name': sanitized_service_name,
                                           'django_settings': settings_module},
                          wsgi_py_path)
 
 
 @hooks.hook()
 def start():
-    if os.path.exists(os.path.join('/etc/init/', sanitized_unit_name + '.conf')):
-        service_start(sanitized_unit_name)
+    if os.path.exists(os.path.join('/etc/init/', sanitized_service_name + '.conf')):
+        service_start(sanitized_service_name)
 
 
 @hooks.hook()
 def stop():
-    if os.path.exists(os.path.join('/etc/init/', sanitized_unit_name + '.conf')):
-        service_stop(sanitized_unit_name)
+    if os.path.exists(os.path.join('/etc/init/', sanitized_service_name + '.conf')):
+        service_stop(sanitized_service_name)
 
 
 @hooks.hook()
@@ -395,7 +404,7 @@
     apt_update()
     for retry in range(0, 24):
         try:
-            apt_install(CHARM_PACKAGES)
+            apt_install(CHARM_DEB_PACKAGES)
         except subprocess.CalledProcessError as e:
             log("Error ({}) running  {}. Output: {}".format(
                 e.returncode, e.cmd, e.output))
@@ -406,9 +415,11 @@
 
     # FIXME: pull new code ?
 
+    pip_packages = list(CHARM_PIP_PACKAGES)
     if extra_pip_pkgs:
-        for package in extra_pip_pkgs.split(','):
-            pip_install(package, upgrade=True)
+        pip_packages.extend(extra_pip_pkgs.split(','))
+    for package in pip_packages:
+        pip_install(package, upgrade=True)
 
     if requirements_pip_files:
         for req_file in requirements_pip_files.split(','):
@@ -629,7 +640,7 @@
 @hooks.hook('amqp-relation-joined')
 def amqp_relation_joined():
     relation_set(relation_settings={
-        'username': sanitized_unit_name, 'vhost': sanitized_unit_name})
+        'username': sanitized_service_name, 'vhost': sanitized_service_name})
 
 
 @hooks.hook('amqp-relation-changed')
@@ -644,8 +655,8 @@
 
     templ_vars = config_data
     templ_vars.update({
-        'username': sanitized_unit_name,
-        'vhost': config_data['celery_amqp_vhost'] or sanitized_unit_name
+        'username': sanitized_service_name,
+        'vhost': config_data['celery_amqp_vhost'] or sanitized_service_name
     })
 
     templ_vars.update(relation_get())
@@ -716,7 +727,6 @@
 config_data = config()
 log("got config: %s" % str(config_data), DEBUG)
 
-django_version = config_data['django_version']
 vcs = config_data['vcs']
 repos_url = config_data['repos_url']
 repos_username = config_data['repos_username']
@@ -739,9 +749,8 @@
 django_south = config_data['django_south']
 django_south_version = config_data['django_south_version']
 
-unit_name = service_name()
-sanitized_unit_name = sanitize(unit_name)
-vcs_clone_dir = os.path.join(install_root, sanitized_unit_name)
+sanitized_service_name = sanitize(service_name())
+vcs_clone_dir = os.path.join(install_root, sanitized_service_name)
 if application_path:
     working_dir = os.path.join(vcs_clone_dir, application_path)
 else:
@@ -759,7 +768,7 @@
     if application_path:
         settings_module = '.'.join([os.path.basename(working_dir), 'settings'])
     else:
-        settings_module = '.'.join([sanitized_unit_name, 'settings'])
+        settings_module = '.'.join([sanitized_service_name, 'settings'])
 
 django_run_dir = os.path.join(working_dir, "run/")
 django_logs_dir = os.path.join(working_dir, "logs/")