connectors-ci: make source-file testable in airbyte-ci #27107

alafanechere · 2023-06-07T07:33:35Z

What

Relates to #25053

Fixing `source-file`

source-file integration tests uses docker-compose to spin up a sftp container used for testing.
To make the container orchestration and connection work in airbyte-ci we should bind python integration test to the global docker host.
The docker host should also have access to the file we want to mount of sftp, to do so we move them to the /tmp folder so that both the container under test, docker host have access to it and the docker host can mount them to the sftp containers.

Fixing `source-file-secure`

source-file-secure has an implicit dependency to source-file: its docker image is based on source-file docker image in which the source-file package was installed.
But, outside of the dockerized execution, executing import source_file in python in source-file-secure fails because our pipeline did not handle installing other connector package.
To fix this problem:

Dependency to source-file in source-file-secure is declared explicitely inside its setup.py (@evantahler we previously chatted about the benefits of explicitely variants dependencies in setup.py)
We detect dependencies to mount in the dagger pipeline by running python setup.py egg_ingo
We mount the required dependencies when installing the source-file-secure in the test environment.
When building the source-file-secure docker image we mount and install the same dependencies.

github-actions · 2023-06-07T07:33:55Z

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
You, or an Airbyter, have run /test successfully on this PR - or on a non-forked branch
You've updated the connector's metadata.yaml file (new!)

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

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

alafanechere · 2023-06-07T16:30:08Z

airbyte-integrations/connectors/source-file-secure/Dockerfile

+### WARNING ###
+# This Dockerfile will soon be deprecated.
+# It is not used to build the connector image we publish to DockerHub.
+# The new logic to build the connector image is declared with Dagger here:
+# https://github.com/airbytehq/airbyte/blob/master/tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py#L771
+
+# If you need to add a custom logic to build your connector image, you can do it by adding a finalize_build.sh or finalize_build.py script in the connector folder.
+# Please reach out to the Connectors Operations team if you have any question.


source-file-secure is now fully built from Dagger. As source-file needs to be mounted at build time it was easier to accomplish it with Dagger.

alafanechere · 2023-06-07T18:39:59Z

airbyte-integrations/connectors/source-file-secure/requirements.txt

@@ -1,3 +1,2 @@
 -e ../../bases/connector-acceptance-test
-e ../source-file


source-file dependency is now explicitly defined in setup.py.

alafanechere · 2023-06-07T18:41:07Z

airbyte-integrations/connectors/source-file-secure/setup.py


 from setuptools import find_packages, setup

+
+def local_dependency(name: str) -> str:


Compute the absolute path to the local dependencies. In dagger pipelines we mount local dependencies to /local_dependencies.

Oh dear python!

I like this solution.

My only reservations are having

Dagger relying on file:// to mark relative dependencies, or pip -e as this isnt always the case (example)

our connector setup.py's relying on knowing how dagger works under the hood.

So I want to ask a question for #1 and propose a possible idea for 2

I dont think we can do away with this, but how come we dont mount the relative import to the same relative location in the dagger file tree?

What if, in the dagger machine, we symlinked the local host path to the dagger /local_dependencies version? Would remove the need for having DAGGER_BUILD in our connectors folder.

alafanechere · 2023-06-07T18:50:25Z

airbyte-integrations/connectors/source-file-secure/source_file_secure/source.py

-try:
-    import source_file.source
-except ModuleNotFoundError:
-    current_dir = os.path.dirname(os.path.abspath(__file__))
-    parent_source_local = os.path.join(current_dir, "../../source-file")
-    if os.path.isdir(parent_source_local):
-        sys.path.append(parent_source_local)
-    else:
-        raise RuntimeError("not found parent source folder")
-    import source_file.source


This is not required anymore as the source file connector is installed as a normal python deps with pip install.

…dagger

This reverts commit b6c9c47.

github-actions · 2023-06-07T22:53:34Z

Affected Connector Report

The latest commit has removed all connector-related changes. There are no more dependent connectors for this PR.

alafanechere · 2023-06-07T22:59:32Z

/test connector=connectors/source-file

🕑 connectors/source-file https://github.com/airbytehq/airbyte/actions/runs/5205532222
✅ connectors/source-file https://github.com/airbytehq/airbyte/actions/runs/5205532222
Python tests coverage:

