Issue 29777652: #6145 - Introduce deploy script for websites (Closed)
Description#6145 - Introduce deploy script for websites
Patch Set 1 #
Total comments: 20
Patch Set 2 : For comment 2 #Patch Set 3 : Forgot to remove unused import #
Total comments: 12
Patch Set 4 : For comment 7 #Patch Set 5 : Forgot to add some changes #
Total comments: 12
Patch Set 6 : For comment 12 #
Total comments: 8
Patch Set 7 : For comment 15 #Patch Set 8 : Use of a different name convention for the script #MessagesTotal messages: 19
Hi Paco! In general the script looks good. See some of the comments to the details below. Cheers, Vasily https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:16: abs_file_name = os.path.join(os.path.dirname(os.path.realpath(__file__)), It seems that you are using the directory of this script as a temporary directory. Is there a particular reason to not use a real temporary directory instead? It seems unlikely that the location of this script will be a good place to stash temporary junk. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:20: with closing(urllib2.urlopen(url)) as page: This code is more or less doing `urllib.urlretrieve` (https://docs.python.org/2/library/urllib.html#urllib.urlretrieve). Have you thought of using it? https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:31: sys.exit("File not found on remote source") We have a rule for selecting the type of quotes in stings. Basically it's "use single quotes unless your string contains single quotes". See https://adblockplus.org/coding-style#python. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:99: description='''Fetch a compressed archive in the form of $HASH.tar.gz and It's nicer to be consistent with the type of quotes you use for multiline strings. Normally double quotes are preferred, like in line 101. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:99: description='''Fetch a compressed archive in the form of $HASH.tar.gz and "Fetch" and "deploys" is inconsistent. It should be either "fetch and deploy" or "fetches and deploys". First one seems better. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:102: expected files to be fetched are $HASH.tar.gz and $HASH.md5 in order to This indentation is a bit hard to follow. Perhaps it would be better to add spaces in front of additional lines to make arguments clearly separated. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:108: parser.add_argument('--hash', action='store', type=str, nargs='?', 'store' is the default action, so you don't really need to specify it. I guess it's ok if you do, but you can also just skip it. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:124: url_file = 'https://{0}.eyeofiles.com/{1}.tar.gz'.format(domain, hash) What if --domain is not provided? https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:134: clean(hash) `clean()` won't be called in cases of errors -- is this intentional?
Thanks for your review, I'll be sending a new patch shortly https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:16: abs_file_name = os.path.join(os.path.dirname(os.path.realpath(__file__)), On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > It seems that you are using the directory of this script as a temporary > directory. Is there a particular reason to not use a real temporary directory > instead? It seems unlikely that the location of this script will be a good place > to stash temporary junk. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:20: with closing(urllib2.urlopen(url)) as page: On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > This code is more or less doing `urllib.urlretrieve` > (https://docs.python.org/2/library/urllib.html#urllib.urlretrieve). Have you > thought of using it? Acknowledged. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:31: sys.exit("File not found on remote source") On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > We have a rule for selecting the type of quotes in stings. Basically it's "use > single quotes unless your string contains single quotes". See > https://adblockplus.org/coding-style#python. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:99: description='''Fetch a compressed archive in the form of $HASH.tar.gz and On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > "Fetch" and "deploys" is inconsistent. It should be either "fetch and deploy" or > "fetches and deploys". First one seems better. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:99: description='''Fetch a compressed archive in the form of $HASH.tar.gz and On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > "Fetch" and "deploys" is inconsistent. It should be either "fetch and deploy" or > "fetches and deploys". First one seems better. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:102: expected files to be fetched are $HASH.tar.gz and $HASH.md5 in order to On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > This indentation is a bit hard to follow. Perhaps it would be better to add > spaces in front of additional lines to make arguments clearly separated. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:108: parser.add_argument('--hash', action='store', type=str, nargs='?', On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > 'store' is the default action, so you don't really need to specify it. I guess > it's ok if you do, but you can also just skip it. I know this is the default action, but I think it is nice to see it when reading the code. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:124: url_file = 'https://{0}.eyeofiles.com/{1}.tar.gz'.format(domain, hash) On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > What if --domain is not provided? You are right, I think it's better if I only provide an argument called 'source' so instead of having a magical conditional you need to always provide the source of the files, this way there is no misunderstanding on what's happening behind the scenes. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:134: clean(hash) On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > `clean()` won't be called in cases of errors -- is this intentional? Acknowledged.
Hi Paco, Good progress. I didn't finish reviewing the new patch, but I will post some comments already so you can look/respond to them. Cheers, Vasily https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:108: parser.add_argument('--hash', action='store', type=str, nargs='?', On 2018/05/21 22:23:41, f.lopez wrote: > On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > > 'store' is the default action, so you don't really need to specify it. I guess > > it's ok if you do, but you can also just skip it. > > I know this is the default action, but I think it is nice to see it when reading > the code. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29777653/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:124: url_file = 'https://{0}.eyeofiles.com/{1}.tar.gz'.format(domain, hash) On 2018/05/21 22:23:41, f.lopez wrote: > On 2018/05/15 18:09:37, Vasily Kuznetsov wrote: > > What if --domain is not provided? > > You are right, I think it's better if I only provide an argument called 'source' > so instead of having a magical conditional you need to always provide the source > of the files, this way there is no misunderstanding on what's happening behind > the scenes. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:22: filename, _ = urllib.urlretrieve(url, abs_file_name) 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. https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:97: required=True, nargs='?', 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)?
A few more comments after I had a second look. Now consider patch 3 reviewed :) https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:62: remove_tree(dcmp.left + "/" + name) It's more portable (and generally a better practice) to use `os.path.join` for this. https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:112: destination = '/var/www/' + args.website 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. https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:114: print "Deploying files" Remember the quotes rule ;P https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:121: shutil.rmtree(_tmp_dir) 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.
Gonna send a patch shortly https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:22: filename, _ = urllib.urlretrieve(url, abs_file_name) On 2018/05/22 16:38:37, Vasily Kuznetsov wrote: > 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. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:62: remove_tree(dcmp.left + "/" + name) On 2018/05/23 12:25:43, Vasily Kuznetsov wrote: > It's more portable (and generally a better practice) to use `os.path.join` for > this. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:97: required=True, nargs='?', On 2018/05/22 16:38:37, Vasily Kuznetsov wrote: > 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)? Acknowledged. https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:112: destination = '/var/www/' + args.website On 2018/05/23 12:25:43, Vasily Kuznetsov wrote: > 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. This is not expected to change, since it is the default value we look for in the infrastructure part https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:114: print "Deploying files" On 2018/05/23 12:25:44, Vasily Kuznetsov wrote: > Remember the quotes rule ;P Oh my god... https://codereview.adblockplus.org/29777652/diff/29786557/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:121: shutil.rmtree(_tmp_dir) On 2018/05/23 12:25:44, Vasily Kuznetsov wrote: > 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. ok I like it, thanks
LGTM
https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:1: #!/usr/bin/env python According to our legal department all source files should contain the Copyright stanza associated with the project. Also I am missing a __doc__ briefly explaining the purpose of this script (which you could re-use in the ArgumentParser epilog). https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:60: def copytree(src, dst): Why don't you use shutil.copytree? And why isn't that rationale documented or at least referred (read: linked) here? https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:66: s = os.path.join(src, item) So `s` is an abbreviation for `src` which is of course an abbreviation for `source`. Aha. https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:67: d = os.path.join(dst, item) And it continues. At least you are consistent. https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:84: parser.add_argument('--hash', action='store', type=str, required=True, Is this sure to always be a hash? Wouldn't `--revision`, `--version` or `--name` be a more future-proof choice of name? https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:88: parser.add_argument('--website', action='store', type=str, Why does option even exist? IT should not be possible to specify the target location (or a vital part thereof) from "the outside world" but be bound to the specific user's website (or set of websites). Otherwise we cant host multiple websites from different stakeholders on the same host, because one who owns /var/www/foo could easily overwrite the content of another one who owns /var/www/bar.
https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:1: #!/usr/bin/env python On 2018/06/07 21:52:00, mathias wrote: > According to our legal department all source files should contain the Copyright > stanza associated with the project. Also I am missing a __doc__ briefly > explaining the purpose of this script (which you could re-use in the > ArgumentParser epilog). Acknowledged. https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:60: def copytree(src, dst): On 2018/06/07 21:52:00, mathias wrote: > Why don't you use shutil.copytree? And why isn't that rationale documented or at > least referred (read: linked) here? Acknowledged. https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:66: s = os.path.join(src, item) On 2018/06/07 21:52:00, mathias wrote: > So `s` is an abbreviation for `src` which is of course an abbreviation for > `source`. Aha. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:67: d = os.path.join(dst, item) On 2018/06/07 21:52:00, mathias wrote: > And it continues. At least you are consistent. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:84: parser.add_argument('--hash', action='store', type=str, required=True, On 2018/06/07 21:52:00, mathias wrote: > Is this sure to always be a hash? Wouldn't `--revision`, `--version` or `--name` > be a more future-proof choice of name? Acknowledged. https://codereview.adblockplus.org/29777652/diff/29799626/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:88: parser.add_argument('--website', action='store', type=str, On 2018/06/07 21:52:00, mathias wrote: > Why does option even exist? IT should not be possible to specify the target > location (or a vital part thereof) from "the outside world" but be bound to the > specific user's website (or set of websites). Otherwise we cant host multiple > websites from different stakeholders on the same host, because one who owns > /var/www/foo could easily overwrite the content of another one who owns > /var/www/bar. Acknowledged.
Almost there :) https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:29: _doc = """This script MUST be renamed in the form of deploy_$WEBSITE, e.g. Why don't you use __doc__? https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:45: with open(file) as f: You can `open(file) as handle` or something similar. This is the only magic letter left (but also appears in `read_md5` below). https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:121: name_directory = os.path.join(temporary_directory, name) This name is a bit irritating (and was a bit irritating before). Maybe use something like `tarball_directory` or similar. https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:127: sys.exit("Hashes don't match") Sorry for not noticing this earlier, but this error message is not really descriptive enough. It may require looking in (or remembering) the code to understand what it means. Try: error_message = '{0}.tar.gz does not match {0}.md5'.format(name) sys.exit(error_message) Or something like that.
https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... File modules/adblockplus/files/web/static/deploy_script.py (right): https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:29: _doc = """This script MUST be renamed in the form of deploy_$WEBSITE, e.g. On 2018/06/18 19:58:38, mathias wrote: > Why don't you use __doc__? Acknowledged. https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:45: with open(file) as f: On 2018/06/18 19:58:39, mathias wrote: > You can `open(file) as handle` or something similar. This is the only magic > letter left (but also appears in `read_md5` below). Acknowledged. https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:121: name_directory = os.path.join(temporary_directory, name) On 2018/06/18 19:58:38, mathias wrote: > This name is a bit irritating (and was a bit irritating before). Maybe use > something like `tarball_directory` or similar. Acknowledged. https://codereview.adblockplus.org/29777652/diff/29809750/modules/adblockplus... modules/adblockplus/files/web/static/deploy_script.py:127: sys.exit("Hashes don't match") On 2018/06/18 19:58:39, mathias wrote: > Sorry for not noticing this earlier, but this error message is not really > descriptive enough. It may require looking in (or remembering) the code to > understand what it means. Try: > > error_message = '{0}.tar.gz does not match {0}.md5'.format(name) > sys.exit(error_message) > > Or something like that. Acknowledged.
LGTM. 
|
