stabilize --new-pr and --update-pr, and be more strict w.r.t. specifying a meaningful commit message

easybuild/tools/github.py

@@ -585,7 +585,7 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, target
     * stage/commit all files in PR branch
     * push PR branch to GitHub (to account specified by --github-user)
 
-    :param paths: tuple of paths that will be used to create/update PR: (easyconfigs, files to delete, patch files)
+    :param paths: paths to categorized lists of files (easyconfigs, files to delete, patches)
     :param ecs: list of parsed easyconfigs, incl. for dependencies (if robot is enabled)
     :param start_branch: name of branch to start from
     :param pr_branch: name of branch to push to GitHub
@@ -631,6 +631,17 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, target
     print_msg("copying easyconfigs to %s..." % target_dir)
     file_info = copy_easyconfigs(ec_paths, target_dir)
 
+    # figure out commit message to use
+    if commit_msg:
+        cnt = len(file_info['paths_in_repo'])
+        _log.debug("Using specified commit message for all %d new/modified easyconfigs at once: %s", cnt, commit_msg)
+    elif all(file_info['new']) and not paths['patch_files'] and not paths['files_to_delete']:
+        # automagically derive meaningful commit message if all easyconfig files are new
+        commit_msg = "adding easyconfigs: %s" % ', '.join(os.path.basename(p) for p in file_info['paths_in_repo'])
+    else:
+        raise EasyBuildError("A meaningful commit message must be specified via --pr-commit-msg when "
+                             "modifying/deleting easyconfigs and/or specifying patches")
+
     # figure out to which software name patches relate, and copy them to the right place
     if paths['patch_files']:
         patch_specs = det_patch_specs(paths['patch_files'], file_info)
@@ -709,14 +720,6 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, target
                              "Refused to make empty pull request.")
 
     # commit
-    if commit_msg:
-        _log.debug("Committing all %d new/modified easyconfigs at once", len(file_info['paths_in_repo']))
-    else:
-        commit_msg_parts = []
-        for path, new in zip(file_info['paths_in_repo'], file_info['new']):
-            commit_msg_parts.append("%s easyconfig %s" % (('modify', 'add')[new], os.path.basename(path)))
-        commit_msg = ', '.join(commit_msg_parts)
-
     git_repo.index.commit(commit_msg)
 
     # push to GitHub
@@ -836,14 +839,12 @@ def new_pr(paths, ecs, title=None, descr=None, commit_msg=None):
     """
     Open new pull request using specified files
 
-    :param paths: list of paths that will be used to create/update PR
+    :param paths: paths to categorized lists of files (easyconfigs, files to delete, patches)
     :param ecs: list of parsed easyconfigs, incl. for dependencies (if robot is enabled)
     :param title: title to use for pull request
     :param descr: description to use for description
     :param commit_msg: commit message to use
     """
-    _log.experimental("Opening new pull request for: %s", paths)
-
     pr_branch_name = build_option('pr_branch_name')
     pr_target_account = build_option('pr_target_account')
     pr_target_repo = build_option('pr_target_repo')
@@ -859,7 +860,7 @@ def new_pr(paths, ecs, title=None, descr=None, commit_msg=None):
     github_token = fetch_github_token(github_user)
     if github_token is None:
         raise EasyBuildError("GitHub token for user '%s' must be available to use --new-pr", github_user)
