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

releaseAutomation.py

Index: releaseAutomation.py
diff --git a/releaseAutomation.py b/releaseAutomation.py
index 5578abf1d948fe185e046cdb70ccd4734fa876ed..82678385338a138fd7e889a3c4f767549bb9c66b 100644
--- a/releaseAutomation.py
+++ b/releaseAutomation.py
@@ -9,6 +9,8 @@ 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
 
 
@@ -45,7 +47,100 @@ def create_sourcearchive(repo, output):
                 process.wait()
 
 
+def repo_has_uncommitted(aborts_process=True):
+    """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',
+    }
+
+    # 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 uncommitted, aborts_process
+
+
+def repo_has_outgoing(aborts_process=False):
+    """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
+
+
+def repo_has_incoming(repo_paths, aborts_process=True):
+    """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)
+            incoming = True
+        except subprocess.CalledProcessError:
+            pass
+
+    return incoming, aborts_process
+
+
+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
+
+    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?')
+    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
+
+    return choice.lower() == 'yes'
+
+
 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()
+            )):
+        print('Aborting release.')
+        return 1
+
     if type == 'gecko':
         import buildtools.packagerGecko as packager
     elif type == 'safari':