Skip to content

ci-connector-ops: Check test strictness level on GA source connectors #19383

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 14, 2022
Merged

ci-connector-ops: Check test strictness level on GA source connectors #19383

merged 6 commits into from
Nov 14, 2022

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Nov 14, 2022 •
edited
Loading

What

Closes #19378
Now the test_strictness_level option is available on SAT's acceptance-test-config.yml we want to enforce the usage of high test_strictness_level on GA connectors.

How

  • Create a new python package call ci-connector-ops. It can be used in the future for addition of othe CI checks our connector ops team might implement.
  • Add a script with a bunch of utils to check if the modified connectors are GA and have test_strictness_level == high in their acceptance-test-config.yml.
  • Create a new GitHub action workflow (connector_ops_ci.yml)with a Check test strictness level job, (again, this can evolve in the future with additional jobs such as running SAT 🥁 ). This job is triggered on PRs when a connector path is modified.

Recommended reading order

  1. airbyte/.github/workflows/connector_ops_ci.yml

    Line 1 in 0455882

    name: Connector Ops CI

  2. airbyte/tools/ci_connector_ops/ci_connector_ops/check_test_strictness_level.py

    Line 14 in 0455882

    def find_connectors_with_bad_strictness_level() -> List[str]:

  3. airbyte/tools/ci_connector_ops/ci_connector_ops/utils.py

    Line 11 in 0455882

    AIRBYTE_REPO = git.Repo(".")

🚨 User Impact 🚨

This check will fail in all existing PR modifying GA connectors. It's made to encourage connectors maintainers to set high test_strictness_level.

@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:09 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:11 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:15 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:22 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:25 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:27 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:30 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:34 Inactive
@alafanechere alafanechere force-pushed the augustin/ci/check-strictmode-on-ga branch from f5f223a to 663fae5 Compare November 14, 2022 15:40
@octavia-squidington-iv octavia-squidington-iv removed the area/connectors Connector related issues label Nov 14, 2022
@alafanechere alafanechere changed the title ci-connector-ops: Check test strictness level on GA connectors ci-connector-ops: Check test strictness level on GA source connectors Nov 14, 2022
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:44 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 15:47 Inactive
alafanechere
alafanechere commented Nov 14, 2022
View reviewed changes
airbyte-integrations/connectors/source-facebook-marketing/acceptance-test-config.yml Outdated
@@ -22,3 +22,4 @@ tests:
skip_comprehensive_incremental_tests: true
full_refresh:
- config_path: "secrets/config.json"
# TODO remove this change
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was only made to check the GitHub workflows works as expected. Must be reverted before merge.

@alafanechere alafanechere marked this pull request as ready for review November 14, 2022 16:07
@alafanechere alafanechere requested review from a team and evantahler November 14, 2022 16:08
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 16:17 Inactive
evantahler
evantahler approved these changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

Screenshot 2022-11-14 at 10 26 50 AM

It works!

This is great. One nit about a race condition, but 👍 from me!

.github/workflows/connector_ops_ci.yml
Comment on lines +20 to +21
- name: Install ci-connector-ops package
run: pip install --quiet -e ./tools/ci_connector_ops
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 running a local package on /this/ branch

.github/workflows/connector_ops_ci.yml
Comment on lines +4 to +6
pull_request:
paths:
- "airbyte-integrations/connectors/source-**"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 only runs on connector changes

tools/ci_connector_ops/ci_connector_ops/utils.py Outdated
Comment on lines 33 to 35
head_commit = AIRBYTE_REPO.head.commit
master_diffs = head_commit.diff(AIRBYTE_REPO.remotes.origin.refs.master)
changed_source_connector_files = [diff.b_path for diff in master_diffs if diff.b_path.startswith(SOURCE_CONNECTOR_PATH_PREFIX)]
Copy link
Contributor

Choose a reason for hiding this comment

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

neat!

tools/ci_connector_ops/ci_connector_ops/utils.py
return response.json()


OSS_CATALOG = download_catalog(OSS_CATALOG_URL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since you are using the published catalog, there's a race-condition if /this/ PR is changing the release stage that the test won't run. Could we look at the local YML files in this branch instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, you're right. Moreover, the approach I took to compute the diff would also detect connector changes made on master after the PR was opened and not rebased... I improved the behavior in 929e3b0

@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 22:31 Inactive
@octavia-squidington-iv octavia-squidington-iv removed the area/connectors Connector related issues label Nov 14, 2022
@alafanechere alafanechere temporarily deployed to more-secrets November 14, 2022 22:39 Inactive
@alafanechere alafanechere merged commit 1c961f4 into master Nov 14, 2022
@alafanechere alafanechere deleted the augustin/ci/check-strictmode-on-ga branch November 14, 2022 23:23
@github-actions github-actions bot mentioned this pull request Nov 15, 2022
Closed
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Nov 18, 2022
Merged
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
@alafanechere @akashkulk
ci-connector-ops: Check test strictness level on GA source connectors (…
5395bf9 
…#19383)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evantahler evantahler evantahler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make GA connector PR check fail if test strictness level is not high
3 participants
@alafanechere @evantahler @octavia-squidington-iv