QA checks: implement icon and documentation checks #21845

alafanechere · 2023-01-25T14:27:04Z

What

Closes #21715
Closes #21714
Closes #21712

Implement actual QA checks that will run on /test:

check if a documentation file exists
check an icon file is available
check the changelog is up to date

How

Create a Connector utility class to gather all the connector metadata we need for these checks (doc path, icon path etc.)

Implement :

check_documentation_file_exists
check_changelog_entry_is_updated
check_connector_icon_is_available
check_documentation_follows_guidelines -> I disabled this check as its too strict and will lead to multiple failures of GA connectors with docs slightly drifting from the expected template.

🚨 User Impact 🚨

The connectors failing at the following checks will get a failed build:

check_documentation_file_exists
check_changelog_entry_is_updated
check_connector_icon_is_available

github-actions · 2023-01-25T14:37:51Z

Airbyte Code Coverage

There is no coverage information present for the Files changed

Total Project Coverage	23.97%	❌

alafanechere · 2023-01-25T14:40:54Z

/test on source-facebook-marketing -> QA Checks are expected to pass

alafanechere · 2023-01-25T14:45:06Z

/test on source-hellobaton -> QA Checks are expected to fail because of missing docs

pedroslopez

Nice!

Q: At what point do you think we need to add tests for these checks? I think if we're relying on some of this to determine cloud availability we should have some level of testing around these tools.

tools/ci_connector_ops/ci_connector_ops/utils.py

pedroslopez · 2023-01-25T16:53:37Z

tools/ci_connector_ops/ci_connector_ops/qa_checks.py

+    return connector.documentation_file_path.exists()
+
+def check_documentation_follows_guidelines(connector: Connector) -> bool:
+    """Documentation guidelines are defined here https://hackmd.io/Bz75cgATSbm7DjrAqgl4rw"""


neat! I hadn't seen these guidelines.

Are these linked to from any official airbyte site (e.g. docs?) If so, may be worth pointing to the airbyte link rather than hackmd in case it ever changes...

@airbytehq/docs is https://hackmd.io/Bz75cgATSbm7DjrAqgl4rw making its way to our official doc?

Nope, not yet at least. The hackmd format lets contributors use the markdown format more easily than the Docs site.

I meant: is there a link to this template? Are these connectors doc guidelines shared in our official doc?

We link to it from our Contribution guide: https://docs.airbyte.com/contributing-to-airbyte/updating-documentation#additional-guidelines
Is that what you're looking for?

pedroslopez · 2023-01-25T16:57:56Z

tools/ci_connector_ops/ci_connector_ops/qa_checks.py

+    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


nit: This check is great to have, though having it run as the first part of the /check command feels like it might be a bit of a DX hit. I think in general people tend to work on changes and deal with bumping versions and changelog later on in the process, but from looking at the example runs, this is running as part of the integration-tests step and blocks other SATs from running.

This isn't really a comment on this specific set of changes, but I think it would be nice if we could separate the integration-tests step from these QA checks (and potentially have them run in parallel), so people can still /test and see the SAT results. In my mind integration tests and these QA checks are somewhat separate.

@pedroslopez I've moved the run-qa-check command in a different step of the test-command.yml workflow. I had to change the report.sh script to define the build as failing if test or QA checks are failing. (cc. @bechurch I think this script will help you understand how the build results are persisted to S3).

The changes are in 80fd32d .

Example run for source-pokeapi -> expected success

Example run for source-hellobaton -> expected failure

alafanechere · 2023-01-25T20:28:15Z

This isn't really a comment on this specific set of changes, but I think it would be nice if we could separate the integration-tests step from these QA checks (and potentially have them run in parallel), so people can still /test and see the SAT results. In my mind integration tests and these QA checks are somewhat separate.

I think you're 💯 right. I'll run QA checks in a different step, after SAT, but in the same /test workflow.

At what point do you think we need to add tests for these checks? I think if we're relying on some of this to determine cloud availability we should have some level of testing around these tools.

👮‍♂️ . I will, in this PR.

alafanechere · 2023-01-26T13:48:44Z

tools/ci_connector_ops/ci_connector_ops/utils.py

 from typing import Dict, Optional, Set, Tuple

 import git
 import requests
 import yaml

 AIRBYTE_REPO = git.Repo(".")
+DIFFED_BRANCH = "origin/master"


I declared this as a constant for easier mocking. When running unit tests we want to switch it to the active branch.

alafanechere · 2023-01-26T13:51:08Z

@pedroslopez I implemented your suggested changes:

Make QA checks run after integration tests in /test
Write tests for the QA checks + refactor the utils to use the new Connector dataclass

pedroslopez

alafanechere temporarily deployed to more-secrets January 25, 2023 14:29 — with GitHub Actions Inactive

implement checks

08e3def

alafanechere force-pushed the augustin/qa-checks/doc-and-icon-availability branch from c2cbf6a to 08e3def Compare January 25, 2023 14:31

alafanechere temporarily deployed to more-secrets January 25, 2023 14:33 — with GitHub Actions Inactive

alafanechere temporarily deployed to more-secrets January 25, 2023 14:36 — with GitHub Actions Inactive

alafanechere marked this pull request as ready for review January 25, 2023 14:38

alafanechere requested a review from a team January 25, 2023 14:38

