Skip to content

Commit 9c4e38d

Browse files
authored
remove extensibility from required reviewers checks (#37716)
## What <!-- * Describe what the change is solving. Link all GitHub issues related to this change. --> * Reduce PR noise of automatic review requesting on things that we don't actually review. * Close #37662 Notes: * Kat still wants the breaking change reviewer tags * investigating what API sources wants out of this autotagger [separately](https://airbytehq-team.slack.com/archives/C02U9R3AF37/p1714499322978079) and will follow up in a separate PR based on that ## How <!-- * Describe how code changes achieve the solution. --> * Remove these checks. This made more sense conceptually for code and testing than having empty sets, becuase we would still output an empty requirements file. instead of hacking that apart, I think this makes more sense, but am open to other ideas ## Can this PR be safely reverted and rolled back? <!-- * If unsure, leave it blank. --> - [x] YES 💚 - [ ] NO ❌
1 parent af975f4 commit 9c4e38d

File tree

3 files changed

+27
-135
lines changed

3 files changed

+27
-135
lines changed

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

+4-25
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@
77
import yaml
88
from connector_ops import utils
99

10-
BACKWARD_COMPATIBILITY_REVIEWERS = {"connector-extensibility"}
11-
TEST_STRICTNESS_LEVEL_REVIEWERS = {"connector-extensibility"}
12-
BYPASS_REASON_REVIEWERS = {"connector-extensibility"}
13-
STRATEGIC_PYTHON_CONNECTOR_REVIEWERS = {"gl-python", "connector-extensibility"}
10+
# TODO: Check to see if GL still wants to get tagged in this. If so,
11+
# the logic needs to be audited to make sure its actually just python.
12+
STRATEGIC_PYTHON_CONNECTOR_REVIEWERS = {"gl-python"}
13+
# The breaking change reviewers is still in active use.
1414
BREAKING_CHANGE_REVIEWERS = {"breaking-change-reviewers"}
1515
REVIEW_REQUIREMENTS_FILE_PATH = ".github/connector_org_review_requirements.yaml"
1616

@@ -31,29 +31,8 @@ def find_changed_strategic_connectors(
3131
return {connector for connector in changed_connectors if connector.is_strategic_connector and connector.language in languages}
3232

3333

34-
def get_bypass_reason_changes() -> Set[utils.Connector]:
35-
"""Find connectors that have modified bypass_reasons.
36-
37-
Returns:
38-
Set[str]: Set of connector names e.g {"source-github"}: The set of important connectors that have changed bypass_reasons.
39-
"""
40-
bypass_reason_changes = utils.get_changed_acceptance_test_config(diff_regex="bypass_reason")
41-
return bypass_reason_changes.intersection(find_changed_strategic_connectors())
42-
43-
4434
def find_mandatory_reviewers() -> List[Dict[str, Union[str, Dict[str, List]]]]:
4535
requirements = [
46-
{
47-
"name": "Backwards compatibility test skip",
48-
"teams": list(BACKWARD_COMPATIBILITY_REVIEWERS),
49-
"is_required": utils.get_changed_acceptance_test_config(diff_regex="disable_for_version"),
50-
},
51-
{
52-
"name": "Acceptance test strictness level",
53-
"teams": list(TEST_STRICTNESS_LEVEL_REVIEWERS),
54-
"is_required": utils.get_changed_acceptance_test_config(diff_regex="test_strictness_level"),
55-
},
56-
{"name": "Strategic connector bypass reasons", "teams": list(BYPASS_REASON_REVIEWERS), "is_required": get_bypass_reason_changes()},
5736
{
5837
"name": "Strategic python connectors",
5938
"teams": list(STRATEGIC_PYTHON_CONNECTOR_REVIEWERS),

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

-12
Original file line numberDiff line numberDiff line change
@@ -81,18 +81,6 @@ def get_connector_name_from_path(path):
8181
return path.split("/")[2]
8282

8383

84-
def get_changed_acceptance_test_config(diff_regex: Optional[str] = None) -> Set[str]:
85-
"""Retrieve the set of connectors for which the acceptance_test_config file was changed in the current branch (compared to master).
86-
87-
Args:
88-
diff_regex (str): Find the edited files that contain the following regex in their change.
89-
90-
Returns:
91-
Set[Connector]: Set of connectors that were changed
92-
"""
93-
return get_changed_file(ACCEPTANCE_TEST_CONFIG_FILE_NAME, diff_regex)
94-
95-
9684
def get_changed_metadata(diff_regex: Optional[str] = None) -> Set[str]:
9785
"""Retrieve the set of connectors for which the metadata file was changed in the current branch (compared to master).
9886

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

+23-98
Original file line numberDiff line numberDiff line change
@@ -35,37 +35,8 @@ def strategic_connector_file():
3535

3636

3737
@pytest.fixture
38-
def not_strategic_backward_compatibility_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path) -> List:
39-
expected_teams = list(required_reviewer_checks.BACKWARD_COMPATIBILITY_REVIEWERS)
40-
backup_path = tmp_path / "backup_poke_acceptance"
41-
shutil.copyfile(pokeapi_acceptance_test_config_path, backup_path)
42-
with open(pokeapi_acceptance_test_config_path, "a") as acceptance_test_config_file:
43-
acceptance_test_config_file.write("disable_for_version: 0.0.0")
44-
yield expected_teams
45-
shutil.copyfile(backup_path, pokeapi_acceptance_test_config_path)
46-
47-
48-
@pytest.fixture
49-
def not_strategic_test_strictness_level_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path) -> List:
50-
expected_teams = list(required_reviewer_checks.TEST_STRICTNESS_LEVEL_REVIEWERS)
51-
backup_path = tmp_path / "non_strategic_acceptance_test_config.backup"
52-
shutil.copyfile(pokeapi_acceptance_test_config_path, backup_path)
53-
with open(pokeapi_acceptance_test_config_path, "a") as acceptance_test_config_file:
54-
acceptance_test_config_file.write("test_strictness_level: foo")
55-
yield expected_teams
56-
shutil.copyfile(backup_path, pokeapi_acceptance_test_config_path)
57-
58-
59-
@pytest.fixture
60-
def not_strategic_bypass_reason_file_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path):
61-
# The bypass reason logic explicitly only checks for bypass reasons on strategic connectors
62-
expected_teams = []
63-
backup_path = tmp_path / "non_strategic_acceptance_test_config.backup"
64-
shutil.copyfile(pokeapi_acceptance_test_config_path, backup_path)
65-
with open(pokeapi_acceptance_test_config_path, "a") as acceptance_test_config_file:
66-
acceptance_test_config_file.write("bypass_reason:")
67-
yield expected_teams
68-
shutil.copyfile(backup_path, pokeapi_acceptance_test_config_path)
38+
def strategic_connector_metadata_path():
39+
return "airbyte-integrations/connectors/source-amplitude/metadata.yaml"
6940

7041

7142
@pytest.fixture
@@ -90,45 +61,6 @@ def strategic_connector_file_change_expected_team(tmp_path, strategic_connector_
9061
shutil.copyfile(backup_path, strategic_connector_file)
9162

9263

93-
@pytest.fixture
94-
def strategic_connector_backward_compatibility_file_change_expected_team(tmp_path, strategic_connector_file):
95-
expected_teams = list(
96-
required_reviewer_checks.STRATEGIC_PYTHON_CONNECTOR_REVIEWERS.union(required_reviewer_checks.BACKWARD_COMPATIBILITY_REVIEWERS)
97-
)
98-
backup_path = tmp_path / "strategic_acceptance_test_config.backup"
99-
shutil.copyfile(strategic_connector_file, backup_path)
100-
with open(strategic_connector_file, "a") as strategic_acceptance_test_config_file:
101-
strategic_acceptance_test_config_file.write("disable_for_version: 0.0.0")
102-
yield expected_teams
103-
shutil.copyfile(backup_path, strategic_connector_file)
104-
105-
106-
@pytest.fixture
107-
def strategic_connector_bypass_reason_file_change_expected_team(tmp_path, strategic_connector_file):
108-
expected_teams = list(
109-
required_reviewer_checks.STRATEGIC_PYTHON_CONNECTOR_REVIEWERS.union(required_reviewer_checks.BYPASS_REASON_REVIEWERS)
110-
)
111-
backup_path = tmp_path / "strategic_acceptance_test_config.backup"
112-
shutil.copyfile(strategic_connector_file, backup_path)
113-
with open(strategic_connector_file, "a") as strategic_acceptance_test_config_file:
114-
strategic_acceptance_test_config_file.write("bypass_reason:")
115-
yield expected_teams
116-
shutil.copyfile(backup_path, strategic_connector_file)
117-
118-
119-
@pytest.fixture
120-
def strategic_connector_test_strictness_level_file_change_expected_team(tmp_path, strategic_connector_file):
121-
expected_teams = list(
122-
required_reviewer_checks.STRATEGIC_PYTHON_CONNECTOR_REVIEWERS.union(required_reviewer_checks.TEST_STRICTNESS_LEVEL_REVIEWERS)
123-
)
124-
backup_path = tmp_path / "strategic_acceptance_test_config.backup"
125-
shutil.copyfile(strategic_connector_file, backup_path)
126-
with open(strategic_connector_file, "a") as strategic_acceptance_test_config_file:
127-
strategic_acceptance_test_config_file.write("test_strictness_level: 0.0.0")
128-
yield expected_teams
129-
shutil.copyfile(backup_path, strategic_connector_file)
130-
131-
13264
@pytest.fixture
13365
def test_breaking_change_release_expected_team(tmp_path, pokeapi_metadata_path) -> List:
13466
expected_teams = list(required_reviewer_checks.BREAKING_CHANGE_REVIEWERS)
@@ -142,6 +74,21 @@ def test_breaking_change_release_expected_team(tmp_path, pokeapi_metadata_path)
14274
shutil.copyfile(backup_path, pokeapi_metadata_path)
14375

14476

77+
@pytest.fixture
78+
def strategic_connector_breaking_change_release_expected_teams(tmp_path, strategic_connector_metadata_path):
79+
expected_teams = list(
80+
required_reviewer_checks.STRATEGIC_PYTHON_CONNECTOR_REVIEWERS.union(required_reviewer_checks.BREAKING_CHANGE_REVIEWERS)
81+
)
82+
backup_path = tmp_path / "strategic_acceptance_test_config.backup"
83+
shutil.copyfile(strategic_connector_metadata_path, backup_path)
84+
with open(strategic_connector_metadata_path, "a") as strategic_connector_metadata_file:
85+
strategic_connector_metadata_file.write(
86+
"releases:\n breakingChanges:\n 23.0.0:\n message: hi\n upgradeDeadline: 2025-01-01"
87+
)
88+
yield expected_teams
89+
shutil.copyfile(backup_path, strategic_connector_metadata_path)
90+
91+
14592
def verify_no_requirements_file_was_generated(captured: str):
14693
assert captured.out.split("\n")[0].split("=")[-1] == "false"
14794

@@ -168,41 +115,19 @@ def check_review_requirements_file(capsys, expected_teams: List):
168115
verify_review_requirements_file_contains_expected_teams(requirements_file_path, expected_teams)
169116

170117

171-
def test_find_mandatory_reviewers_backward_compatibility(capsys, not_strategic_backward_compatibility_change_expected_team):
172-
check_review_requirements_file(capsys, not_strategic_backward_compatibility_change_expected_team)
173-
174-
175-
def test_find_mandatory_reviewers_test_strictness_level(capsys, not_strategic_test_strictness_level_change_expected_team):
176-
check_review_requirements_file(capsys, not_strategic_test_strictness_level_change_expected_team)
177-
178-
179-
def test_find_mandatory_reviewers_not_strategic_bypass_reason(capsys, not_strategic_bypass_reason_file_change_expected_team):
180-
check_review_requirements_file(capsys, not_strategic_bypass_reason_file_change_expected_team)
181-
182-
183118
def test_find_mandatory_reviewers_ga(capsys, strategic_connector_file_change_expected_team):
184119
check_review_requirements_file(capsys, strategic_connector_file_change_expected_team)
185120

186121

187-
def test_find_mandatory_reviewers_strategic_backward_compatibility(
188-
capsys, strategic_connector_backward_compatibility_file_change_expected_team
189-
):
190-
check_review_requirements_file(capsys, strategic_connector_backward_compatibility_file_change_expected_team)
191-
192-
193-
def test_find_mandatory_reviewers_strategic_bypass_reason(capsys, strategic_connector_bypass_reason_file_change_expected_team):
194-
check_review_requirements_file(capsys, strategic_connector_bypass_reason_file_change_expected_team)
195-
196-
197-
def test_find_mandatory_reviewers_strategic_test_strictness_level(
198-
capsys, strategic_connector_test_strictness_level_file_change_expected_team
199-
):
200-
check_review_requirements_file(capsys, strategic_connector_test_strictness_level_file_change_expected_team)
201-
202-
203122
def test_find_mandatory_reviewers_breaking_change_release(capsys, test_breaking_change_release_expected_team):
204123
check_review_requirements_file(capsys, test_breaking_change_release_expected_team)
205124

206125

207126
def test_find_mandatory_reviewers_no_tracked_changed(capsys, not_strategic_not_tracked_change_expected_team):
208127
check_review_requirements_file(capsys, not_strategic_not_tracked_change_expected_team)
128+
129+
130+
def test_find_mandatory_reviewers_strategic_connector_breaking_change_release(
131+
capsys, strategic_connector_breaking_change_release_expected_teams
132+
):
133+
check_review_requirements_file(capsys, strategic_connector_breaking_change_release_expected_teams)

0 commit comments

Comments
 (0)