Issue 4354, 4355 - handle dirty/outdated repos on release

releaseAutomation.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+from __future__ import print_function
 
 import os
 import re
 import codecs
 import subprocess
 import tarfile
 import json
-

[Vasily Kuznetsov, 2017/08/08 18:10:12]

There's no need for this empty line. The imports format is normally[1]: stdlib,
3-rd party libraries, application/library specific, with the groups being
separated by empty lines. My interpretation of PEP8 is that there should be no
empty lines within groups, otherwise it becomes confusing.

[1]: https://www.python.org/dev/peps/pep-0008/#imports

[tlucas, 2017/08/09 08:12:35]

Acknowledged.

[tlucas, 2017/08/15 14:40:12]

Done.

-from itertools import groupby
 
 from packager import readMetadata, getDefaultFileName
 
 
 def get_dependencies(prefix, repos):
     from ensure_dependencies import read_deps, safe_join
     repo = repos[prefix]
     deps = read_deps(repo)
     if deps:
         for subpath in deps:
@@ skipping 17 matching lines @@
                         if os.path.basename(fileinfo.name) in ('.hgtags', '.hgignore'):
                             continue
                         filedata = repoarchive.extractfile(fileinfo)
                         fileinfo.name = re.sub(r'^[^/]+/', prefix, fileinfo.name)
                         archive.addfile(fileinfo, filedata)
             finally:
                 process.stdout.close()
                 process.wait()
 
 
-def repo_has_uncommitted(aborts_process=True):
+def repo_has_uncommitted():
     """Checks if the given repository is clean"""
-    uncommitted = False

[Vasily Kuznetsov, 2017/08/08 18:10:12]

