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

[Sebastian Noack, 2017/08/15 15:54:45]

I would add a blank line below __future__ imports.

[tlucas, 2017/08/16 08:38:15]

Done.

+
 import os
 import re
 import codecs
 import subprocess
 import tarfile
 import json
 
 from packager import readMetadata, getDefaultFileName
 
 
@@ skipping 40 matching lines @@
 
     return False
 
 
 def repo_has_outgoing():
     """Checks whether there would be outgoing changesets to the given path"""
     try:
         subprocess.check_output(['hg', 'outgoing'])
         print('Detected outgoing changesets!')
         return True
-    except subprocess.CalledProcessError:
-        return False

[Sebastian Noack, 2017/08/15 15:54:45]

What is if an unexpected exit code occurs? I think we should still raise the
exception in that case.

  try:
      ...
  except subprocess.CalledProcessError as e:
      if e.returncode == 1:
          return False
      raise

Same below.

[tlucas, 2017/08/16 08:38:15]

Done.

+    except subprocess.CalledProcessError as e:
+        if e.returncode == 1:
+            return False
+        raise
 
 
-def repo_has_incoming(repo_paths):
+def repo_has_incoming(*repo_paths):
     """Checks whether the local repositories are up-to-date"""
     incoming = False
 
     for repo_path in repo_paths:
         try:
             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
 
 
 def continue_with_outgoing():
-    """Asks the user if he wants to continue despite facing warnings"""
+    """Asks the user if they want to continue despite facing warnings"""
 
     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?')
+    print('Are you sure about continuing the release process?')
 
     while True:
-        choice = raw_input('Please choose (yes / no): ').lower()
+        choice = raw_input('Please choose (yes / no): ').lower().strip()
 
         if choice == 'yes':
             return True
         if choice == 'no':
             return False
 
-        print('Please answer "yes" or "no"!')
+
+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
-    def can_safely_release():

[Sebastian Noack, 2017/08/15 15:54:45]

Do you really need a function here? Wouldn't this be equivalent to following?

  can_safely_release = (repo_has_uncommitted() and
                        repo_has_incoming() and
                        repo_has_outgoing() and
                        continue_with_outgoing())
  if not can_safely_release:
      ...

[tlucas, 2017/08/16 08:38:15]

I don't, but the equivalent would be:

can_safely_release = (not repo_has_uncommitted() and
                      not repo_has_incoming() and (
                          not repo_has_outgoing() or
                          continue_with_outgoing()))

Imho the if-switches (and the indented following logic) make it more clear how
"can_safely_release" should behave. If you don't mind, i would like to keep
this.

[Sebastian Noack, 2017/08/16 08:48:35]

I guess, it is OK to keep the function. But then there is no reason IMO to nest
this function into the run() function, as it doesn't access any nested
variables. So mind moving it to the top-level?

[tlucas, 2017/08/16 08:53:23]

Agreed, Done.

-        if repo_has_uncommitted():
-            return False
-        if repo_has_incoming():
-            return False
-        if repo_has_outgoing():
-            return continue_with_outgoing()
-
-    if not can_safely_release():
+    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])