alafanechere temporarily deployed to more-secrets January 25, 2023 14:41 — with GitHub Actions Inactive

improve check_changelog_entry_is_updated

c49df14

alafanechere force-pushed the augustin/qa-checks/doc-and-icon-availability branch from fc66c0b to c49df14 Compare January 25, 2023 14:43

alafanechere temporarily deployed to more-secrets January 25, 2023 14:45 — with GitHub Actions Inactive

alafanechere temporarily deployed to more-secrets January 25, 2023 14:46 — with GitHub Actions Inactive

alafanechere temporarily deployed to more-secrets January 25, 2023 14:54 — with GitHub Actions Inactive

pedroslopez approved these changes Jan 25, 2023

View reviewed changes

alafanechere temporarily deployed to more-secrets January 26, 2023 08:20 — with GitHub Actions Inactive

alafanechere force-pushed the augustin/qa-checks/doc-and-icon-availability branch 2 times, most recently from a606d4d to c9c8570 Compare January 26, 2023 09:16

alafanechere temporarily deployed to more-secrets January 26, 2023 09:19 — with GitHub Actions Inactive

alafanechere force-pushed the augustin/qa-checks/doc-and-icon-availability branch from c9c8570 to 9cc775e Compare January 26, 2023 09:31

alafanechere temporarily deployed to more-secrets January 26, 2023 09:34 — with GitHub Actions Inactive

alafanechere temporarily deployed to more-secrets January 26, 2023 09:53 — with GitHub Actions Inactive

worfklow improvement: run qa check in a separate step

80fd32d

alafanechere force-pushed the augustin/qa-checks/doc-and-icon-availability branch 2 times, most recently from a71b326 to a7bc513 Compare January 26, 2023 13:35

tests

3eccb4c

alafanechere force-pushed the augustin/qa-checks/doc-and-icon-availability branch from a7bc513 to 3eccb4c Compare January 26, 2023 13:38

octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 26, 2023

alafanechere force-pushed the augustin/qa-checks/doc-and-icon-availability branch from 2cb730a to 3eccb4c Compare January 26, 2023 13:42

octavia-squidington-iv removed the area/frontend Related to the Airbyte webapp label Jan 26, 2023

Merge branch 'master' into augustin/qa-checks/doc-and-icon-availability

89d3cb6

alafanechere temporarily deployed to more-secrets January 26, 2023 13:45 — with GitHub Actions Inactive

alafanechere commented Jan 26, 2023

View reviewed changes

alafanechere requested a review from pedroslopez January 26, 2023 13:54

pedroslopez approved these changes Jan 26, 2023

View reviewed changes

alafanechere merged commit 616e2e1 into master Jan 26, 2023

alafanechere deleted the augustin/qa-checks/doc-and-icon-availability branch January 26, 2023 15:03

This was referenced Jan 27, 2023

Bump Airbyte version from 0.40.29 to 0.40.30 #21954

Closed

Bump Airbyte version from 0.40.29 to 0.40.30 #21991

Closed

Bump Airbyte version from 0.40.29 to 0.40.30 #22045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QA checks: implement icon and documentation checks #21845

QA checks: implement icon and documentation checks #21845

alafanechere commented Jan 25, 2023 •

edited

Loading

github-actions bot commented Jan 25, 2023 •

edited

Loading

alafanechere commented Jan 25, 2023

alafanechere commented Jan 25, 2023 •

edited

Loading

pedroslopez left a comment

pedroslopez Jan 25, 2023

alafanechere Jan 26, 2023

Amruta-Ranade Jan 26, 2023

alafanechere Jan 26, 2023

Amruta-Ranade Jan 26, 2023

pedroslopez Jan 25, 2023

alafanechere Jan 26, 2023

alafanechere commented Jan 25, 2023

alafanechere Jan 26, 2023 •

edited

Loading

alafanechere commented Jan 26, 2023

pedroslopez left a comment

QA checks: implement icon and documentation checks #21845

QA checks: implement icon and documentation checks #21845

Conversation

alafanechere commented Jan 25, 2023 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

github-actions bot commented Jan 25, 2023 • edited Loading

Airbyte Code Coverage

alafanechere commented Jan 25, 2023

alafanechere commented Jan 25, 2023 • edited Loading

pedroslopez left a comment

Choose a reason for hiding this comment

pedroslopez Jan 25, 2023

Choose a reason for hiding this comment

alafanechere Jan 26, 2023

Choose a reason for hiding this comment

Amruta-Ranade Jan 26, 2023

Choose a reason for hiding this comment

alafanechere Jan 26, 2023

Choose a reason for hiding this comment

Amruta-Ranade Jan 26, 2023

Choose a reason for hiding this comment

pedroslopez Jan 25, 2023

Choose a reason for hiding this comment

alafanechere Jan 26, 2023

Choose a reason for hiding this comment

alafanechere commented Jan 25, 2023

alafanechere Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

alafanechere commented Jan 26, 2023

pedroslopez left a comment

Choose a reason for hiding this comment

alafanechere commented Jan 25, 2023 •

edited

Loading

github-actions bot commented Jan 25, 2023 •

edited

Loading

alafanechere commented Jan 25, 2023 •

edited

Loading

alafanechere Jan 26, 2023 •

edited

Loading