Add connector ops' required review when changing bypass_reasons on GA connectors

tools/ci_connector_ops/README.md

@@ -19,3 +19,17 @@ pip install -e . # assuming you are in the ./tools/ci_connector_ops directory
 ```
 
 pip will make binaries for all the commands in setup.py, so you can run `allowed-hosts-checks` directly from the virtual-env
+
+## Testing Locally
+
+To install requirements to run unit tests, use:
+
+```
+pip install -e ".[tests]"
+```
+
+Unit tests are currently configured to be run from the base `airbyte` directory. You can run the tests from that directory with the following command:
+
+```
+pytest -s tools/ci_connector_ops/tests
+```

tools/ci_connector_ops/ci_connector_ops/acceptance_test_config_checks.py

@@ -4,14 +4,15 @@
 
 import logging
 import sys
-from typing import List, Dict, Union
+from typing import List, Dict, Union, Set
 import yaml
 
 from ci_connector_ops import utils
 
 RELEASE_STAGE_TO_STRICTNESS_LEVEL_MAPPING = {"generally_available": "high"}
 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"
 
@@ -39,23 +40,36 @@ def find_connectors_with_bad_strictness_level() -> List[utils.Connector]:
                 connectors_with_bad_strictness_level.append(connector)
     return connectors_with_bad_strictness_level
 
-def find_changed_ga_connectors() -> List[utils.Connector]:
+def find_changed_ga_connectors() -> Set[utils.Connector]:
     """Find GA connectors modified on the current branch.
 
     Returns:
-        List[utils.Connector]: The list of GA connectors that were modified on the current branch.
+        Set[utils.Connector]: The set of GA connectors that were modified on the current branch.
     """
     changed_connectors = utils.get_changed_connectors()
-    return [connector for connector in changed_connectors if connector.release_stage == "generally_available"]
+    return {connector for connector in changed_connectors if connector.release_stage == "generally_available"}
+
+def get_ga_bypass_reason_changes() -> Set[utils.Connector]:
+    """Find GA 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_ga_connectors())
 
 def find_mandatory_reviewers() -> List[Union[str, Dict[str, List]]]:
     ga_connector_changes = find_changed_ga_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_ga_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 ga_connector_changes:
         return list(GA_CONNECTOR_REVIEWERS)
     return []

tools/ci_connector_ops/ci_connector_ops/utils.py

@@ -41,13 +41,13 @@ def get_connector_name_from_path(path):
     return path.split("/")[2]
 
 def get_changed_acceptance_test_config(diff_regex: Optional[str]=None) -> Set[str]:
-    """Retrieve a list of connector names for which the acceptance_test_config file was changed in the current branch (compared to master).
+    """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[str]: Set of connector names e.g {"source-pokeapi"}
+        Set[Connector]: Set of connectors that were changed
     """
     if diff_regex is None:
         diff_command_args = ("--name-only", DIFFED_BRANCH)
@@ -59,7 +59,7 @@ def get_changed_acceptance_test_config(diff_regex: Optional[str]=None) -> Set[st
         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)
     }
-    return {get_connector_name_from_path(changed_file) for changed_file in changed_acceptance_test_config_paths}
+    return {Connector(get_connector_name_from_path(changed_file)) for changed_file in changed_acceptance_test_config_paths}
 
 
 @dataclass(frozen=True)
@@ -152,7 +152,7 @@ def __repr__(self) -> str:
         return self.technical_name
 
 def get_changed_connectors() -> Set[Connector]:
-    """Retrieve a list of Connectors that were changed in the current branch (compared to master).
+    """Retrieve a set of Connectors that were changed in the current branch (compared to master).
     """
     changed_source_connector_files = {
         file_path

tools/ci_connector_ops/setup.py

@@ -24,7 +24,7 @@
 ]
 
 setup(
-    version="0.1.15",
+    version="0.1.16",
     name="ci_connector_ops",
     description="Packaged maintained by the connector operations team to perform CI for connectors",
     author="Airbyte",

tools/ci_connector_ops/tests/test_acceptance_test_config_checks.py

@@ -3,7 +3,7 @@
 #
 
 import shutil
-from typing import List
+from typing import List, Optional
 import yaml
 
 import pytest
@@ -44,6 +44,16 @@ def not_ga_test_strictness_level_change_expected_team(tmp_path, pokeapi_acceptan
     yield expected_teams
     shutil.copyfile(backup_path, pokeapi_acceptance_test_config_path)
 
+@pytest.fixture
+def not_ga_bypass_reason_file_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path):
+    expected_teams = []
+    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:
+        acceptance_test_config_file.write("bypass_reason:")
+    yield expected_teams
+    shutil.copyfile(backup_path, pokeapi_acceptance_test_config_path)
+
 @pytest.fixture
 def not_ga_not_tracked_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path):
     expected_teams = []
@@ -54,7 +64,6 @@ def not_ga_not_tracked_change_expected_team(tmp_path, pokeapi_acceptance_test_co
     yield expected_teams
     shutil.copyfile(backup_path, pokeapi_acceptance_test_config_path)
 
-
 @pytest.fixture
 def ga_connector_file_change_expected_team(tmp_path, ga_connector_file):
     expected_teams = list(acceptance_test_config_checks.GA_CONNECTOR_REVIEWERS)
@@ -66,7 +75,7 @@ def ga_connector_file_change_expected_team(tmp_path, ga_connector_file):
     shutil.copyfile(backup_path, ga_connector_file)
 
 @pytest.fixture
-def ga_connector_backward_compatibility_file_change(tmp_path, ga_connector_file):
+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)}]
     backup_path = tmp_path / "ga_acceptance_test_config.backup"
     shutil.copyfile(ga_connector_file, backup_path)
@@ -76,7 +85,17 @@ def ga_connector_backward_compatibility_file_change(tmp_path, ga_connector_file)
     shutil.copyfile(backup_path, ga_connector_file)
 
 @pytest.fixture
-def ga_connector_test_strictness_level_file_change(tmp_path, ga_connector_file):
+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)}]
+    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:
+        ga_acceptance_test_config_file.write("bypass_reason:")
+    yield expected_teams
+    shutil.copyfile(backup_path, ga_connector_file)
+
+@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)}]
     backup_path = tmp_path / "ga_acceptance_test_config.backup"
     shutil.copyfile(ga_connector_file, backup_path)
@@ -85,33 +104,47 @@ def ga_connector_test_strictness_level_file_change(tmp_path, ga_connector_file):
     yield expected_teams
     shutil.copyfile(backup_path, ga_connector_file)
 
-def check_review_requirements_file_contains_expected_teams(capsys, expected_teams: List):
-    acceptance_test_config_checks.write_review_requirements_file()
-    captured = capsys.readouterr()
+def verify_no_requirements_file_was_generated(captured: str):
+    assert captured.out.split("\n")[0].split("=")[-1] == "false"
+
+def verify_requirements_file_was_generated(captured: str):
     assert captured.out.split("\n")[0].split("=")[-1] == "true"
-    requirements_file_path = acceptance_test_config_checks.REVIEW_REQUIREMENTS_FILE_PATH
+
+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
 
-def test_find_mandatory_reviewers_backward_compatibility(mock_diffed_branched, capsys, not_ga_backward_compatibility_change_expected_team):
-    check_review_requirements_file_contains_expected_teams(capsys, not_ga_backward_compatibility_change_expected_team)
+def check_review_requirements_file(capsys, expected_teams: List):
+    acceptance_test_config_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
+        verify_review_requirements_file_contains_expected_teams(requirements_file_path, expected_teams)
 
+def test_find_mandatory_reviewers_backward_compatibility(mock_diffed_branched, capsys, not_ga_backward_compatibility_change_expected_team):
+    check_review_requirements_file(capsys, not_ga_backward_compatibility_change_expected_team)
 
 def test_find_mandatory_reviewers_test_strictness_level(mock_diffed_branched, capsys, not_ga_test_strictness_level_change_expected_team):
-    check_review_requirements_file_contains_expected_teams(capsys, not_ga_test_strictness_level_change_expected_team)
+    check_review_requirements_file(capsys, not_ga_test_strictness_level_change_expected_team)
+
+def test_find_mandatory_reviewers_not_ga_bypass_reason(mock_diffed_branched, capsys, not_ga_bypass_reason_file_change_expected_team):
+    check_review_requirements_file(capsys, not_ga_bypass_reason_file_change_expected_team)
 
-
 def test_find_mandatory_reviewers_ga(mock_diffed_branched, capsys, ga_connector_file_change_expected_team):
-    check_review_requirements_file_contains_expected_teams(capsys, ga_connector_file_change_expected_team)
+    check_review_requirements_file(capsys, ga_connector_file_change_expected_team)
+
+def test_find_mandatory_reviewers_ga_backward_compatibility(mock_diffed_branched, capsys, ga_connector_backward_compatibility_file_change_expected_team):
+    check_review_requirements_file(capsys, ga_connector_backward_compatibility_file_change_expected_team)
 
-def test_find_mandatory_reviewers_ga_backward_compatibility(mock_diffed_branched, capsys, ga_connector_backward_compatibility_file_change):
-    check_review_requirements_file_contains_expected_teams(capsys, ga_connector_backward_compatibility_file_change)
+def test_find_mandatory_reviewers_ga_bypass_reason(mock_diffed_branched, capsys, ga_connector_bypass_reason_file_change_expected_team):
+    check_review_requirements_file(capsys, ga_connector_bypass_reason_file_change_expected_team)
 
-def test_find_mandatory_reviewers_ga_test_strictness_level(mock_diffed_branched, capsys, ga_connector_test_strictness_level_file_change):
-    check_review_requirements_file_contains_expected_teams(capsys, ga_connector_test_strictness_level_file_change)
+def test_find_mandatory_reviewers_ga_test_strictness_level(mock_diffed_branched, capsys, ga_connector_test_strictness_level_file_change_expected_team):
+    check_review_requirements_file(capsys, ga_connector_test_strictness_level_file_change_expected_team)
 
 def test_find_mandatory_reviewers_no_tracked_changed(mock_diffed_branched, capsys, not_ga_not_tracked_change_expected_team):
-    acceptance_test_config_checks.write_review_requirements_file()
-    captured = capsys.readouterr()
-    assert captured.out.split("\n")[0].split("=")[-1] == "false"
+    check_review_requirements_file(capsys, not_ga_not_tracked_change_expected_team)