SERVER-55279: Commit queue message validation got tricked by merging multiple commits

Author: zamj

buildscripts/tests/test_validate_commit_message.py

@@ -1,9 +1,12 @@
 """Unit tests for the evergreen_task_timeout script."""
 import itertools
 import unittest
+from typing import List
 from mock import MagicMock, patch
 
-from buildscripts.validate_commit_message import main, STATUS_OK, STATUS_ERROR, GIT_SHOW_COMMAND
+import evergreen
+
+from buildscripts.validate_commit_message import main, STATUS_OK, STATUS_ERROR
 
 # pylint: disable=missing-docstring,no-self-use
 
@@ -23,6 +26,18 @@ def ns(relative_name):  # pylint: disable=invalid-name
     return NS + "." + relative_name
 
 
+def create_mock_evg_client(code_change_messages: List[str]) -> MagicMock:
+    mock_code_change = MagicMock()
+    mock_code_change.commit_messages = code_change_messages
+
+    mock_patch = MagicMock()
+    mock_patch.module_code_changes = [mock_code_change]
+
+    mock_evg_client = MagicMock()
+    mock_evg_client.patch_by_id.return_value = mock_patch
+    return mock_evg_client
+
+
 def interleave_new_format(older):
     """Create a new list containing a new and old format copy of each string."""
     newer = [
@@ -33,7 +48,8 @@ def interleave_new_format(older):
 
 
 class ValidateCommitMessageTest(unittest.TestCase):
-    def test_valid(self):
+    @patch.object(evergreen.RetryingEvergreenApi, "get_api")
+    def test_valid_commits(self, get_api_mock):
         messages = [
             "Fix lint",
             "EVG-1",  # Test valid projects with various number lengths
@@ -48,22 +64,28 @@ def test_valid(self):
             "Import wiredtiger: 58115abb6fbb3c1cc7bfd087d41a47347bce9a69 from branch mongodb-4.4",
             "Import tools: 58115abb6fbb3c1cc7bfd087d41a47347bce9a69 from branch mongodb-4.4"
         ]
+        api_mock = create_mock_evg_client(interleave_new_format(messages))
+
+        get_api_mock.return_value = api_mock
+        self.assertTrue(main(["fake_version"]) == STATUS_OK)
 
-        self.assertTrue(
-            all(main([message]) == STATUS_OK for message in interleave_new_format(messages)))
+    @patch.object(evergreen.RetryingEvergreenApi, "get_api")
+    def test_private(self, get_api_mock):
+        messages = ["XYZ-1"]
+        api_mock = create_mock_evg_client(interleave_new_format(messages))
 
-    def test_private(self):
-        self.assertEqual(main(["XYZ-1"]), STATUS_ERROR)
+        get_api_mock.return_value = api_mock
+        self.assertTrue(main(["fake_version"]) == STATUS_ERROR)
 
-    def test_catch_all(self):
-        self.assertTrue(
-            all(
-                main([message]) == STATUS_ERROR
-                for message in interleave_new_format(INVALID_MESSAGES)))
+    @patch.object(evergreen.RetryingEvergreenApi, "get_api")
+    def test_private_with_public(self, get_api_mock):
+        messages = [
+            "Fix lint",
+            "EVG-1",  # Test valid projects with various number lengths
+            "SERVER-20",
+            "XYZ-1"
+        ]
+        api_mock = create_mock_evg_client(interleave_new_format(messages))
 
-    def test_last_git_commit_success(self):
-        with patch(
-                ns("subprocess.check_output"),
-                return_value=bytearray('SERVER-1111 this is a test', 'utf-8')) as check_output_mock:
-            self.assertEqual(main([]), 0)
-            check_output_mock.assert_called_with(GIT_SHOW_COMMAND)
+        get_api_mock.return_value = api_mock
+        self.assertTrue(main(["fake_version"]) == STATUS_ERROR)

buildscripts/validate_commit_message.py

@@ -28,12 +28,12 @@
 #
 """Validate that the commit message is ok."""
 import argparse
-import os
 import re
-import subprocess
 import sys
 import logging
+from evergreen import RetryingEvergreenApi
 
+EVG_CONFIG_FILE = "./.evergreen.yml"
 LOGGER = logging.getLogger(__name__)
 
 COMMON_PUBLIC_PATTERN = r'''
@@ -121,29 +121,28 @@ def main(argv=None):
         usage="Validate the commit message. "
         "It validates the latest message when no arguments are provided.")
     parser.add_argument(
-        "message",
-        metavar="commit message",
-        nargs="*",
-        help="The commit message to validate",
+        "version_id",
+        metavar="version id",
+        help="The id of the version to validate",
     )
     args = parser.parse_args(argv)
-
-    if not args.message:
-        print('Validating last git commit message')
-        result = subprocess.check_output(GIT_SHOW_COMMAND)
-        message = result.decode('utf-8')
-    else:
-        message = " ".join(args.message)
-
-    if any(valid_pattern.match(message) for valid_pattern in VALID_PATTERNS):
-        return STATUS_OK
-    else:
-        if any(private_pattern.match(message) for private_pattern in PRIVATE_PATTERNS):
-            error_type = "Found a reference to a private project"
-        else:
-            error_type = "Found a commit without a ticket"
-        LOGGER.error(f"{error_type}\n{message}")  # pylint: disable=logging-fstring-interpolation
-        return STATUS_ERROR
+    evg_api = RetryingEvergreenApi.get_api(config_file=EVG_CONFIG_FILE)
+
+    code_changes = evg_api.patch_by_id(args.version_id).module_code_changes
+
+    for change in code_changes:
+        for message in change.commit_messages:
+            if any(valid_pattern.match(message) for valid_pattern in VALID_PATTERNS):
+                continue
+            elif any(private_pattern.match(message) for private_pattern in PRIVATE_PATTERNS):
+                error_type = "Found a reference to a private project"
+            else:
+                error_type = "Found a commit without a ticket"
+            if error_type:
+                LOGGER.error(f"{error_type}\n{message}")  # pylint: disable=logging-fstring-interpolation
+                return STATUS_ERROR
+
+    return STATUS_OK
 
 
 if __name__ == "__main__":

etc/evergreen.yml

@@ -6910,6 +6910,7 @@ tasks:
   - *kill_processes
   - *cleanup_environment
   - func: "set up venv"
+  - func: "configure evergreen api credentials"
   - command: subprocess.exec
     type: test
     params:

etc/pip/components/resmoke.req

@@ -1,6 +1,6 @@
 curatorbin == 1.2.1
 PyKMIP == 0.4.0 # It's now 0.8.0. We're far enough back to have API conflicts.
-evergreen.py == 3.1.0
+evergreen.py == 3.2.0
 jinja2
 MarkupSafe == 1.1.0  # See SERVER-57036, this is a transitive dependency of jinja2
 mock

evergreen/commit_message_validate.sh

@@ -6,15 +6,6 @@ cd src
 set -o verbose
 set -o errexit
 if [ "${is_commit_queue}" = "true" ]; then
-  # Since `commit_message` is an evergreen expansion, we need a way to ensure we
-  # properly deal with any special characters that could cause issues (like "). To
-  # do this, we will write it out to a file, then read that file into a variable.
-  cat > commit_message.txt << END_OF_COMMIT_MSG
-${commit_message}
-END_OF_COMMIT_MSG
-
-  commit_message_content=$(cat commit_message.txt)
-
   activate_venv
-  $python buildscripts/validate_commit_message.py "$commit_message_content"
+  $python buildscripts/validate_commit_message.py ${version_id}
 fi