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
@@ skipping 52 matching lines @@
     try:
         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):
+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

[Wladimir Palant, 2017/08/16 10:46:09]

This seems inconsistent compared to repo_has_outgoing() above, I guess you want
`return True` without any additional variables here as well?

[tlucas, 2017/08/16 11:05:19]

If i returned immediately on the first occurrence of incoming changesets the
user would not be notified about incoming changesets of repositories, that might
be checked later. (e.g. if both the extension itself and the
downloads-repository have incoming changesets). Imho that missing information
would not be user-friendly and i would rather check for outgoing changesets for
all involved repositories as well - what do you think about this?

[Wladimir Palant, 2017/08/16 11:11:46]

I see, I overlooked that difference between the two functions. However, does
this even work? This function requires a list of repositories as parameter, yet
the caller isn't passing in any parameters.

[tlucas, 2017/08/16 11:27:54]

You are right, re-added the correct parameters to the callers / callees 

         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"""

[Wladimir Palant, 2017/08/16 10:46:09]

Please formulate this in a gender-neutral way, "they want" - we might have
female developers run this as well ;)

[tlucas, 2017/08/16 11:05:18]

Alright - i didn't want to leave anyone out :)

[tlucas, 2017/08/16 11:27:54]

Done.

+    """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?')

[Wladimir Palant, 2017/08/16 10:46:08]

Nit: it's "release process"

[tlucas, 2017/08/16 11:05:18]

Acknowledged.

[tlucas, 2017/08/16 11:27:54]

Done.

+    print('Are you sure about continuing the release process?')
 
     while True:
-        choice = raw_input('Please choose (yes / no): ').lower()

[Wladimir Palant, 2017/08/16 10:46:08]

Maybe strip whitespace as well?

[tlucas, 2017/08/16 11:05:18]

Acknowledged.

[tlucas, 2017/08/16 11:27:54]

Done.

+        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"!')

[Sebastian Noack, 2017/08/16 10:33:29]

This message seems redundant. If we don't return and jump back to the head of
the loop 'Please choose (yes / no): ' is already printed, again.

[tlucas, 2017/08/16 10:40:29]

Right, missed that. Done.

 
-
-def can_safely_release():
+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():
+    if repo_has_incoming(*repo_paths):
         return False
     if repo_has_outgoing():
         return continue_with_outgoing()
 
 
 def run(baseDir, type, version, keyFile, downloadsRepo):
-    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])