#6145 - Introduce deploy script for websites

modules/adblockplus/files/web/static/deploy_script.py

 #!/usr/bin/env python
+#
+# This file is part of the Adblock Plus infrastructure
+# Copyright (C) 2018-present eyeo GmbH
+#
+# Adblock Plus is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# Adblock Plus is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import argparse
 from filecmp import dircmp
 import hashlib
 import os
 import sys
 import shutil
 import tarfile
 import tempfile
 import urllib
-import urllib2
-
-_tmp_dir = tempfile.mkdtemp()
 
 
-def download(url):
+__doc__ = """This script MUST be renamed in the form of $WEBSITE, e.g.
+          help.eyeo.com, --name must be provided in order to fetch the
+          files, expected files to be fetched are $NAME.tar.gz and $NAME.md5 in
+          order to compare the hashes. --source must be an URL, e.g.
+          https://helpcenter.eyeofiles.com"""
+
+
+def download(url, temporary_directory):
     file_name = url.split('/')[-1]
-    abs_file_name = os.path.join(_tmp_dir, file_name)
+    absolute_file_path = os.path.join(temporary_directory, file_name)
     print 'Downloading: ' + file_name
-    try:
-        filename, _ = urllib.urlretrieve(url, abs_file_name)

[Vasily Kuznetsov, 2018/05/22 16:38:37]

If you're throwing away `filename`, you can just ignore the return value of
`urlretrieve`. Or you can return `filename` from this function, since it now
contains the file where the data was saved.

[f.lopez, 2018/06/06 00:05:03]

Acknowledged.

-        return abs_file_name
-    except urllib2.HTTPError as e:
-        if e.code == 404:
-            sys.exit('File not found on remote source')
-    except Exception as e:
-        sys.exit(e)
+    urllib.urlretrieve(url, absolute_file_path)
+    return absolute_file_path
 
 
 def calculate_md5(file):
-    with open(file) as f:
-        data = f.read()
+    with open(file) as file_handle:
+        data = file_handle.read()
         md5_result = hashlib.md5(data).hexdigest()
     return md5_result.strip()
 
 
 def read_md5(file):
-    with open(file) as f:
-        md5_result = f.readline()
+    with open(file) as file_handle:
+        md5_result = file_handle.readline()
     return md5_result.strip()
 
 
-def untar(tar_file):
+def untar(tar_file, temporary_directory):
     if tarfile.is_tarfile(tar_file):
         with tarfile.open(tar_file, 'r:gz') as tar:
-            tar.extractall(_tmp_dir)
+            tar.extractall(temporary_directory)
 
 
 def remove_tree(to_remove):
     if os.path.exists(to_remove):
         if os.path.isdir(to_remove):
             shutil.rmtree(to_remove)
         else:
             os.remove(to_remove)
 
 
-def deploy_files(dcmp):
-    for name in dcmp.diff_files:
-        copytree(dcmp.right, dcmp.left)
-    for name in dcmp.left_only:
-        remove_tree(dcmp.left + "/" + name)

[Vasily Kuznetsov, 2018/05/23 12:25:43]

It's more portable (and generally a better practice) to use `os.path.join` for
this.

[f.lopez, 2018/06/06 00:05:03]

Acknowledged.

-    for name in dcmp.right_only:
-        copytree(dcmp.right, dcmp.left)
-    for sub_dcmp in dcmp.subdirs.values():
-        deploy_files(sub_dcmp)
+def deploy_files(directory_comparison):
+    for name in directory_comparison.diff_files:
+        copytree(directory_comparison.right, directory_comparison.left)
+    for name in directory_comparison.left_only:
+        remove_tree(os.path.join(directory_comparison.left, name))
+    for name in directory_comparison.right_only:
+        copytree(directory_comparison.right, directory_comparison.left)
+    for subdirectory_comparison in directory_comparison.subdirs.values():
+        deploy_files(subdirectory_comparison)
 
 
-def copytree(src, dst):
-    if not os.path.exists(dst):
-        os.makedirs(dst)
-        shutil.copystat(src, dst)
-    lst = os.listdir(src)
-    for item in lst:
-        s = os.path.join(src, item)
-        d = os.path.join(dst, item)
-        if os.path.isdir(s):
-            copytree(s, d)
+# shutil.copytree copies a tree but the destination directory MUST NOT exist
+# this might break the site for the duration of the files being deployed
+# for more info read: https://docs.python.org/2/library/shutil.html
+def copytree(source, destination):
+    if not os.path.exists(destination):
+        os.makedirs(destination)
+        shutil.copystat(source, destination)
+    source_items = os.listdir(source)
+    for item in source_items:
+        source_path = os.path.join(source, item)
+        destination_path = os.path.join(destination, item)
+        if os.path.isdir(source_path):
+            copytree(source_path, destination_path)
         else:
