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

cat/connectors-ci: replace docker runner with dagger runner in CAT #28000

Merged
merged 35 commits into from
Aug 11, 2023

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Jul 6, 2023

What

Closes #27858.
In CAT, we want to run connector under test container with Dagger instead of docker run.
Reasons are explained in #27858.
Dagger requires Python 3.10, CAT is running Python 3.9,: we have to upgrade CAT to Python 3.10, this explain the breadth of changes in this PR.

How

1. Upgrade CAT to Python 3.10

  • Remove the connector dependency to CAT (by editing setup.py and requirements.txt), because connectors run on Python 3.9. I don't think this dependency is required anymore as CAT should be run from airbyte-ci, or with acceptance-test-docker.sh, not from connectors.
  • Remove CAT dependency to airbyte-cdk, which is running Python 3.9. Replacing this dependency to airbyte-protocol.models makes more sense: CAT is not only made to test python connectors and only uses the protocol models.
  • Upgrade CAT dependency whose previous version are not working with 3.10 (hypothesis-json-schema)
  • Manage CAT with poetry and update its Dockerfile accordingly

2. Refactor the ConnectorRunner to use dagger

We can get rid of a lot of docker related code and use Dagger API to provision the connector under test container with the required resources. The container under test image is passed from airbyte-ci using a tarball.

3. Change airbyte-ci to run CAT with dagger-in-dagger instead of docker-in-docker

We can get rid of the the docker-in-docker paradigm. Binding CAT to a docker daemon sidecar container is not longer required and will reduce the number of moving parts and, hopefully, transient failures.
As CAT is now built on the fly it's also easier to make CAT changes and test these change on the same PR.

4. Make sure that other way of running CAT (directly via python execution or with acceptance-test-docker.sh is still supported

  • acceptance-test-docker.sh still works
  • Running CAT from connector python venv does not work anymore as CAT is not a connector dependency and is using python 3.10 vs 3.9 on connectors.

5. Update CAT unit test to make them pass.

  • TODO

6. Publish CAT 1.0.0

  • TODO

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan and you've followed all steps in the Breaking Changes Checklist
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • The connector tests are passing in CI
  • You've updated the connector's metadata.yaml file (new!)
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

Comment on lines -182 to -193
@pytest.fixture(scope="session", autouse=True)
def pull_docker_image(acceptance_test_config) -> None:
"""Startup fixture to pull docker image"""
image_name = acceptance_test_config.connector_image
config_filename = "acceptance-test-config.yml"
try:
ConnectorRunner(image_name=image_name, volume=Path("."))
except errors.ImageNotFound:
pytest.exit(f"Docker image `{image_name}` not found, please check your {config_filename} file", returncode=1)
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 is not needed as Dagger will use the container passed by airbyte-ci, build the container according to the local docker file, or pull the image if it's not :dev

Copy link
Contributor

Choose a reason for hiding this comment

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

Love to see try and ifs get deleted

@@ -240,3 +240,17 @@ def _scan_schema(subschema, path=""):

_scan_schema(schema)
return paths


def flatten_tuples(to_flatten):
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 function was orignally imported from the old jsonschema version, it was a private function that disappeared in the newest jsonschema package version, I redeclared it, as it, here.

@@ -109,30 +109,6 @@ def test_verify_records_schema(configured_catalog: ConfiguredAirbyteCatalog):
({"a": "2021-08-10T12:43:15Z"}, {"type": "object", "properties": {"a": {"type": "string", "format": "date-time"}}}, True),
({"a": "2018-11-13T20:20:39+00:00"}, {"type": "object", "properties": {"a": {"type": "string", "format": "date-time"}}}, True),
({"a": "2018-21-13T20:20:39+00:00"}, {"type": "object", "properties": {"a": {"type": "string", "format": "date-time"}}}, False),
# This is valid for postgres sql but not valid for bigquery
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following format are not supported by Airbyte.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was there any harm in leaving this in?


@property
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this code is not useful anymore as the Dagger API makes this straightforward.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is great!

@alafanechere alafanechere marked this pull request as ready for review July 7, 2023 13:48
@alafanechere alafanechere requested review from a team July 7, 2023 14:17
@alafanechere
Copy link
Contributor Author

alafanechere commented Jul 7, 2023

Trying out a nightly build on this branch: https://github.com/airbytehq/airbyte/actions/runs/5488096627

@alafanechere
Copy link
Contributor Author

alafanechere commented Aug 11, 2023

/legacy-publish connector=bases/connector-acceptance-test auto-bump-version=false

Error: Unexpected inputs provided: ["auto-bump-version"]

@alafanechere
Copy link
Contributor Author

alafanechere commented Aug 11, 2023

/legacy-publish connector=bases/connector-acceptance-test

🕑 Publishing the following connectors:
bases/connector-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/5834009213


Connector Version Did it publish?
bases/connector-acceptance-test

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@alafanechere
Copy link
Contributor Author

alafanechere commented Aug 11, 2023

/legacy-publish connector=bases/connector-acceptance-test

🕑 Publishing the following connectors:
bases/connector-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/5834086316


Connector Version Did it publish?
bases/connector-acceptance-test

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@octavia-squidington-iii
Copy link
Collaborator

source-persistiq test report (commit 301f9623b0) - ❌

⏲️ Total pipeline duration: 01mn30s

Step Result
Validate airbyte-integrations/connectors/source-persistiq/metadata.yaml
Connector version semver check
Connector version increment check
QA checks
Code format checks
Connector package install

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-persistiq test

@octavia-squidington-iii
Copy link
Collaborator

source-waiteraid test report (commit 301f9623b0) - ❌

⏲️ Total pipeline duration: 01mn55s

Step Result
Validate airbyte-integrations/connectors/source-waiteraid/metadata.yaml
Connector version semver check
Connector version increment check
QA checks
Code format checks
Connector package install
Build source-waiteraid docker image for platform linux/x86_64
Acceptance tests

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-waiteraid test

@alafanechere
Copy link
Contributor Author

The publish does not work anymore because:

* Where:
Build file '/actions-runner/_work/airbyte/airbyte/airbyte-integrations/bases/connector-acceptance-test/build.gradle' line: 3

* What went wrong:
An exception occurred applying plugin request [id: 'airbyte-python']
> Failed to apply plugin 'airbyte-python'.
   > Error: Python module lacks requirement.txt and setup.py

@alafanechere
Copy link
Contributor Author

alafanechere commented Aug 11, 2023

/legacy-publish connector=bases/connector-acceptance-test

🕑 Publishing the following connectors:
bases/connector-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/5834340676


Connector Version Did it publish?
bases/connector-acceptance-test

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@alafanechere
Copy link
Contributor Author

alafanechere commented Aug 11, 2023

/legacy-publish connector=bases/connector-acceptance-test run-tests=false

🕑 Publishing the following connectors:
bases/connector-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/5834457894


Connector Version Did it publish?
bases/connector-acceptance-test

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@alafanechere alafanechere merged commit 00d9462 into master Aug 11, 2023
15 of 21 checks passed
@alafanechere alafanechere deleted the augustin/cat/dagger-runner branch August 11, 2023 15:58
@girarda girarda mentioned this pull request Aug 11, 2023
harrytou pushed a commit to KYVENetwork/airbyte that referenced this pull request Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connector-ci: Use Dagger in CAT
4 participants