-    
+
     # create branch, commit files to it & push to GitHub
     file_info, deleted_paths, git_repo, branch, diff_stat = _easyconfigs_pr_common(paths, ecs,
                                                                                    pr_branch=pr_branch_name,
@@ -876,7 +877,6 @@ def new_pr(paths, ecs, title=None, descr=None, commit_msg=None):
     classes_counted = sorted([(classes.count(c), c) for c in nub(classes)])
     class_label = ','.join([tc for (cnt, tc) in classes_counted if cnt == classes_counted[-1][0]])
 
-
     if title is None:
         if file_info['ecs'] and all(file_info['new']) and not deleted_paths:
             # mention software name/version in PR title (only first 3)
@@ -935,17 +935,17 @@ def update_pr(pr, paths, ecs, commit_msg=None):
     Update specified pull request using specified files
 
     :param pr: ID of pull request to update
-    :param paths: list of paths that will be used to create/update PR
+    :param paths: paths to categorized lists of files (easyconfigs, files to delete, patches)
     :param ecs: list of parsed easyconfigs, incl. for dependencies (if robot is enabled)
     :param commit_msg: commit message to use
     """
-
-    _log.experimental("Updating pull request #%s with %s", pr, paths)
-
     github_user = build_option('github_user')
     if github_user is None:
         raise EasyBuildError("GitHub user must be specified to use --update-pr")
 
+    if commit_msg is None:
+        raise EasyBuildError("A meaningful commit message must be specified via --pr-commit-msg when using --update-pr")
+
     pr_target_account = build_option('pr_target_account')
     pr_target_repo = build_option('pr_target_repo')
 

test/framework/options.py

@@ -2256,7 +2256,6 @@ def test_minimal_toolchains(self):
         args = [
             ec_file,
             '--minimal-toolchains',
-            '--experimental',
             '--module-naming-scheme=HierarchicalMNS',
             '--dry-run',
         ]
@@ -2389,10 +2388,8 @@ def test_new_update_pr(self):
 
         args = [
             '--new-pr',
-            '--experimental',
             '--github-user=%s' % GITHUB_TEST_ACCOUNT,
             toy_ec,
-            toy_patch,
             '-D',
             '--disable-cleanup-tmpdir',
         ]
@@ -2401,6 +2398,13 @@ def test_new_update_pr(self):
         txt = self.get_stdout()
         self.mock_stdout(False)
 
+        # determine location of repo clone, can be used to test --git-working-dirs-path (and save time)
+        dirs = glob.glob(os.path.join(self.test_prefix, 'eb-*', '*', 'git-working-dir*'))
+        if len(dirs) == 1:
+            git_working_dir = dirs[0]
+        else:
+            self.assertTrue(False, "Failed to find temporary git working dir: %s" % dirs)
+
         regexs = [
             r"^== fetching branch 'develop' from https://github.com/hpcugent/easybuild-easyconfigs.git...",
             r"^Opening pull request \[DRY RUN\]",
@@ -2410,19 +2414,35 @@ def test_new_update_pr(self):
             r"\(created using `eb --new-pr`\)",  # description
             r"^\* overview of changes:",
             r".*/toy-0.0-gompi-1.3.12-test.eb\s*\|",
-            r".*/toy-0.0_typo.patch\s*\|",
-            r"^\s*2 files changed",
+            r"^\s*1 file changed",
         ]
         for regex in regexs:
             regex = re.compile(regex, re.M)
             self.assertTrue(regex.search(txt), "Pattern '%s' found in: %s" % (regex.pattern, txt))
 
-        # determine location of repo clone, can be used to test --git-working-dirs-path (and save time)
-        dirs = glob.glob(os.path.join(self.test_prefix, 'eb-*', '*', 'git-working-dir*'))
-        if len(dirs) == 1:
-            git_working_dir = dirs[0]
-        else:
-            self.assertTrue(False, "Failed to find temporary git working dir: %s" % dirs)
+        # a custom commit message is required when doing more than just adding new easyconfigs (e.g., adding a patch)
+        args.extend([
+            '--git-working-dirs-path=%s' % git_working_dir,
+            toy_patch,
+        ])
+        error_msg = "A meaningful commit message must be specified via --pr-commit-msg"
+
+        self.mock_stdout(True)
+        self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True, testing=False)
+        self.mock_stdout(False)
+
+        # add required commit message, try again
+        args.append('--pr-commit-msg="just a test"')
+        self.mock_stdout(True)
+        self.eb_main(args, do_build=True, raise_error=True, testing=False)
+        txt = self.get_stdout()
+        self.mock_stdout(False)
+
+        regexs[-1] = r"^\s*2 files changed"
+        regexs.append(r".*/toy-0.0_typo.patch\s*\|")
+        for regex in regexs:
+            regex = re.compile(regex, re.M)
+            self.assertTrue(regex.search(txt), "Pattern '%s' found in: %s" % (regex.pattern, txt))
 
         GITHUB_TEST_ORG = 'test-organization'
         args.extend([
@@ -2461,13 +2481,17 @@ def test_new_update_pr(self):
             # PR for EasyBuild v2.5.0 release
             # we need a PR where the base branch is still available ('develop', in this case)
             '--update-pr=2237',
-            '--experimental',
             '--github-user=%s' % GITHUB_TEST_ACCOUNT,
             toy_ec,
             '-D',
             # only to speed things up
             '--git-working-dirs-path=%s' % git_working_dir,
         ]
+
+        error_msg = "A meaningful commit message must be specified via --pr-commit-msg when using --update-pr"
+        self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True)
+
+        args.append('--pr-commit-msg="just a test"')
         self.mock_stdout(True)
         self.eb_main(args, do_build=True, raise_error=True, testing=False)
         txt = self.get_stdout()
@@ -2493,12 +2517,12 @@ def test_new_pr_delete(self):
 
         args = [
             '--new-pr',
-            '--experimental',
             '--github-user=%s' % GITHUB_TEST_ACCOUNT,
             ':bzip2-1.0.6.eb',
             '-D',
             '--disable-cleanup-tmpdir',
             '--pr-title=delete bzip2-1.6.0',
+            '--pr-commit-msg="delete bzip2-1.6.0.eb"'
         ]
         self.mock_stdout(True)
         self.eb_main(args, do_build=True, raise_error=True, testing=False)
@@ -2545,7 +2569,6 @@ def test_new_pr_dependencies(self):
 
         args = [
             '--new-pr',
-            '--experimental',
             '--github-user=%s' % GITHUB_TEST_ACCOUNT,
             os.path.join(self.test_prefix, 'foo-1.0.eb'),
             '-D',
@@ -2585,7 +2608,6 @@ def test_empty_pr(self):
         # try to open new pr with unchanged file
         args = [
             '--new-pr',
-            '--experimental',
             ec,
             '-D',
             '--github-user=%s' % GITHUB_TEST_ACCOUNT,