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 23 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():
+    """Checks if the given repository is clean"""
+    buff = subprocess.check_output(['hg', 'status'])
+
+    if len(buff):
+        print('Dirty / uncommitted changes in repository!')
+        return True
+
+    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.

+
+
+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
+
+    return incoming
+
+
+def continue_with_outgoing():
+    """Asks the user if he wants 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?')
+
+    while True:
+        choice = raw_input('Please choose (yes / no): ').lower()
+
+        if choice == 'yes':
+            return True
+        if choice == 'no':
+            return False
+
+        print('Please answer "yes" or "no"!')
+
+
 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():
+        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':
         import buildtools.packagerChrome as packager
 
     # Replace version number in metadata file "manually", ConfigParser will mess
@@ skipping 57 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])