-            shutil.copy2(s, d)
+            shutil.copy2(source_path, destination_path)
 
 
 if __name__ == '__main__':
+    website = os.path.basename(__file__)
     parser = argparse.ArgumentParser(
-        description="""Fetch a compressed archive in the form of $HASH.tar.gz
-                    and deploy it to /var/www/$WEBSITE folder""",
-        epilog="""--hash must be provided in order to fetch the files,
-               expected files to be fetched are $HASH.tar.gz and $HASH.md5 in
-               order to compare the hashes.
-               --source must be an URL, e.g.
-               https://helpcenter.eyeofiles.com""",
+        description="""Fetch a compressed archive in the form of $NAME.tar.gz
+                    and deploy it to /var/www/{0} folder""".format(website),
+        epilog=__doc__,
     )
-    parser.add_argument('--hash', action='store', type=str,
-                        nargs='?', required=True,
-                        help='Hash of the commit to deploy')
-    parser.add_argument('--source', action='store', type=str,
-                        required=True, nargs='?',

[Vasily Kuznetsov, 2018/05/22 16:38:37]

Won't nargs='?' make the argument (of --source) optional? And then what happens
if it's not provided (if --source is given without any argument)?

[f.lopez, 2018/06/06 00:05:03]

Acknowledged.

+    parser.add_argument('--name', action='store', type=str, required=True,
+                        help='Name of the tarball to deploy')
+    parser.add_argument('--source', action='store', type=str, required=True,
                         help='The source where files will be downloaded')
-    parser.add_argument('--website', action='store', type=str, nargs='?',
-                        help='The name of the website [e.g. help.eyeo.com]')
-    args = parser.parse_args()
-    hash = args.hash
-    source = args.source
-    url_file = '{0}/{1}.tar.gz'.format(source, hash)
-    url_md5 = '{0}/{1}.md5'.format(source, hash)
-    down_file = download(url_file)
-    down_md5 = download(url_md5)
+    arguments = parser.parse_args()
+    name = arguments.name
+    source = arguments.source
+    url_file = '{0}/{1}.tar.gz'.format(source, name)
+    url_md5 = '{0}/{1}.md5'.format(source, name)
+    temporary_directory = tempfile.mkdtemp()
     try:
-        if calculate_md5(down_file) == read_md5(down_md5):
-            untar(down_file)
-            hash_directory = os.path.join(_tmp_dir, hash)
-            destination = '/var/www/' + args.website

[Vasily Kuznetsov, 2018/05/23 12:25:43]

Maybe also `os.path.join`?

Also, do you think it's ok to hardcode '/var/www/'? Perhaps better to make it an
option (maybe with this default)? It's a genuine question -- if this thing is
never expected to change, perhaps it's ok.

[f.lopez, 2018/06/06 00:05:03]

This is not expected to change, since it is the default value we look for in the
infrastructure part

-            dcmp = dircmp(destination, hash_directory)
-            print "Deploying files"

[Vasily Kuznetsov, 2018/05/23 12:25:44]

Remember the quotes rule ;P

[f.lopez, 2018/06/06 00:05:03]

Oh my god...

-            deploy_files(dcmp)
+        downloaded_file = download(url_file, temporary_directory)
+        downloaded_md5 = download(url_md5, temporary_directory)
+        if calculate_md5(downloaded_file) == read_md5(downloaded_md5):
+            untar(downloaded_file, temporary_directory)
+            tarball_directory = os.path.join(temporary_directory, name)
+            destination = os.path.join('/var/www/', website)
+            directory_comparison = dircmp(destination, tarball_directory)
+            print 'Deploying files'
+            deploy_files(directory_comparison)
         else:
-            sys.exit("Hashes don't match")
-    except Exception as e:
-        sys.exit(e)
+            error_message = """{0}.tar.gz md5 computation doesn't match {0}.md5
+                            contents""".format(name)
+            sys.exit(error_message)
+    except Exception as error:
+        sys.exit(error)
     finally:
-        shutil.rmtree(_tmp_dir)

[Vasily Kuznetsov, 2018/05/23 12:25:44]

The temporary directory won't be deleted if download() fails in lines 106 and
107 or if argument parsing throws an exception. I would propose to:
- move the creation of temporary directory into the `if __name__ == '__main__'
block,
- create it only before it's actually needed (before download()),
- pass it into download() as a parameter,
- move the download() calls into the try-finally block.
This way the temporary directory will always be deleted.

The side effect of this will be that download() will be called in the same
try-except block and you can do all exception handling only in lines 118-119...
and reduce duplication.

[f.lopez, 2018/06/06 00:05:03]

ok I like it, thanks

+        shutil.rmtree(temporary_directory)