You're spending quite some effort reformatting the changes that `hg status`
outputs. Have you checked with the users of this script if this has value for
them? It just seems to me that a message along the lines of "repo is dirty -- no
release for you" would be quite sufficient and then the devs could run `hg
status` themselves.

[tlucas, 2017/08/09 08:12:35]

Acknowledged.

[tlucas, 2017/08/15 14:40:12]

Done.

-    status_mapping = {
-        'M': 'modified',
-        'A': 'added',
-        'R': 'removed',
-        '!': 'missing',
-        '?': 'untracked',
-    }
+    buff = subprocess.check_output(['hg', 'status'])
 
-    # Check for any uncommitted changes
-    buff = subprocess.check_output(['hg', 'status'])
     if len(buff):
-        uncommitted = True
-        dirty_files = buff.strip().split(os.linesep)
         print('Dirty / uncommitted changes in repository!')

[Vasily Kuznetsov, 2017/08/08 18:10:11]

Using print as a function only seems to work under Python2 if you give it one
argument. If someone later adds another one, it will print a tuple. The correct
way to use print function would be to add a `from __future__ import
print_function` as the first import of the module.

[tlucas, 2017/08/09 08:12:35]

Acknowledged.

[tlucas, 2017/08/15 14:40:12]

Done.

-        grouped_dirty = groupby(dirty_files, key=lambda x: x.split(' ')[0])
-        for dirty_grp in grouped_dirty:
-            print('{}:'.format(status_mapping[dirty_grp[0]]))
-            for dirty in dirty_grp[1]:
-                print('  {}'.format(dirty.split(' ')[1]))
+        return True
 
-    return uncommitted, aborts_process
+    return False
 
 
-def repo_has_outgoing(aborts_process=False):
+def repo_has_outgoing():
     """Checks whether there would be outgoing changesets to the given path"""
     try:
-        buff = subprocess.check_output(['hg', 'outgoing'])
-        print('Detected outgoing changesets:')
-        print(buff)
-        return True, aborts_process
-    except subprocess.CalledProcessError:
-        return False, aborts_process
+        subprocess.check_output(['hg', 'outgoing'])
+        print('Detected outgoing changesets!')
+        return True
+    except subprocess.CalledProcessError as e:
+        if e.returncode == 1:
+            return False
+        raise
 
 
-def repo_has_incoming(repo_paths, aborts_process=True):
+def repo_has_incoming(*repo_paths):
     """Checks whether the local repositories are up-to-date"""
     incoming = False
 
     for repo_path in repo_paths:
         try:
-            buff = subprocess.check_output(['hg', 'incoming', '-R', repo_path])
-            print('Detected incoming changesets:')

[Vasily Kuznetsov, 2017/08/08 18:10:12]

Here we're printing the incoming changesets but actually it would probably be
enough to just print the list of outdated repositories.

[tlucas, 2017/08/09 08:12:35]

Acknowledged.

[tlucas, 2017/08/15 14:40:12]

Done.

-            print(buff)
+            subprocess.check_output(['hg', 'incoming', '-R', repo_path])
+            print('Detected incoming changesets in "{}"'.format(repo_path))
             incoming = True
-        except subprocess.CalledProcessError:
-            pass
+        except subprocess.CalledProcessError as e:
+            if e.returncode != 1:
+                raise
 
-    return incoming, aborts_process
+    return incoming
 
 
-def ask_to_continue():
-    """Asks the user if he wants to continue despite facing warnings"""
-    try:
-        input = raw_input

[Vasily Kuznetsov, 2017/08/08 18:10:11]

AFAIK, this compatibility trick doesn't work (under Python3) if you do it inside
a function. Have you tried to run this code with Python3?

[tlucas, 2017/08/09 08:12:34]

I tested a snippet of this, but not in function-scope. You are right about it
not working in a function though - maybe i should ignore the
python3-compatibility for now?

[Vasily Kuznetsov, 2017/08/09 17:09:09]

Yes, I think it would be the best to ignore Python3 compatibility in this case
-- it adds complexity for kind of no good reason at the moment. As for the print
function, I would say, if you add `from __future__ import print_function` to the
top, I'd be cool with that.

And then we will address Python3 compatibility of the built tools all together
in the ticket you've just created.

[tlucas, 2017/08/15 14:40:12]

Done.

-    except NameError:
-        pass
+def continue_with_outgoing():
+    """Asks the user if they want to continue despite facing warnings"""
 
-    print('The above error/s has/have been detected within the repositories.')

[Vasily Kuznetsov, 2017/08/08 18:10:11]

I know that you took this from the ticket, but I'm wondering if this line and
the following one actually add value. Imagine we have some outgoing commits.
What the user will see is:

    Detected outgoing changesets:

    changeset 1
    changeset 2

    The above error/s has/have been detected within the repositories.
    You might want to check whether this is ok or not.
    Are you sure about continuing the release-process?

This is a lot of text, but it's not very informative. How about replacing the
first two lines of the scary message with something like "If you proceed with
the release, they will be included in the release and pushed.":

    Detected outgoing changesets:

    changeset 1
    changeset 2

    If you proceed with the release, they will be included in the release and
pushed.
    Are you sure about continuing the release-process?

Would not this be better? (I posted it to the ticket)

[tlucas, 2017/08/09 08:12:35]

I like it (and it was actually me, who added this message to the ticket - so
it's still only me to blame ;) ) - maybe we should indeed wait for a response of
those actually doing the releases.

[tlucas, 2017/08/15 14:40:12]

Done.

-    print('You might want to check whether this is ok or not.')
-    print('Are you sure about continuing the release-process?')
+    print('If you proceed with the release, they will be included in the '
+          'release and pushed.')
+    print('Are you sure about continuing the release process?')
+
     while True:

[Vasily Kuznetsov, 2017/08/08 18:10:11]

What do you think about rewriting this loop, and the return statement like this:

    while True:
        choice = input('Please choose (yes / no): ').lower()
        if choice == 'yes':
            return True
        if choice == 'no':
            return False

It seems a bit more clear to me. What do you think?

[tlucas, 2017/08/09 08:12:35]

Acknowledged.

[tlucas, 2017/08/15 14:40:12]

Done.

-        choice = input('Please choose (yes / no): ')
-        if choice.lower() not in ('yes', 'no'):

[Vasily Kuznetsov, 2017/08/08 18:10:11]

('yes', 'no') should be a set (this is error A102 in flake8-eyeo plugin)

[tlucas, 2017/08/09 08:12:35]

This is removed while following your proposed approach above

[tlucas, 2017/08/15 14:40:12]

Done. Kept the reminder on what to specifically enter.

-            print('Please answer "yes" or "no"!')
-        else:
-            break
+        choice = raw_input('Please choose (yes / no): ').lower().strip()
 
-    return choice.lower() == 'yes'
+        if choice == 'yes':
+            return True
+        if choice == 'no':
+            return False
+
+
+def can_safely_release(*repo_paths):
+    """Run repository-checks in order to bail out early if necessary"""
+    if repo_has_uncommitted():
+        return False
+    if repo_has_incoming(*repo_paths):
+        return False
+    if repo_has_outgoing():
+        return continue_with_outgoing()
 
 
 def run(baseDir, type, version, keyFile, downloadsRepo):
-    # run repository-checks and bail out early, in case the user does not
-    # explicitly want to continue OR if the check would cause the process to
-    # abort anyway
-    repo_checks = (

[Vasily Kuznetsov, 2017/08/08 18:10:11]

I think the new code in this function is unnecessarily flexible (which is ok)
and rather hard to follow as a result (this is not ok, given that we don't need
the flexibility). What if we add a `sanity_check` function instead:

    function sanity_check(baseDir, downloadsRepo):
        if repo_has_incoming():
            return False
        if repo_has_uncommitted():
            return False
        if repo_has_outgoing():
            return continue_with_outgoing()  # I renamed `ask_to_continue`.

and then in `run` we'll have:

    if not sanity_check():
        print('Release aborted.')
        return 1

The whole thing reads almost like English and is about the same number of lines.
What do you think?

[tlucas, 2017/08/09 08:12:35]

Acknowledged. I'll also rename the function according to your proposal

[tlucas, 2017/08/15 14:40:12]

Done.

-        (repo_has_uncommitted, ()),
-        (repo_has_outgoing, ()),
-        (repo_has_incoming, ((baseDir, downloadsRepo),)),
-    )
-
-    check_results = [func(*args) for func, args in repo_checks]
-
-    if (
-            any(check and aborts for check, aborts in check_results)
-            or (
-                any(check and not aborts for check, aborts in check_results)
-                and not ask_to_continue()
-            )):
+    if not can_safely_release(baseDir, downloadsRepo):
         print('Aborting release.')
         return 1
 
     if type == 'gecko':
         import buildtools.packagerGecko as packager
     elif type == 'safari':
         import buildtools.packagerSafari as packager
     elif type == 'edge':
         import buildtools.packagerEdge as packager
     elif type == 'chrome':
@@ skipping 60 matching lines @@
     create_sourcearchive(baseDir, archivePath)
     downloads.append(archivePath)
 
     # Now add the downloads and commit
     subprocess.check_call(['hg', 'add', '-R', downloadsRepo] + downloads)
     subprocess.check_call(['hg', 'commit', '-R', downloadsRepo, '-m', 'Releasing %s %s' % (extensionName, version)])
 
     # Push all changes
     subprocess.check_call(['hg', 'push', '-R', baseDir])
     subprocess.check_call(['hg', 'push', '-R', downloadsRepo])