Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(49)

Issue 29777652: #6145 - Introduce deploy script for websites

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by f.lopez
Modified:
5 days, 13 hours ago
Visibility:
Public.

Description

#6145 - Introduce deploy script for websites

Patch Set 1 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -0 lines) Patch
A modules/adblockplus/files/web/static/deploy_script.py View 1 chunk +136 lines, -0 lines 9 comments Download

Messages

Total messages: 2
f.lopez
1 week, 3 days ago (2018-05-10 23:21:09 UTC) #1
Vasily Kuznetsov
5 days, 13 hours ago (2018-05-15 18:09:37 UTC) #2
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5