-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove release stage references in favor of support level or ab_internal #29232
Changes from 13 commits
320d982
473d01e
f714830
0e0c241
fd65b29
54e79c5
e7ddacc
abb3046
1861b95
4828b13
692842d
205fa6d
31aa865
0c7c6a6
ef3c687
00f0361
c571681
6c0260c
dce9b8e
9538097
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,13 +9,33 @@ | |
|
||
from connector_ops import utils | ||
|
||
RELEASE_STAGE_TO_STRICTNESS_LEVEL_MAPPING = {"generally_available": "high"} | ||
HIGH_TEST_STRICTNESS_LEVEL_THRESHOLD = { | ||
"sl": 300, | ||
"ql": 400, | ||
} | ||
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 is_important_connector(connector: utils.Connector) -> bool: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opted to use the idea of important connector which is either a high sl and/or high ql. This is because we may have a connector that is deemed qualitatively important (SL) and we should enable high strictness in advance of a high QL. defn: https://docs.google.com/document/d/161FF__wOjwS6j-1fVyHexDD1cJCQ5RJ2LMC8FI2N6yI/edit The alternative is to use just QL. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would love to see this exposed as a Connector property as it might be useful in |
||
"""Check if a connector requires higher scrutiny. | ||
|
||
Args: | ||
connector (utils.Connector): The connector to check. | ||
|
||
Returns: | ||
bool: True if the connector requires high test strictness level, False otherwise. | ||
""" | ||
if connector.ab_internal_sl >= HIGH_TEST_STRICTNESS_LEVEL_THRESHOLD["sl"]: | ||
return True | ||
|
||
if connector.ab_internal_ql >= HIGH_TEST_STRICTNESS_LEVEL_THRESHOLD["ql"]: | ||
return True | ||
|
||
return False | ||
|
||
|
||
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. | ||
|
@@ -30,51 +50,49 @@ def find_connectors_with_bad_strictness_level() -> List[utils.Connector]: | |
connectors_with_bad_strictness_level = [] | ||
changed_connector = utils.get_changed_connectors(destination=False, third_party=False) | ||
for connector in changed_connector: | ||
expected_test_strictness_level = RELEASE_STAGE_TO_STRICTNESS_LEVEL_MAPPING.get(connector.release_stage) | ||
can_check_strictness_level = all( | ||
[item is not None for item in [connector.release_stage, expected_test_strictness_level, connector.acceptance_test_config]] | ||
) | ||
if can_check_strictness_level: | ||
requires_high_test_strictness_level = is_important_connector(connector) | ||
check_for_high_strictness = connector.acceptance_test_config is not None and requires_high_test_strictness_level | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. absolutely! |
||
if check_for_high_strictness: | ||
try: | ||
assert connector.acceptance_test_config.get("test_strictness_level") == expected_test_strictness_level | ||
assert connector.acceptance_test_config.get("test_strictness_level") == "high" | ||
except AssertionError: | ||
connectors_with_bad_strictness_level.append(connector) | ||
return connectors_with_bad_strictness_level | ||
|
||
|
||
def find_changed_ga_connectors() -> Set[utils.Connector]: | ||
"""Find GA connectors modified on the current branch. | ||
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.release_stage == "generally_available"} | ||
return {connector for connector in changed_connectors if is_important_connector(connector)} | ||
|
||
|
||
def get_ga_bypass_reason_changes() -> Set[utils.Connector]: | ||
"""Find GA connectors that have modified bypass_reasons. | ||
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_ga_connectors()) | ||
return bypass_reason_changes.intersection(find_changed_important_connectors()) | ||
|
||
|
||
def find_mandatory_reviewers() -> List[Union[str, Dict[str, List]]]: | ||
ga_connector_changes = find_changed_ga_connectors() | ||
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_ga_bypass_reason_changes() | ||
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 ga_connector_changes: | ||
if important_connector_changes: | ||
return list(GA_CONNECTOR_REVIEWERS) | ||
return [] | ||
|
||
|
@@ -83,7 +101,7 @@ def check_test_strictness_level(): | |
connectors_with_bad_strictness_level = find_connectors_with_bad_strictness_level() | ||
if connectors_with_bad_strictness_level: | ||
logging.error( | ||
f"The following GA connectors must enable high test strictness level: {connectors_with_bad_strictness_level}. Please check this documentation for details: https://docs.airbyte.com/connector-development/testing-connectors/connector-acceptance-tests-reference/#strictness-level" | ||
f"The following connectors must enable high test strictness level: {connectors_with_bad_strictness_level}. Please check this documentation for details: https://docs.airbyte.com/connector-development/testing-connectors/connector-acceptance-tests-reference/#strictness-level" | ||
) | ||
sys.exit(1) | ||
else: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,15 +8,35 @@ | |
|
||
from connector_ops import utils | ||
|
||
RELEASE_STAGES_TO_CHECK = ["generally_available", "beta"] | ||
ALLOWED_HOST_THRESHOLD = { | ||
"sl": 200, | ||
"ql": 300, | ||
} | ||
|
||
def _requires_allowed_hosts(connector: utils.Connector) -> bool: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"""Check if a connector requires allowed hosts. | ||
|
||
Args: | ||
connector (utils.Connector): The connector to check. | ||
|
||
Returns: | ||
bool: True if the connector requires allowed hosts, False otherwise. | ||
""" | ||
if connector.ab_internal_sl >= ALLOWED_HOST_THRESHOLD["sl"]: | ||
return True | ||
|
||
if connector.ab_internal_ql >= ALLOWED_HOST_THRESHOLD["ql"]: | ||
return True | ||
|
||
return False | ||
|
||
|
||
def get_connectors_missing_allowed_hosts() -> List[utils.Connector]: | ||
connectors_missing_allowed_hosts: List[utils.Connector] = [] | ||
changed_connectors = utils.get_changed_connectors(destination=False, third_party=False) | ||
|
||
for connector in changed_connectors: | ||
if connector.release_stage in RELEASE_STAGES_TO_CHECK: | ||
if _requires_allowed_hosts(connector): | ||
missing = not connector_has_allowed_hosts(connector) | ||
if missing: | ||
connectors_missing_allowed_hosts.append(connector) | ||
|
@@ -31,7 +51,7 @@ def connector_has_allowed_hosts(connector: utils.Connector) -> bool: | |
def check_allowed_hosts(): | ||
connectors_missing_allowed_hosts = get_connectors_missing_allowed_hosts() | ||
if connectors_missing_allowed_hosts: | ||
logging.error(f"The following {RELEASE_STAGES_TO_CHECK} connectors must include allowedHosts: {connectors_missing_allowed_hosts}") | ||
logging.error(f"The following connectors must include allowedHosts: {connectors_missing_allowed_hosts}") | ||
sys.exit(1) | ||
else: | ||
sys.exit(0) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
from glob import glob | ||
from pathlib import Path | ||
from typing import List, Optional, Set, Tuple, Union | ||
from pydash.objects import get | ||
|
||
import git | ||
import requests | ||
|
@@ -272,9 +273,46 @@ def version_in_dockerfile_label(self) -> str: | |
def name_from_metadata(self) -> Optional[str]: | ||
return self.metadata.get("name") if self.metadata else None | ||
|
||
|
||
@property | ||
def support_level(self) -> Optional[str]: | ||
return self.metadata.get("supportLevel") if self.metadata else None | ||
|
||
@property | ||
def release_stage(self) -> Optional[str]: | ||
return self.metadata.get("releaseStage") if self.metadata else None | ||
def ab_internal_sl(self) -> int: | ||
"""Airbyte Internal Field. | ||
|
||
More info can be found here: https://www.notion.so/Internal-Metadata-Fields-32b02037e7b244b7934214019d0b7cc9 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a new page to notion since our wiki is now deprecated |
||
|
||
Returns: | ||
int: The value | ||
""" | ||
default_value = 100 | ||
sl_value = get(self.metadata, "ab_internal.sl") | ||
|
||
if sl_value is None: | ||
logging.warning(f"Connector {self.technical_name} does not have a `ab_internal.sl` defined in metadata.yaml. Defaulting to {default_value}") | ||
return default_value | ||
|
||
return sl_value | ||
|
||
@property | ||
def ab_internal_ql(self) -> int: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should already be defined but putting defaults here in case. These should be converted to hard errors on the introduction of metadata v2 where |
||
"""Airbyte Internal Field. | ||
|
||
More info can be found here: https://www.notion.so/Internal-Metadata-Fields-32b02037e7b244b7934214019d0b7cc9 | ||
|
||
Returns: | ||
int: The value | ||
""" | ||
default_value = 100 | ||
ql_value = get(self.metadata, "ab_internal.ql") | ||
|
||
if ql_value is None: | ||
logging.warning(f"Connector {self.technical_name} does not have a `ab_internal.ql` defined in metadata.yaml. Defaulting to {default_value}") | ||
return default_value | ||
|
||
return ql_value | ||
|
||
@property | ||
def allowed_hosts(self) -> Optional[List[str]]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,79 +100,6 @@ def compute_registry_overrides(merged_df): | |
return registries | ||
|
||
|
||
def merge_into_metadata_definitions( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it going away? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was an artifact from when we transformed the old registry into the metadata files at the very begining. Because we no longer do that, this function is unused, it just happened to be missed by the grim reaper last time |
||
id_field: str, connector_type: str, oss_connector_df: pd.DataFrame, cloud_connector_df: pd.DataFrame | ||
) -> List[PartialMetadataDefinition]: | ||
"""Merges the OSS and Cloud connector metadata into a single metadata definition. | ||
|
||
Args: | ||
id_field (str): The field that uniquely identifies a connector. | ||
connector_type (str): The type of connector (source or destination). | ||
oss_connector_df (pd.DataFrame): The dataframe containing the related OSS connector in the registry. | ||
cloud_connector_df (pd.DataFrame): The dataframe containing the related Cloud connector in the registry. | ||
|
||
Returns: | ||
pd.Series: The merged metadata definition. | ||
""" | ||
merged_connectors = pd.merge( | ||
oss_connector_df, cloud_connector_df, on=id_field, how="outer", suffixes=(OSS_SUFFIX, CLOUD_SUFFIX), indicator=True | ||
) | ||
|
||
# Replace numpy nan values with None | ||
sanitized_connectors = merged_connectors.replace([np.nan], [None]) | ||
|
||
def build_metadata(connector_registry_entry: dict) -> PartialMetadataDefinition: | ||
"""Builds the metadata definition for a single connector. | ||
|
||
Args: | ||
connector_registry_entry (dict): The merged connector metadata from the existing json registries. | ||
|
||
Returns: | ||
PartialMetadataDefinition: The final metadata definition. | ||
""" | ||
raw_data = { | ||
"name": get_field_with_fallback(connector_registry_entry, "name"), | ||
"definitionId": connector_registry_entry[id_field], | ||
"connectorType": connector_type, | ||
"dockerRepository": get_field_with_fallback(connector_registry_entry, "dockerRepository"), | ||
"githubIssueLabel": get_field_with_fallback(connector_registry_entry, "dockerRepository").replace("airbyte/", ""), | ||
"dockerImageTag": get_field_with_fallback(connector_registry_entry, "dockerImageTag"), | ||
"icon": get_field_with_fallback(connector_registry_entry, "icon"), | ||
"supportUrl": get_field_with_fallback(connector_registry_entry, "documentationUrl"), | ||
"connectorSubtype": get_field_with_fallback(connector_registry_entry, "sourceType"), | ||
"releaseStage": get_field_with_fallback(connector_registry_entry, "releaseStage"), | ||
"license": "MIT", | ||
"supportsDbt": get_field_with_fallback(connector_registry_entry, "supportsDbt"), | ||
"supportsNormalization": get_field_with_fallback(connector_registry_entry, "supportsNormalization"), | ||
"allowedHosts": get_field_with_fallback(connector_registry_entry, "allowedHosts"), | ||
"normalizationConfig": get_field_with_fallback(connector_registry_entry, "normalizationConfig"), | ||
"suggestedStreams": get_field_with_fallback(connector_registry_entry, "suggestedStreams"), | ||
"resourceRequirements": get_field_with_fallback(connector_registry_entry, "resourceRequirements"), | ||
} | ||
|
||
# remove none values | ||
data = {k: v for k, v in raw_data.items() if v is not None} | ||
|
||
metadata = {"metadataSpecVersion": "1.0", "data": data} | ||
|
||
registries = compute_registry_overrides(connector_registry_entry) | ||
metadata["data"]["registries"] = registries | ||
|
||
return PartialMetadataDefinition.construct(**metadata) | ||
|
||
metadata_list = [build_metadata(connector_registry_entry) for _, connector_registry_entry in sanitized_connectors.iterrows()] | ||
|
||
return metadata_list | ||
|
||
|
||
def validate_metadata(metadata: PartialMetadataDefinition) -> tuple[bool, str]: | ||
try: | ||
ConnectorMetadataDefinitionV0.parse_obj(metadata) | ||
return True, None | ||
except Exception as e: | ||
return False, str(e) | ||
|
||
|
||
# ASSETS | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the metadata are up to date to match the same subset of connector nightly build originally targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They dont match the same subset unfortunately. There will likely be a bit of a mix up