QA checks: implement icon and documentation checks (#21845)

.github/workflows/test-command.yml

@@ -146,7 +146,6 @@ jobs:
             **/normalization_test_output/**/build/compiled/airbyte_utils/**
             **/normalization_test_output/**/build/run/airbyte_utils/**
             **/normalization_test_output/**/models/generated/**
-
       - name: Test coverage reports artifacts
         if: github.event.inputs.comment-id && success()
         uses: actions/upload-artifact@v3
@@ -155,10 +154,14 @@ jobs:
           path: |
             **/${{ github.event.inputs.connector }}/htmlcov/**
           retention-days: 3
-
+      - name: Run QA checks for ${{ github.event.inputs.connector }}
+        id: qa_checks
+        if: always()
+        run: |
+          run-qa-checks ${{ github.event.inputs.connector }}
       - name: Report Status
         if: github.ref == 'refs/heads/master' && always()
-        run: ./tools/status/report.sh ${{ github.event.inputs.connector }} ${{github.repository}} ${{github.run_id}} ${{steps.test.outcome}}
+        run: ./tools/status/report.sh ${{ github.event.inputs.connector }} ${{github.repository}} ${{github.run_id}} ${{steps.test.outcome}} ${{steps.qa_checks.outcome}}
         env:
           AWS_ACCESS_KEY_ID: ${{ secrets.STATUS_API_AWS_ACCESS_KEY_ID }}
           AWS_SECRET_ACCESS_KEY: ${{ secrets.STATUS_API_AWS_SECRET_ACCESS_KEY }}

tools/bin/ci_integration_test.sh

@@ -32,7 +32,6 @@ else
     export SUB_BUILD="CONNECTORS_BASE"
   elif [[ "$connector" == *"connectors"* ]]; then
     connector_name=$(echo $connector | cut -d / -f 2)
-    run-qa-checks $connector_name
     selected_integration_test=$(echo "$all_integration_tests" | grep "^$connector_name$" || echo "")
     integrationTestCommand="$(_to_gradle_path "airbyte-integrations/$connector" integrationTest)"
   else

tools/ci_connector_ops/ci_connector_ops/qa_checks.py

@@ -2,65 +2,106 @@
 # Copyright (c) 2023 Airbyte, Inc., all rights reserved.
 #
 
+
 import sys
 
-def check_documentation_markdown_file(connector_name: str) -> bool:
+from ci_connector_ops.utils import Connector
+
+
+def check_documentation_file_exists(connector: Connector) -> bool:
     """Check if a markdown file with connector documentation is available 
-    in docs/integrations/<connector-type>/<connector-name>.md
+    in docs/integrations/<connector-type>s/<connector-name>.md
 
     Args:
-        connector_name (str): The connector name
+        connector (Connector): a Connector dataclass instance.
 
     Returns:
         bool: Wether a documentation file was found.
     """
-    # TODO implement
-    return True
 
-def check_changelog_entry_is_updated(connector_name: str) -> bool:
+    return connector.documentation_file_path.exists()
+
+def check_documentation_follows_guidelines(connector: Connector) -> bool:
+    """Documentation guidelines are defined here https://hackmd.io/Bz75cgATSbm7DjrAqgl4rw"""
+    follows_guidelines = True
+    with open(connector.documentation_file_path) as f:
+        doc_lines = [l.lower() for l in f.read().splitlines()]
+    if not doc_lines[0].startswith("# "):
+        print("The connector name is not used as the main header in the documentation.")
+        follows_guidelines = False
+    if connector.definition: # We usually don't have a definition if the connector is not published.
+        if doc_lines[0].strip() != f"# {connector.definition['name'].lower()}":
+            print("The connector name is not used as the main header in the documentation.")
+            follows_guidelines = False
+    elif not doc_lines[0].startswith("# "):
+        print("The connector name is not used as the main header in the documentation.")
+        follows_guidelines = False
+
+    expected_sections = [
+        "## Prerequisites", 
+        "## Setup guide",
+        "## Supported sync modes",
+        "## Supported streams",
+        "## Changelog"
+    ]
+
+    for expected_section in expected_sections:
+        if expected_section.lower() not in doc_lines:
+            print(f"Connector documentation is missing a '{expected_section.replace('#', '').strip()}' section.")
+            follows_guidelines = False
+    return follows_guidelines
+
+def check_changelog_entry_is_updated(connector: Connector) -> bool:
     """Check that the changelog entry is updated for the latest connector version
     in docs/integrations/<connector-type>/<connector-name>.md
 
     Args:
-        connector_name (str): The connector name
+        connector (Connector): a Connector dataclass instance.
 
     Returns:
         bool: Wether a the changelog is up to date.
     """
-    # TODO implement
-    return True
-
-def check_connector_icon_is_available(connector_name: str) -> bool:
+    if not check_documentation_file_exists(connector):
+        return False
+    with open(connector.documentation_file_path) as f:
+        after_changelog = False
+        for line in f:
+            if "# changelog" in line.lower():
+                after_changelog = True
+            if after_changelog and connector.version in line:
+                return True
+    return False
+
+def check_connector_icon_is_available(connector: Connector) -> bool:
     """Check an SVG icon exists for a connector in
     in airbyte-config/init/src/main/resources/icons/<connector-name>.svg
 
     Args:
-        connector_name (str): The connector name
+        connector (Connector): a Connector dataclass instance.
 
     Returns:
         bool: Wether an icon exists for this connector.
     """
-    # TODO implement
-    return True
+    return connector.icon_path.exists()
 
-def check_connector_https_url_only(connector_name: str) -> bool:
+def check_connector_https_url_only(connector: Connector) -> bool:
     """Check a connector code contains only https url.
 
     Args:
-        connector_name (str): The connector name
+        connector (Connector): a Connector dataclass instance.
 
     Returns:
         bool: Wether the connector code contains only https url.
     """
     # TODO implement
     return True
 
-def check_connector_has_no_critical_vulnerabilities(connector_name: str) -> bool:
+def check_connector_has_no_critical_vulnerabilities(connector: Connector) -> bool:
     """Check if the connector image is free of critical Snyk vulnerabilities.
     Runs a docker scan command.
 
     Args:
-        connector_name (str): The connector name
+        connector (Connector): a Connector dataclass instance.
 
     Returns:
         bool: Wether the connector is free of critical vulnerabilities.
@@ -69,23 +110,29 @@ def check_connector_has_no_critical_vulnerabilities(connector_name: str) -> bool
     return True
 
 QA_CHECKS = [
-    check_documentation_markdown_file,
+    check_documentation_file_exists,
+    # Disabling the following check because it's likely to not pass on a lot of connectors.
+    # check_documentation_follows_guidelines, 
     check_changelog_entry_is_updated,
     check_connector_icon_is_available,
     check_connector_https_url_only,
     check_connector_has_no_critical_vulnerabilities
 ]
 
 def run_qa_checks():
-    connector_name = sys.argv[1]
-    print(f"Running QA checks for {connector_name}")
-    qa_check_results = {qa_check.__name__: qa_check(connector_name) for qa_check in QA_CHECKS}
+    connector_technical_name = sys.argv[1].split("/")[-1]
+    if not connector_technical_name.startswith("source-") and not connector_technical_name.startswith("destination-"):
+        print("No QA check to run as this is not a connector.")
+        sys.exit(0)
+    connector = Connector(connector_technical_name)
+    print(f"Running QA checks for {connector_technical_name}:{connector.version}")
+    qa_check_results = {qa_check.__name__: qa_check(connector) for qa_check in QA_CHECKS}
     if not all(qa_check_results.values()):
-        print(f"QA checks failed for {connector_name}")
+        print(f"QA checks failed for {connector_technical_name}:{connector.version}:")
         for check_name, check_result in qa_check_results.items():
             check_result_prefix = "✅" if check_result else "❌"
             print(f"{check_result_prefix} - {check_name}")
         sys.exit(1)
     else:
-        print(f"All QA checks succeeded for {connector_name}")
+        print(f"All QA checks succeeded for {connector_technical_name}:{connector.version}")
         sys.exit(0)

tools/ci_connector_ops/ci_connector_ops/sat_config_checks.py

@@ -15,40 +15,38 @@
 GA_CONNECTOR_REVIEWERS = {"gl-python"}
 REVIEW_REQUIREMENTS_FILE_PATH = ".github/connector_org_review_requirements.yaml"
 
-def find_connectors_with_bad_strictness_level() -> List[str]:
+def find_connectors_with_bad_strictness_level() -> List[utils.Connector]:
     """Check if changed connectors have the expected SAT test strictness level according to their release stage.
     1. Identify changed connectors
     2. Retrieve their release stage from the catalog
     3. Parse their acceptance test config file
     4. Check if the test strictness level matches the strictness level expected for their release stage.
 
     Returns:
-        List[str]: List of changed connector names that are not matching test strictness level expectations.
+        List[utils.Connector]: List of changed connector that are not matching test strictness level expectations.
     """
     connectors_with_bad_strictness_level = []
-    changed_connector_names = utils.get_changed_connector_names()
-    for connector_name in changed_connector_names:
-        connector_release_stage = utils.get_connector_release_stage(connector_name)
-        expected_test_strictness_level = RELEASE_STAGE_TO_STRICTNESS_LEVEL_MAPPING.get(connector_release_stage)
-        _, acceptance_test_config = utils.get_acceptance_test_config(connector_name)
+    changed_connector = utils.get_changed_connectors()
+    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, acceptance_test_config]]
+            [item is not None for item in [connector.release_stage, expected_test_strictness_level, connector.acceptance_test_config]]
         )
         if can_check_strictness_level:
             try:
-                assert acceptance_test_config.get("test_strictness_level") == expected_test_strictness_level
+                assert connector.acceptance_test_config.get("test_strictness_level") == expected_test_strictness_level
             except AssertionError:
-                connectors_with_bad_strictness_level.append(connector_name)
+                connectors_with_bad_strictness_level.append(connector)
     return connectors_with_bad_strictness_level
 
-def find_changed_ga_connectors() -> List[str]:
+def find_changed_ga_connectors() -> List[utils.Connector]:
     """Find GA connectors modified on the current branch.
 
     Returns:
-        List[str]: The list of GA connector that were modified on the current branch.
+        List[utils.Connector]: The list of GA connectors that were modified on the current branch.
     """
-    changed_connector_names = utils.get_changed_connector_names()
-    return [connector_name for connector_name in changed_connector_names if utils.get_connector_release_stage(connector_name) == "generally_available"]
+    changed_connectors = utils.get_changed_connectors()
+    return [connector for connector in changed_connectors if connector.release_stage == "generally_available"]
 
 def find_mandatory_reviewers() -> List[Union[str, Dict[str, List]]]:
     ga_connector_changes = find_changed_ga_connectors()