Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

require extra review for breaking change messages #31361

Merged
merged 15 commits into from
Oct 16, 2023
Prev Previous commit
Next Next commit
rename
  • Loading branch information
pedroslopez committed Oct 16, 2023
commit 000ef28b7c5136b31c70f6938ef2eeadc635103a
Original file line number Diff line number Diff line change
@@ -4,18 +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"}
BREAKING_CHANGE_REVIEWERS = {"breaking-change-reviewers"}
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.
@@ -39,49 +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]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving it to a separate module!

"""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]]]]:
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()
breaking_change_changes = utils.get_changed_metadata(diff_regex="upgradeDeadline")

required_reviewers = []

if backward_compatibility_changes:
required_reviewers.append({"name": "Backwards Compatibility Test Skip", "teams": list(BACKWARD_COMPATIBILITY_REVIEWERS)})
if test_strictness_level_changes:
required_reviewers.append({"name": "Acceptance Test Strictness Level", "teams": list(TEST_STRICTNESS_LEVEL_REVIEWERS)})
if ga_bypass_reason_changes:
required_reviewers.append({"name": "GA Acceptance Test Bypass", "teams": list(GA_BYPASS_REASON_REVIEWERS)})
if important_connector_changes:
required_reviewers.append({"name": "GA Connectors", "teams": list(GA_CONNECTOR_REVIEWERS)})
if breaking_change_changes:
required_reviewers.append({"name": "Breaking Changes", "teams": list(BREAKING_CHANGE_REVIEWERS)})

return required_reviewers


def check_test_strictness_level():
connectors_with_bad_strictness_level = find_connectors_with_bad_strictness_level()
if connectors_with_bad_strictness_level:
@@ -91,23 +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 = [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)}")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved all required reviewer-related stuff here since it was no longer just for acceptance test config

Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
#
# 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]]]]:
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()
breaking_change_changes = utils.get_changed_metadata(diff_regex="upgradeDeadline")

required_reviewers = []

if backward_compatibility_changes:
required_reviewers.append({"name": "Backwards Compatibility Test Skip", "teams": list(BACKWARD_COMPATIBILITY_REVIEWERS)})
if test_strictness_level_changes:
required_reviewers.append({"name": "Acceptance Test Strictness Level", "teams": list(TEST_STRICTNESS_LEVEL_REVIEWERS)})
if ga_bypass_reason_changes:
required_reviewers.append({"name": "GA Acceptance Test Bypass", "teams": list(GA_BYPASS_REASON_REVIEWERS)})
if important_connector_changes:
required_reviewers.append({"name": "GA Connectors", "teams": list(GA_CONNECTOR_REVIEWERS)})
if breaking_change_changes:
required_reviewers.append({"name": "Breaking Changes", "teams": list(BREAKING_CHANGE_REVIEWERS)})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a mapping between change detection function and mandatory reviewers can be more readable


return required_reviewers


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)}")
4 changes: 2 additions & 2 deletions airbyte-ci/connectors/connector_ops/pyproject.toml
Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@ freezegun = "^1.1.0"

[tool.poetry.scripts]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please bump the package version 🙏

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"
Original file line number Diff line number Diff line change
@@ -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


@@ -35,7 +35,7 @@ def ga_connector_file():

@pytest.fixture
def not_ga_backward_compatibility_change_expected_team(tmp_path, pokeapi_acceptance_test_config_path) -> List:
expected_teams = 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:
@@ -46,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 = 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:
@@ -79,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:
@@ -90,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 = 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:
@@ -101,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 = 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:
@@ -112,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 = 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:
@@ -123,7 +123,7 @@ def ga_connector_test_strictness_level_file_change_expected_team(tmp_path, ga_co

@pytest.fixture
def test_breaking_change_release_expected_team(tmp_path, pokeapi_metadata_path) -> List:
expected_teams = list(acceptance_test_config_checks.BREAKING_CHANGE_REVIEWERS)
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:
@@ -149,13 +149,13 @@ def verify_review_requirements_file_contains_expected_teams(requirements_file_pa


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)