Name                      Stmts   Miss  Cover
---------------------------------------------
source_file/__init__.py       2      0   100%
source_file/utils.py         13      1    92%
source_file/source.py        83      7    92%
source_file/client.py       335     56    83%
---------------------------------------------
TOTAL                       433     64    85%
Name                      Stmts   Miss  Cover
---------------------------------------------
source_file/__init__.py       2      0   100%
source_file/client.py       335     56    83%
source_file/utils.py         13      8    38%
source_file/source.py        83     62    25%
---------------------------------------------
TOTAL                       433    126    71%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: Incremental syncs are not supported on this connector.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:598: The previous and actual discovered catalogs are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:695: This tests currently leads to too much failures. We need to fix the connectors at scale first.
======================== 35 passed, 4 skipped in 53.89s ========================

alafanechere · 2023-06-07T22:59:37Z

/test connector=connectors/source-file-secure

🕑 connectors/source-file-secure https://github.com/airbytehq/airbyte/actions/runs/5205532489
❌ connectors/source-file-secure https://github.com/airbytehq/airbyte/actions/runs/5205532489
🐛 https://gradle.com/s/sys35fyaxoyew

Build Failed

Test summary info:

Could not find result summary

evantahler · 2023-06-08T00:10:56Z

While out of scope for this PR, I have the opinion that using local docker containers for testing connectors is bad for a number of reasons:

CI anti-patterns like this
slow to spin up and down containers for each test/suite
false sense of isolation. we are in general poor at clearning up files/dbs after tests. This is a problem masked by destroying the container which appears when moving to a hosted DB, like testing snowflake
unobservable. If a test fails, it is hard to connect to the DB or S3 bucked to see what went wrong

To that end, I would propose that we work to remove all test containers like this, and set up persistent test hosts for all connectors

evantahler

👍 from me, mainly about explicit dependencies!
I'd wait for the other reviews on the 🐍 code before merging

evantahler · 2023-06-08T00:12:03Z

airbyte-integrations/connectors/source-file-secure/Dockerfile

+
+# If you need to add a custom logic to build your connector image, you can do it by adding a finalize_build.sh or finalize_build.py script in the connector folder.
+# Please reach out to the Connectors Operations team if you have any question.
+FROM airbyte/source-file:0.3.10


evantahler · 2023-06-08T00:12:34Z

airbyte-integrations/connectors/source-file-secure/setup.py

@@ -21,8 +32,10 @@
    "xlrd==2.0.1",
    "openpyxl==3.0.10",
    "pyxlsb==1.0.9",
+    local_dependency("source-file"),


evantahler · 2023-06-08T00:14:59Z

airbyte-integrations/connectors/source-file/setup.py

@@ -24,7 +24,7 @@
    "pyxlsb==1.0.9",
 ]

-TEST_REQUIREMENTS = ["pytest~=6.2", "pytest-docker~=1.0.0", "pytest-mock~=3.6.1"]
+TEST_REQUIREMENTS = ["pytest~=6.2", "pytest-docker~=1.0.0", "pytest-mock~=3.6.1", "docker-compose"]


Is this a python package to work with docker?

The source-file integration test use docker-compose without specifying it as a test dependency. I added this so that our pipeline installs it within the test environment, that does not have docker-compose by default.

bnchrch

Great work here @alafanechere

This must have been annoying to work through

I had one alternative idea, but Im not positive it would work and in the spirit of "done not perfect"

bnchrch · 2023-06-08T01:53:57Z

airbyte-integrations/connectors/source-file-secure/setup.py


 from setuptools import find_packages, setup

+
+def local_dependency(name: str) -> str:


Oh dear python!

I like this solution.

My only reservations are having

Dagger relying on file:// to mark relative dependencies, or pip -e as this isnt always the case (example)

our connector setup.py's relying on knowing how dagger works under the hood.

So I want to ask a question for #1 and propose a possible idea for 2

I dont think we can do away with this, but how come we dont mount the relative import to the same relative location in the dagger file tree?

What if, in the dagger machine, we symlinked the local host path to the dagger /local_dependencies version? Would remove the need for having DAGGER_BUILD in our connectors folder.

