require extra review for breaking change messages (#31361)

<!--
Thanks for your contribution! 
Before you submit the pull request, 
I'd like to kindly remind you to take a moment and read through our guidelines
to ensure that your contribution aligns with the type of contributions our project accepts.
All the information you need can be found here:
   https://docs.airbyte.com/contributing-to-airbyte/

We truly appreciate your interest in contributing to Airbyte,
and we're excited to see what you have to offer! 

If you have any questions or need any assistance, feel free to reach out in #contributions Slack channel.
-->

## What

We want to add Kat as a required reviewer when breaking change releases are made. This adds some logic to our existing checks to do so.

This also sets `request-reviewers: true` on our action so reviews are automatically requested when required.

Close #31141

## How

- Created new group, `breaking-change-reviewers` of which Kat is the only member currently to make a bit more flexible if we do want to change who gets notified or add more people.
- Refactor mandatory reviewer logic to support multiple required review groups at a time, rather than just requesting the first group.
- Add a method to get diff from connector metadata, + refactor the current method to get acceptance test config changes to use a common method.
    - Since it's not just acceptance test config changes that cause required reviewers, also moved out some code from `acceptance_test_config_checks` to `required-reviewer_checks`

.github/workflows/connector_teams_review_requirements.yml

@@ -39,4 +39,5 @@ jobs:
         with:
           status: ${{ steps.get-mandatory-reviewers.outputs.MANDATORY_REVIEWERS  }}
           token: ${{ secrets.OCTAVIA_4_ROOT_ACCESS }}
+          request-reviews: true
           requirements-file: .github/connector_org_review_requirements.yaml

airbyte-ci/connectors/connector_ops/connector_ops/acceptance_test_config_checks.py

@@ -4,17 +4,10 @@
 
 import logging
 import sys
-from typing import Dict, List, Set, Union
+from typing import List
 
-import yaml
 from connector_ops import utils
 
-BACKWARD_COMPATIBILITY_REVIEWERS = {"connector-operations", "connector-extensibility"}
-TEST_STRICTNESS_LEVEL_REVIEWERS = {"connector-operations"}
-GA_BYPASS_REASON_REVIEWERS = {"connector-operations"}
-GA_CONNECTOR_REVIEWERS = {"gl-python"}
-REVIEW_REQUIREMENTS_FILE_PATH = ".github/connector_org_review_requirements.yaml"
-
 
 def find_connectors_with_bad_strictness_level() -> List[utils.Connector]:
     """Check if changed connectors have the expected connector acceptance test strictness level according to their release stage.
@@ -38,43 +31,6 @@ def find_connectors_with_bad_strictness_level() -> List[utils.Connector]:
     return connectors_with_bad_strictness_level
 
 
-def find_changed_important_connectors() -> Set[utils.Connector]:
-    """Find important connectors modified on the current branch.
-
-    Returns:
-        Set[utils.Connector]: The set of GA connectors that were modified on the current branch.
-    """
-    changed_connectors = utils.get_changed_connectors(destination=False, third_party=False)
-    return {connector for connector in changed_connectors if connector.is_important_connector}
-
-
-def get_bypass_reason_changes() -> Set[utils.Connector]:
-    """Find connectors that have modified bypass_reasons.
-
-    Returns:
-        Set[str]: Set of connector names e.g {"source-github"}: The set of GA connectors that have changed bypass_reasons.
-    """
-    bypass_reason_changes = utils.get_changed_acceptance_test_config(diff_regex="bypass_reason")
-    return bypass_reason_changes.intersection(find_changed_important_connectors())
-
-
-def find_mandatory_reviewers() -> List[Union[str, Dict[str, List]]]:
-    important_connector_changes = find_changed_important_connectors()
-    backward_compatibility_changes = utils.get_changed_acceptance_test_config(diff_regex="disable_for_version")
-    test_strictness_level_changes = utils.get_changed_acceptance_test_config(diff_regex="test_strictness_level")
-    ga_bypass_reason_changes = get_bypass_reason_changes()
-
-    if backward_compatibility_changes:
-        return [{"any-of": list(BACKWARD_COMPATIBILITY_REVIEWERS)}]
-    if test_strictness_level_changes:
-        return [{"any-of": list(TEST_STRICTNESS_LEVEL_REVIEWERS)}]
-    if ga_bypass_reason_changes:
-        return [{"any-of": list(GA_BYPASS_REASON_REVIEWERS)}]
-    if important_connector_changes:
-        return list(GA_CONNECTOR_REVIEWERS)
-    return []
-
-
 def check_test_strictness_level():
     connectors_with_bad_strictness_level = find_connectors_with_bad_strictness_level()
     if connectors_with_bad_strictness_level:
@@ -84,28 +40,3 @@ def check_test_strictness_level():
         sys.exit(1)
     else:
         sys.exit(0)
-
-
-def write_review_requirements_file():
-    mandatory_reviewers = find_mandatory_reviewers()
-
-    if mandatory_reviewers:
-        requirements_file_content = [
-            {"name": "Required reviewers from the connector org teams", "paths": "unmatched", "teams": mandatory_reviewers}
-        ]
-        with open(REVIEW_REQUIREMENTS_FILE_PATH, "w") as requirements_file:
-            yaml.safe_dump(requirements_file_content, requirements_file)
-        print("CREATED_REQUIREMENTS_FILE=true")
-    else:
-        print("CREATED_REQUIREMENTS_FILE=false")
-
-
-def print_mandatory_reviewers():
-    teams = []
-    mandatory_reviewers = find_mandatory_reviewers()
-    for mandatory_reviewer in mandatory_reviewers:
-        if isinstance(mandatory_reviewer, dict):
-            teams += mandatory_reviewer["any-of"]
-        else:
-            teams.append(mandatory_reviewer)
-    print(f"MANDATORY_REVIEWERS=A review is required from these teams: {','.join(teams)}")

airbyte-ci/connectors/connector_ops/connector_ops/required_reviewer_checks.py

@@ -0,0 +1,79 @@
+#
+# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
+#
+
+from typing import Dict, List, Set, Union
+
+import yaml
+from connector_ops import utils
+
+BACKWARD_COMPATIBILITY_REVIEWERS = {"connector-operations", "connector-extensibility"}
+TEST_STRICTNESS_LEVEL_REVIEWERS = {"connector-operations"}
+GA_BYPASS_REASON_REVIEWERS = {"connector-operations"}
+GA_CONNECTOR_REVIEWERS = {"gl-python"}
+BREAKING_CHANGE_REVIEWERS = {"breaking-change-reviewers"}
+REVIEW_REQUIREMENTS_FILE_PATH = ".github/connector_org_review_requirements.yaml"
+
+
+def find_changed_important_connectors() -> Set[utils.Connector]:
+    """Find important connectors modified on the current branch.
+
+    Returns:
+        Set[utils.Connector]: The set of GA connectors that were modified on the current branch.
+    """
+    changed_connectors = utils.get_changed_connectors(destination=False, third_party=False)
+    return {connector for connector in changed_connectors if connector.is_important_connector}
+
+
+def get_bypass_reason_changes() -> Set[utils.Connector]:
+    """Find connectors that have modified bypass_reasons.
+
+    Returns:
+        Set[str]: Set of connector names e.g {"source-github"}: The set of GA connectors that have changed bypass_reasons.
+    """
+    bypass_reason_changes = utils.get_changed_acceptance_test_config(diff_regex="bypass_reason")
+    return bypass_reason_changes.intersection(find_changed_important_connectors())
+
+
+def find_mandatory_reviewers() -> List[Dict[str, Union[str, Dict[str, List]]]]:
+    requirements = [
+        {
+            "name": "Backwards Compatibility Test Skip",
+            "teams": list(BACKWARD_COMPATIBILITY_REVIEWERS),
+            "is_required": utils.get_changed_acceptance_test_config(diff_regex="disable_for_version"),
+        },
+        {
+            "name": "Acceptance Test Strictness Level",
+            "teams": list(TEST_STRICTNESS_LEVEL_REVIEWERS),
+            "is_required": utils.get_changed_acceptance_test_config(diff_regex="test_strictness_level"),
+        },
+        {"name": "GA Acceptance Test Bypass", "teams": list(GA_BYPASS_REASON_REVIEWERS), "is_required": get_bypass_reason_changes()},
+        {"name": "GA Connectors", "teams": list(GA_CONNECTOR_REVIEWERS), "is_required": find_changed_important_connectors()},
+        {
+            "name": "Breaking Changes",
+            "teams": list(BREAKING_CHANGE_REVIEWERS),
+            "is_required": utils.get_changed_metadata(diff_regex="upgradeDeadline"),
+        },
+    ]
+
+    return [{"name": r["name"], "teams": r["teams"]} for r in requirements if r["is_required"]]
+
+
+def write_review_requirements_file():
+    mandatory_reviewers = find_mandatory_reviewers()
+
+    if mandatory_reviewers:
+        requirements_file_content = [dict(r, paths="unmatched") for r in mandatory_reviewers]
+        with open(REVIEW_REQUIREMENTS_FILE_PATH, "w") as requirements_file:
+            yaml.safe_dump(requirements_file_content, requirements_file)
+        print("CREATED_REQUIREMENTS_FILE=true")
+    else:
+        print("CREATED_REQUIREMENTS_FILE=false")
+
+
+def print_mandatory_reviewers():
+    teams = set()
+    mandatory_reviewers = find_mandatory_reviewers()
+    for reviewers in mandatory_reviewers:
+        teams.update(reviewers["teams"])
+    print(f"MANDATORY_REVIEWERS=A review is required from these teams: {', '.join(teams)}")

airbyte-ci/connectors/connector_ops/connector_ops/utils.py

@@ -34,6 +34,7 @@
 
 
 ACCEPTANCE_TEST_CONFIG_FILE_NAME = "acceptance-test-config.yml"
+METADATA_FILE_NAME = "metadata.yaml"
 AIRBYTE_DOCKER_REPO = "airbyte"
 AIRBYTE_REPO_DIRECTORY_NAME = "airbyte"
 GRADLE_PROJECT_RE_PATTERN = r"project\((['\"])(.+?)\1\)"
@@ -81,6 +82,30 @@ def get_connector_name_from_path(path):
 def get_changed_acceptance_test_config(diff_regex: Optional[str] = None) -> Set[str]:
     """Retrieve the set of connectors for which the acceptance_test_config file was changed in the current branch (compared to master).
 
+    Args:
+        diff_regex (str): Find the edited files that contain the following regex in their change.
+
+    Returns:
+        Set[Connector]: Set of connectors that were changed
+    """
+    return get_changed_file(ACCEPTANCE_TEST_CONFIG_FILE_NAME, diff_regex)
+
+
+def get_changed_metadata(diff_regex: Optional[str] = None) -> Set[str]:
+    """Retrieve the set of connectors for which the metadata file was changed in the current branch (compared to master).
+
+    Args:
+        diff_regex (str): Find the edited files that contain the following regex in their change.
+
+    Returns:
+        Set[Connector]: Set of connectors that were changed
+    """
+    return get_changed_file(METADATA_FILE_NAME, diff_regex)
+
+
+def get_changed_file(file_name: str, diff_regex: Optional[str] = None) -> Set[str]:
+    """Retrieve the set of connectors for which the given file was changed in the current branch (compared to master).
+
     Args:
         diff_regex (str): Find the edited files that contain the following regex in their change.
 
@@ -97,7 +122,7 @@ def get_changed_acceptance_test_config(diff_regex: Optional[str] = None) -> Set[
     changed_acceptance_test_config_paths = {
         file_path
         for file_path in airbyte_repo.git.diff(*diff_command_args).split("\n")
-        if file_path.startswith(SOURCE_CONNECTOR_PATH_PREFIX) and file_path.endswith(ACCEPTANCE_TEST_CONFIG_FILE_NAME)
+        if file_path.startswith(SOURCE_CONNECTOR_PATH_PREFIX) and file_path.endswith(file_name)
     }
     return {Connector(get_connector_name_from_path(changed_file)) for changed_file in changed_acceptance_test_config_paths}
 

airbyte-ci/connectors/connector_ops/pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "poetry.core.masonry.api"
 
 [tool.poetry]
 name = "connector_ops"
-version = "0.2.6"
+version = "0.3.0"
 description = "Packaged maintained by the connector operations team to perform CI for connectors"
 authors = ["Airbyte <[email protected]>"]
 
@@ -30,7 +30,7 @@ freezegun = "^1.1.0"
 
 [tool.poetry.scripts]
 check-test-strictness-level = "connector_ops.acceptance_test_config_checks:check_test_strictness_level"
-write-review-requirements-file = "connector_ops.acceptance_test_config_checks:write_review_requirements_file"
-print-mandatory-reviewers = "connector_ops.acceptance_test_config_checks:print_mandatory_reviewers"
+write-review-requirements-file = "connector_ops.required_reviewer_checks:write_review_requirements_file"
+print-mandatory-reviewers = "connector_ops.required_reviewer_checks:print_mandatory_reviewers"
 allowed-hosts-checks = "connector_ops.allowed_hosts_checks:check_allowed_hosts"
 run-qa-checks = "connector_ops.qa_checks:run_qa_checks"

airbyte-ci/connectors/connector_ops/tests/test_acceptance_test_config_checks.py → airbyte-ci/connectors/connector_ops/tests/test_required_reviewer_checks.py

@@ -8,13 +8,13 @@
 import git
 import pytest
 import yaml
-from connector_ops import acceptance_test_config_checks
+from connector_ops import required_reviewer_checks
 
 
 @pytest.fixture
 def mock_diffed_branched(mocker):
     airbyte_repo = git.Repo(search_parent_directories=True)
-    mocker.patch.object(acceptance_test_config_checks.utils, "DIFFED_BRANCH", airbyte_repo.active_branch)
+    mocker.patch.object(required_reviewer_checks.utils, "DIFFED_BRANCH", airbyte_repo.active_branch)
     return airbyte_repo.active_branch
 
 
@@ -23,14 +23,19 @@ def pokeapi_acceptance_test_config_path():
     return "airbyte-integrations/connectors/source-pokeapi/acceptance-test-config.yml"
 
 
+@pytest.fixture
+def pokeapi_metadata_path():
+    return "airbyte-integrations/connectors/source-pokeapi/metadata.yaml"
+
+
 @pytest.fixture
 def ga_connector_file():
     return "airbyte-integrations/connectors/source-amplitude/acceptance-test-config.yml"
 
 
 @pytest.fixture
 def not_ga_backward_compatibility_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path) -> List:
-    expected_teams = [{"any-of": list(acceptance_test_config_checks.BACKWARD_COMPATIBILITY_REVIEWERS)}]
+    expected_teams = list(required_reviewer_checks.BACKWARD_COMPATIBILITY_REVIEWERS)
     backup_path = tmp_path / "backup_poke_acceptance"
     shutil.copyfile(pokeapi_acceptance_test_config_path, backup_path)
     with open(pokeapi_acceptance_test_config_path, "a") as acceptance_test_config_file:
@@ -41,7 +46,7 @@ def not_ga_backward_compatibility_change_expected_team(tmp_path, pokeapi_accepta
 
 @pytest.fixture
 def not_ga_test_strictness_level_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path) -> List:
-    expected_teams = [{"any-of": list(acceptance_test_config_checks.TEST_STRICTNESS_LEVEL_REVIEWERS)}]
+    expected_teams = list(required_reviewer_checks.TEST_STRICTNESS_LEVEL_REVIEWERS)
     backup_path = tmp_path / "non_ga_acceptance_test_config.backup"
     shutil.copyfile(pokeapi_acceptance_test_config_path, backup_path)
     with open(pokeapi_acceptance_test_config_path, "a") as acceptance_test_config_file:
@@ -74,7 +79,7 @@ def not_ga_not_tracked_change_expected_team(tmp_path, pokeapi_acceptance_test_co
 
 @pytest.fixture
 def ga_connector_file_change_expected_team(tmp_path, ga_connector_file):
-    expected_teams = list(acceptance_test_config_checks.GA_CONNECTOR_REVIEWERS)
+    expected_teams = list(required_reviewer_checks.GA_CONNECTOR_REVIEWERS)
     backup_path = tmp_path / "ga_acceptance_test_config.backup"
     shutil.copyfile(ga_connector_file, backup_path)
     with open(ga_connector_file, "a") as ga_acceptance_test_config_file:
@@ -85,7 +90,7 @@ def ga_connector_file_change_expected_team(tmp_path, ga_connector_file):
 
 @pytest.fixture
 def ga_connector_backward_compatibility_file_change_expected_team(tmp_path, ga_connector_file):
-    expected_teams = [{"any-of": list(acceptance_test_config_checks.BACKWARD_COMPATIBILITY_REVIEWERS)}]
+    expected_teams = list(required_reviewer_checks.BACKWARD_COMPATIBILITY_REVIEWERS)
     backup_path = tmp_path / "ga_acceptance_test_config.backup"
     shutil.copyfile(ga_connector_file, backup_path)
     with open(ga_connector_file, "a") as ga_acceptance_test_config_file:
@@ -96,7 +101,7 @@ def ga_connector_backward_compatibility_file_change_expected_team(tmp_path, ga_c
 
 @pytest.fixture
 def ga_connector_bypass_reason_file_change_expected_team(tmp_path, ga_connector_file):
-    expected_teams = [{"any-of": list(acceptance_test_config_checks.GA_BYPASS_REASON_REVIEWERS)}]
+    expected_teams = list(required_reviewer_checks.GA_BYPASS_REASON_REVIEWERS)
     backup_path = tmp_path / "ga_acceptance_test_config.backup"
     shutil.copyfile(ga_connector_file, backup_path)
     with open(ga_connector_file, "a") as ga_acceptance_test_config_file:
@@ -107,7 +112,7 @@ def ga_connector_bypass_reason_file_change_expected_team(tmp_path, ga_connector_
 
 @pytest.fixture
 def ga_connector_test_strictness_level_file_change_expected_team(tmp_path, ga_connector_file):
-    expected_teams = [{"any-of": list(acceptance_test_config_checks.TEST_STRICTNESS_LEVEL_REVIEWERS)}]
+    expected_teams = list(required_reviewer_checks.TEST_STRICTNESS_LEVEL_REVIEWERS)
     backup_path = tmp_path / "ga_acceptance_test_config.backup"
     shutil.copyfile(ga_connector_file, backup_path)
     with open(ga_connector_file, "a") as ga_acceptance_test_config_file:
@@ -116,6 +121,19 @@ def ga_connector_test_strictness_level_file_change_expected_team(tmp_path, ga_co
     shutil.copyfile(backup_path, ga_connector_file)
 
 
+@pytest.fixture
+def test_breaking_change_release_expected_team(tmp_path, pokeapi_metadata_path) -> List:
+    expected_teams = list(required_reviewer_checks.BREAKING_CHANGE_REVIEWERS)
+    backup_path = tmp_path / "backup_poke_metadata"
+    shutil.copyfile(pokeapi_metadata_path, backup_path)
+    with open(pokeapi_metadata_path, "a") as acceptance_test_config_file:
+        acceptance_test_config_file.write(
+            "releases:\n  breakingChanges:\n    23.0.0:\n      message: hi\n      upgradeDeadline: 2025-01-01"
+        )
+    yield expected_teams
+    shutil.copyfile(backup_path, pokeapi_metadata_path)
+
+
 def verify_no_requirements_file_was_generated(captured: str):
     assert captured.out.split("\n")[0].split("=")[-1] == "false"
 
@@ -127,17 +145,17 @@ def verify_requirements_file_was_generated(captured: str):
 def verify_review_requirements_file_contains_expected_teams(requirements_file_path: str, expected_teams: List):
     with open(requirements_file_path, "r") as requirements_file:
         requirements = yaml.safe_load(requirements_file)
-    assert requirements[0]["teams"] == expected_teams
+    assert any([r["teams"] == expected_teams for r in requirements])
 
 
 def check_review_requirements_file(capsys, expected_teams: List):
-    acceptance_test_config_checks.write_review_requirements_file()
+    required_reviewer_checks.write_review_requirements_file()
     captured = capsys.readouterr()
     if not expected_teams:
         verify_no_requirements_file_was_generated(captured)
     else:
         verify_requirements_file_was_generated(captured)
-        requirements_file_path = acceptance_test_config_checks.REVIEW_REQUIREMENTS_FILE_PATH
+        requirements_file_path = required_reviewer_checks.REVIEW_REQUIREMENTS_FILE_PATH
         verify_review_requirements_file_contains_expected_teams(requirements_file_path, expected_teams)
 
 
@@ -173,5 +191,9 @@ def test_find_mandatory_reviewers_ga_test_strictness_level(
     check_review_requirements_file(capsys, ga_connector_test_strictness_level_file_change_expected_team)
 
 
+def test_find_mandatory_reviewers_breaking_change_release(mock_diffed_branched, capsys, test_breaking_change_release_expected_team):
+    check_review_requirements_file(capsys, test_breaking_change_release_expected_team)
+
+
 def test_find_mandatory_reviewers_no_tracked_changed(mock_diffed_branched, capsys, not_ga_not_tracked_change_expected_team):
     check_review_requirements_file(capsys, not_ga_not_tracked_change_expected_team)