bnchrch · 2023-06-08T01:54:20Z

tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py

+    if await get_file_contents(container, "setup.py"):
+        container_with_egg_info = container.with_exec(["python", "setup.py", "egg_info"])
+        egg_info_output = await container_with_egg_info.stdout()
+        for line in egg_info_output.split("\n"):


This must ahve been painful, well done

bnchrch · 2023-06-08T01:54:44Z

tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py

+
+    if await get_file_contents(container, "setup.py"):
+        container = container.with_exec(install_connector_package_cmd)
+    if await get_file_contents(container, "requirements.txt"):


Nit: We likely will need on for pyproject.toml at some point.

alafanechere · 2023-06-08T13:28:38Z

My only reservations are having

Dagger relying on file:// to mark relative dependencies, or pip -e as this isnt always the case (example)
our connector setup.py's relying on knowing how dagger works under the hood.

I believe there's indeed a cleaner solution I'd like to tackle later:
Have a a single function that buids a python connector container by:

Detecting its local dependencies from pyproject.toml, requirements.txt or setup.py
Mounting these dependencies + the connector code with path relative to the airbyte repo
Install the dependencies according to the context (eg - install tests deps if it's the container to run tests, or install a specific cdk version if the code change has cdk change etc.)

Then we'll have a same base container on top of which we can:

run unit/integration tests
build for CAT and publish

I did not take this approach right now because it was too much refacto, and as we still rely on dockerfiles for python connectors (except source-file-secure) we still have a different container for unit/integration tests (dynamically built with dagger) and one for build and publish based on the Dockerfile.

This is definitely something we'll tackle when we'll want to remove dockerfiles for Python connectors.

alafanechere · 2023-06-08T13:31:11Z

/test connector=connectors/source-file-secure

🕑 connectors/source-file-secure https://github.com/airbytehq/airbyte/actions/runs/5211825882
❌ connectors/source-file-secure https://github.com/airbytehq/airbyte/actions/runs/5211825882
🐛 https://gradle.com/s/vggfbmqvgvntm

Build Failed

Test summary info:

Could not find result summary

…er build

alafanechere · 2023-06-08T14:01:22Z

/test connector=connectors/source-file-secure

🕑 connectors/source-file-secure https://github.com/airbytehq/airbyte/actions/runs/5212111191
✅ connectors/source-file-secure https://github.com/airbytehq/airbyte/actions/runs/5212111191
Python tests coverage:

Name                             Stmts   Miss  Cover
----------------------------------------------------
source_file_secure/__init__.py       2      0   100%
source_file_secure/source.py        33      3    91%
----------------------------------------------------
TOTAL                               35      3    91%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:598: The previous and actual discovered catalogs are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:695: This tests currently leads to too much failures. We need to fix the connectors at scale first.
================= 35 passed, 5 skipped, 39 warnings in 35.11s ==================

…dagger

connectors-ci: make source-file testable in airbyte-ci

78b8379

octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/file labels Jun 7, 2023

make it work

1984f1d

alafanechere changed the base branch from master to 10730-redshiftJDBParameter June 7, 2023 16:26

alafanechere requested review from a team as code owners June 7, 2023 16:26

alafanechere changed the base branch from 10730-redshiftJDBParameter to master June 7, 2023 16:26

octavia-squidington-iii added the connectors/source/file-secure label Jun 7, 2023

revert source-file-secure Dockerfile

a84f280

alafanechere changed the base branch from master to 16658-source-twilio-config June 7, 2023 16:29

alafanechere changed the base branch from 16658-source-twilio-config to master June 7, 2023 16:29

alafanechere commented Jun 7, 2023

View reviewed changes

alafanechere and others added 3 commits June 7, 2023 20:56

bump versions

5ea0d9a

add docstring

3a80a31

Merge branch 'master' into augustin/connectors-ci/fix-source-file-on-…

78b15fe

…dagger

alafanechere requested review from evantahler, bnchrch and a team June 7, 2023 20:23

alafanechere and others added 4 commits June 7, 2023 20:26

Automated Commit - Format and Process Resources Changes

d3f4d80

handle missing secrets

b6c9c47

handle missing secrets

9366142

Revert "handle missing secrets"

20f75e1

This reverts commit b6c9c47.

evantahler approved these changes Jun 8, 2023

View reviewed changes

bnchrch approved these changes Jun 8, 2023

View reviewed changes

source-file-secure based on source-file:0.3.9

afe9318

alafanechere added 2 commits June 8, 2023 15:59

source-file-secure: remove explicit dependency to source file in dock…

6a62ada

…er build

source-file-secure: remove explicit dependency to source file in dock…

3dc35e1

…er build

alafanechere and others added 2 commits June 8, 2023 16:36

source-file-secure FROM source-file:0:3.10

8f2b428

Merge branch 'master' into augustin/connectors-ci/fix-source-file-on-…

f5999aa

…dagger

alafanechere enabled auto-merge (squash) June 8, 2023 14:37

alafanechere merged commit c2b7043 into master Jun 8, 2023

alafanechere deleted the augustin/connectors-ci/fix-source-file-on-dagger branch June 8, 2023 15:04

alafanechere mentioned this pull request Jun 13, 2023

Source File-Secure: source-file-secure should depend on source-file python package instead of source-file docker image #26823

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connectors-ci: make source-file testable in airbyte-ci #27107

connectors-ci: make source-file testable in airbyte-ci #27107

alafanechere commented Jun 7, 2023 •

edited

Loading

github-actions bot commented Jun 7, 2023 •

edited by alafanechere

Loading

alafanechere Jun 7, 2023

alafanechere Jun 7, 2023

alafanechere Jun 7, 2023

bnchrch Jun 8, 2023

alafanechere Jun 7, 2023

github-actions bot commented Jun 7, 2023 •

edited

Loading

alafanechere commented Jun 7, 2023 •

edited by github-actions bot

Loading

alafanechere commented Jun 7, 2023 •

edited by github-actions bot

Loading

evantahler commented Jun 8, 2023 •

edited

Loading

evantahler left a comment

evantahler Jun 8, 2023

evantahler Jun 8, 2023

evantahler Jun 8, 2023

alafanechere Jun 8, 2023

bnchrch left a comment

bnchrch Jun 8, 2023

bnchrch Jun 8, 2023

bnchrch Jun 8, 2023

alafanechere commented Jun 8, 2023

alafanechere commented Jun 8, 2023 •

edited by github-actions bot

Loading

alafanechere commented Jun 8, 2023 •

edited by github-actions bot

Loading

		@@ -1,3 +1,2 @@
		-e ../../bases/connector-acceptance-test
		-e ../source-file


		from setuptools import find_packages, setup


		def local_dependency(name: str) -> str:

connectors-ci: make source-file testable in airbyte-ci #27107

connectors-ci: make source-file testable in airbyte-ci #27107

Conversation

alafanechere commented Jun 7, 2023 • edited Loading

What

Fixing source-file

Fixing source-file-secure

github-actions bot commented Jun 7, 2023 • edited by alafanechere Loading

Before Merging a Connector Pull Request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 7, 2023 • edited Loading

Affected Connector Report

alafanechere commented Jun 7, 2023 • edited by github-actions bot Loading

Build Passed

alafanechere commented Jun 7, 2023 • edited by github-actions bot Loading

Build Failed

evantahler commented Jun 8, 2023 • edited Loading

evantahler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnchrch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere commented Jun 8, 2023

alafanechere commented Jun 8, 2023 • edited by github-actions bot Loading

Build Failed

alafanechere commented Jun 8, 2023 • edited by github-actions bot Loading

Build Passed

alafanechere commented Jun 7, 2023 •

edited

Loading

Fixing `source-file`

Fixing `source-file-secure`

github-actions bot commented Jun 7, 2023 •

edited by alafanechere

Loading

github-actions bot commented Jun 7, 2023 •

edited

Loading

alafanechere commented Jun 7, 2023 •

edited by github-actions bot

Loading

alafanechere commented Jun 7, 2023 •

edited by github-actions bot

Loading

evantahler commented Jun 8, 2023 •

edited

Loading

alafanechere commented Jun 8, 2023 •

edited by github-actions bot

Loading

alafanechere commented Jun 8, 2023 •

edited by github-actions bot

Loading