Run CATs with local CDK #23084

clnoll · 2023-02-15T16:02:14Z

What

Allow us to run all or a subset of CATs against the local CDK.

How

Modifies airbyte-integrations/connectors/<CONNECTOR>/acceptance-test-docker.sh to run airbyte-integrations/bases/connector-acceptance-test/acceptance-test-docker.sh, which will use the local CDK if the LOCAL_CDK environment variable is set. See the POC for information on how this is being done.
Breaks out the docker image-building component so we can build the connector image with the local CDK without running CATs.
Additionally modifies acceptance-test-docker.sh to fetch secrets if the FETCH_SECRETS environment variable is set. This requires the user to have a Google Service Account and the GCP_GSM_CREDENTIALS environment variable to be set, per the instructions here and here.
Includes a wrapper script that accepts a comma-separated list of connectors (using the -c flag) against which to run the CATs with the local CDK. If no connectors are supplied, defaults to running CATs for all connectors that use airbyte-cdk in their setup.py.

Recommended reading order

airbyte-cdk/python/bin/run-cats-with-local-cdk.sh
airbyte-integrations/bases/connector-acceptance-test/acceptance-test-docker.sh
airbyte-integrations/bases/connectors/build-connector-image-with-local-cdk.sh
airbyte-integrations/bases/connectors/utils.sh
<CONNECTOR>/acceptance-test-docker.sh

Closes 22091 and 22695.

github-actions · 2023-02-15T16:10:36Z

Airbyte Code Coverage

There is no coverage information present for the Files changed

Total Project Coverage	27.16%	🍏

airbyte-integrations/bases/connector-acceptance-test/acceptance-test-docker.sh

airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

airbyte-integrations/bases/connector-acceptance-test/acceptance-test-docker.sh

girarda · 2023-02-15T18:14:25Z

airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

+# Clean up from previous test runs
+rm -rf $OUTPUT_DIR && mkdir $OUTPUT_DIR
+
+while getopts ":c:" opt; do


would be useful to give an example of how to run the script

👍 added help text.

girarda · 2023-02-17T00:31:52Z

airbyte-integrations/connectors/source-activecampaign/acceptance-test-docker.sh

@@ -1,16 +1,8 @@
 #!/usr/bin/env sh

-# Build latest connector image
-docker build . -t $(cat acceptance-test-config.yml | grep "connector_image" | head -n 1 | cut -d: -f2-)
+set -e


would it make sense to move this file to a top level (eg tools) and symlink it instead of copypasting the same file 250 times?

I have a slight hesitation with that - we'll have to update our documentation for creating a connector to tell people "create a symlink to <dir>/acceptance-test-docker.sh, but make sure it's a relative symlink not an absolute one". WDYT about keeping the file but reducing it even further so it just sources acceptance-test-docker.sh, and doesn't contain the readlink logic? An alternative would be to have a script that sets up a connector directory with the boilerplate files & relative symlink pre-populated. But that might be a project for another PR as I'm sure there are some other files/directories we'd want to create as part of the script, but am not totally sure yet what they are.

I don't feel too strongly about symlink vs removing even more cruft from the file, but we already have a script to generate connectors. We should update the template so it matches

Thanks for pointing me to the template code! Turns out that the npm generate package does not copy over symlinks, so instead I've opted to clean things up a bit by having the connectors' acceptance-test-docker.sh just source the base acceptance-test-docker.sh, reducing it to just one line. LMK what you think.

airbyte-integrations/connectors/source-greenhouse/1

airbyte-integrations/connectors/source-greenhouse/_secrets/config.json

airbyte-integrations/connectors/source-greenhouse/_secrets/config_users_only.json

airbyte-integrations/connectors/source-greenhouse/testme.sh

airbyte-integrations/connectors/source-posthog/testme.sh

girarda · 2023-02-17T00:53:27Z

airbyte-integrations/bases/connector-acceptance-test/README.md

+## Running the acceptance tests on multiple connectors:
+If you are contributing to the python CDK, you may want to validate your changes by running acceptance tests against multiple connectors.
+
+To do so, from the root of the `airbyte` repo, run `./airbyte-cdk/python/bin/run-cats-with-local-cdk.sh -c <connector1>,<connector2>,...`


I'm getting this error. Any idea what I'm doing wrong?

➜ airbyte git:(run-cats-with-local-cdk) ✗ ./airbyte-cdk/python/bin/run-cats-with-local-cdk.sh -c source-stripe,source-greenhouse Running CATs for source-stripe source-greenhouse source-greenhouse errors: readlink: illegal option -- f usage: readlink [-n] [file ...] source-stripe errors: readlink: illegal option -- f usage: readlink [-n] [file ...] Done.

edit: looks like readlink -f doesn't exist on macos :(

A potential suggestion might be to use raw readlink in a while loop.

oh weird, that run fine for me

I went ahead and switched this to Alex's suggestion, so we don't have to require a non-builtin readlink.

sherifnada

I like that this is modularized in a way that enables the three use cases (testing a single connector, testing multiple connectors, only building the docker image). Nice work!

Left a few comments. Main request for change is that these scripts should be easily discoverable by the people who would benefit from them, so if we can sprinkle them in docs that would be great e.g:

Add something in CDK README for a CDK dev
Ideally we even add something in the connector README that says ... to run with a local version of the CDK use LOCAL_CDK=1 or something but it may be harder to finagle together the right sed command for that

But generally speaking these are all small comments. Overall it looks great.

airbyte-integrations/bases/connectors/build-connector-image-with-local-cdk.sh

airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

sherifnada · 2023-02-17T05:34:16Z

airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

@@ -0,0 +1,75 @@
+#!/usr/bin/env sh
+
+USAGE="$(basename "$0") [-h] [-c connector1,connector2,...] -- Run connector acceptance tests (CATs) against the local CDK, if relevant.\n


which dir is this script expected to be run from? Should there be an assertion on that?

This script can be run from anywhere in the airbyte repo. Added an assertion.

sherifnada · 2023-02-17T05:37:18Z

airbyte-integrations/bases/connector-acceptance-test/README.md

@@ -35,6 +35,15 @@ _Note: this way will also build docker image for the connector_
 ```
 _Note: this will use the latest docker image for connector-acceptance-test and will also build docker image for the connector_

+You can also use the following environment variables with the Gradle and Bash commands:


these instructions should probably also be in the CDK README? or it should point to these

👍 updated.

airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

sherifnada · 2023-02-17T05:39:22Z

airbyte-integrations/bases/connector-acceptance-test/README.md

+## Running the acceptance tests on multiple connectors:
+If you are contributing to the python CDK, you may want to validate your changes by running acceptance tests against multiple connectors.
+
+To do so, from the root of the `airbyte` repo, run `./airbyte-cdk/python/bin/run-cats-with-local-cdk.sh -c <connector1>,<connector2>,...`


oh weird, that run fine for me

sherifnada · 2023-02-17T05:42:24Z

airbyte-integrations/bases/connectors/build-connector-image-with-local-cdk.sh

+sed -iE 's,"airbyte-cdk[^"]*","airbyte-cdk @ file://localhost/airbyte-cdk",' setup.py
+
+# Build the connector image
+docker_build_quiet "$CONNECTOR_TAG"


I was a little thrown off that this built quietly. I am sort of used to seeing Docker build output in our current scripts otherwise I assumed something was going wrong. Any reason you wanted it quiet instead of normal verbosity?

docker build sends its output to stderr by default, so when this script is called from the run-cats-with-local-cdk.sh wrapper, the build output gets printed out along with any other CAT errors. I think it makes sense to use -q in that situation (lmk if you disagree), but now that the image-building happens in its own script, I agree that the build output should be visible. I've updated this script to only use -q if called from run-cats-with-local-cdk.sh

that's fine, can we at least add a log message that says building docker image $NAME or something? it would help the human at the keyboard know that things are moving along rather than stalled

Yep, makes sense to me - added.

sherifnada · 2023-02-17T05:51:59Z

airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

+    esac
+done
+
+if [ -z "$connectors" ]; then


Suggested change

if [ -z "$connectors" ]; then

if [ -z "$connectors" ]; then

echo "You did not specify any connectors using the -c flag which means this script will run for ALL connectors"

echo "You almost certainly did not mean to do this. Exit now would be my advice. But what do I know? I'm just a script."

echo "Hope you have 128 cores on this machine!"

How about I exit with an error, instead of defaulting to all? It feels like running all should work in theory since the tests are network-bound but Docker doesn't seem to want to handle the load.

this could also just fail if $connectors is not defined

exit with error makes more sense :P

sherifnada · 2023-02-17T05:54:56Z

airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

@@ -0,0 +1,75 @@
+#!/usr/bin/env sh


The CDK readme should contain something about this file, maybe replacing the existing guidance saying how to fiddle with gradle etc.

👍 updated.

sherifnada · 2023-02-17T05:57:01Z

airbyte-integrations/connectors/source-activecampaign/acceptance-test-docker.sh

@@ -1,16 +1,8 @@
 #!/usr/bin/env sh

-# Build latest connector image
-docker build . -t $(cat acceptance-test-config.yml | grep "connector_image" | head -n 1 | cut -d: -f2-)
+set -e


Co-authored-by: Alexandre Girard <alexandre@airbyte.io>

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>

…acceptance-test-docker.sh

github-actions · 2023-02-23T22:01:48Z

Affected Connector Report

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

clnoll · 2023-02-24T13:57:17Z

/test connector=connectors/source-gnews

🕑 connectors/source-gnews https://github.com/airbytehq/airbyte/actions/runs/4263090629
❌ connectors/source-gnews https://github.com/airbytehq/airbyte/actions/runs/4263090629
🐛 https://gradle.com/s/5sdvqq5f2px4i

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - docker.errors.Cont...
FAILED test_core.py::TestConnection::test_check[inputs1] - docker.errors.Cont...
FAILED test_core.py::TestDiscovery::test_discover[inputs0] - docker.errors.Co...
FAILED test_core.py::TestBasicRead::test_airbyte_trace_message_on_failure[inputs0]
ERROR test_core.py::TestSpec::test_config_match_spec[inputs0] - docker.errors...
ERROR test_core.py::TestSpec::test_match_expected[inputs0] - docker.errors.Co...
ERROR test_core.py::TestSpec::test_docker_env[inputs0] - docker.errors.Contai...
ERROR test_core.py::TestSpec::test_enum_usage[inputs0] - docker.errors.Contai...
ERROR test_core.py::TestSpec::test_oneof_usage[inputs0] - docker.errors.Conta...
ERROR test_core.py::TestSpec::test_secret_is_properly_marked[inputs0] - docke...
ERROR test_core.py::TestSpec::test_defined_refs_exist_in_json_spec_file[inputs0]
ERROR test_core.py::TestSpec::test_oauth_flow_parameters[inputs0] - docker.er...
ERROR test_core.py::TestSpec::test_backward_compatibility[inputs0] - docker.e...
ERROR test_core.py::TestSpec::test_additional_properties_is_true[inputs0] - d...
ERROR test_core.py::TestDiscovery::test_defined_cursors_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_defined_refs_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_defined_keyword_exist_in_schema[inputs0-allOf]
ERROR test_core.py::TestDiscovery::test_defined_keyword_exist_in_schema[inputs0-not]
ERROR test_core.py::TestDiscovery::test_primary_keys_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_streams_has_sync_modes[inputs0] - doc...
ERROR test_core.py::TestDiscovery::test_additional_properties_is_true[inputs0]
ERROR test_core.py::TestDiscovery::test_backward_compatibility[inputs0] - doc...
ERROR test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contain...
ERROR test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
ERROR test_incremental.py::TestIncremental::test_two_sequential_reads[inputs0]
ERROR test_incremental.py::TestIncremental::test_read_sequential_slices[inputs0]
ERROR test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs0]
============ 4 failed, 10 passed, 37 warnings, 23 errors in 38.59s =============

clnoll · 2023-02-24T15:16:41Z

/test connector=connectors/source-facebook-pages

🕑 connectors/source-facebook-pages https://github.com/airbytehq/airbyte/actions/runs/4263755202
❌ connectors/source-facebook-pages https://github.com/airbytehq/airbyte/actions/runs/4263755202
🐛 https://gradle.com/s/shg6gkio6uv5s

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
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:98: The previous and actual specifications are identical.
============= 1 failed, 32 passed, 2 skipped in 238.17s (0:03:58) ==============

clnoll · 2023-02-24T17:49:48Z

/test connector=connectors/source-courier

🕑 connectors/source-courier https://github.com/airbytehq/airbyte/actions/runs/4264926521
❌ connectors/source-courier https://github.com/airbytehq/airbyte/actions/runs/4264926521
🐛 https://gradle.com/s/kytuwgixzy5gy

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - AssertionError: At l...
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
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:98: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:507: The previous and actual discovered catalogs are identical.
============ 2 failed, 30 passed, 3 skipped, 35 warnings in 33.57s =============

clnoll · 2023-02-24T19:08:36Z

/test connector=connectors/source-zoom

🕑 connectors/source-zoom https://github.com/airbytehq/airbyte/actions/runs/4265485306
✅ connectors/source-zoom https://github.com/airbytehq/airbyte/actions/runs/4265485306
No Python unittests run

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: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:98: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:507: The previous and actual discovered catalogs are identical.
============ 32 passed, 3 skipped, 35 warnings in 718.33s (0:11:58) ============

Scripts to * Run CATs against the local CDK for one connector * Run CATs against the local CDK for multiple connectors * Create a connecter image with the local CDK --------- Co-authored-by: Alexandre Girard <alexandre@airbyte.io> Co-authored-by: Sherif A. Nada <snadalive@gmail.com>

clnoll requested a review from a team as a code owner February 15, 2023 16:02

clnoll requested a review from a team February 15, 2023 16:02

clnoll marked this pull request as draft February 15, 2023 16:02

octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit labels Feb 15, 2023

clnoll temporarily deployed to more-secrets February 15, 2023 16:04 — with GitHub Actions Inactive

clnoll force-pushed the run-cats-with-local-cdk branch from ad65534 to 69478af Compare February 15, 2023 16:52

clnoll temporarily deployed to more-secrets February 15, 2023 16:54 — with GitHub Actions Inactive

girarda reviewed Feb 15, 2023

View reviewed changes

clnoll temporarily deployed to more-secrets February 15, 2023 19:54 — with GitHub Actions Inactive

clnoll temporarily deployed to more-secrets February 15, 2023 20:34 — with GitHub Actions Inactive

clnoll force-pushed the run-cats-with-local-cdk branch from 8af08bf to bfea1a1 Compare February 15, 2023 20:49

clnoll temporarily deployed to more-secrets February 15, 2023 20:51 — with GitHub Actions Inactive

clnoll force-pushed the run-cats-with-local-cdk branch from bfea1a1 to e9c9511 Compare February 15, 2023 20:57

clnoll temporarily deployed to more-secrets February 15, 2023 20:59 — with GitHub Actions Inactive

clnoll force-pushed the run-cats-with-local-cdk branch from e9c9511 to d9baa02 Compare February 15, 2023 21:01

clnoll temporarily deployed to more-secrets February 15, 2023 21:03 — with GitHub Actions Inactive

clnoll temporarily deployed to more-secrets February 15, 2023 21:04 — with GitHub Actions Inactive

clnoll marked this pull request as ready for review February 16, 2023 16:06

clnoll temporarily deployed to more-secrets February 16, 2023 16:07 — with GitHub Actions Inactive

girarda requested changes Feb 17, 2023

View reviewed changes

sherifnada suggested changes Feb 17, 2023

View reviewed changes

clnoll and others added 11 commits February 23, 2023 22:00

Fetch secrets in acceptance-test-docker.sh if FETCH_SECRETS is set

2db95d5

Update airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

719eb99

Co-authored-by: Alexandre Girard <alexandre@airbyte.io>

Break out separate script to create a connector image with the local CDK

c4ce8ac

Update documentation

ca73cef

Exclude airbyte-cdk/build from being copied to the connector image

719a4c7

rm files that shouldn't have been checked in

7999fba

Reorg & CR updates

149b68d

Update airbyte-cdk/python/bin/run-cats-with-local-cdk.sh

710ef98

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>

Update template & connector acceptance-test-docker.sh to source base …

ba990f8

…acceptance-test-docker.sh

Add instructions for building image with local CDK to python templates

d4532bd

CR updates

b0ae0d6

clnoll force-pushed the run-cats-with-local-cdk branch from f0678c2 to 8243697 Compare February 23, 2023 22:01

Formatting

47e6b2a

clnoll force-pushed the run-cats-with-local-cdk branch from 8243697 to 47e6b2a Compare February 23, 2023 22:41

clnoll merged commit 7da6a3b into master Feb 24, 2023

clnoll deleted the run-cats-with-local-cdk branch February 24, 2023 21:13

clnoll mentioned this pull request Feb 24, 2023

POC: update source-greenhouse with option to run SATs with local CDK #21798

Closed

clnoll restored the run-cats-with-local-cdk branch February 24, 2023 23:49

clnoll deleted the run-cats-with-local-cdk branch February 25, 2023 00:04

clnoll restored the run-cats-with-local-cdk branch February 27, 2023 14:59

clnoll deleted the run-cats-with-local-cdk branch February 27, 2023 15:03

clnoll restored the run-cats-with-local-cdk branch February 27, 2023 16:35

clnoll deleted the run-cats-with-local-cdk branch February 27, 2023 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run CATs with local CDK #23084

Run CATs with local CDK #23084

clnoll commented Feb 15, 2023 •

edited

Loading

github-actions bot commented Feb 15, 2023 •

edited

Loading

girarda Feb 15, 2023

clnoll Feb 15, 2023

girarda Feb 17, 2023

sherifnada Feb 17, 2023

clnoll Feb 20, 2023

girarda Feb 21, 2023

clnoll Feb 21, 2023

girarda Feb 17, 2023

sherifnada Feb 17, 2023

clnoll Feb 20, 2023

sherifnada left a comment

sherifnada Feb 17, 2023

clnoll Feb 20, 2023

sherifnada Feb 17, 2023

clnoll Feb 20, 2023

sherifnada Feb 17, 2023

sherifnada Feb 17, 2023

clnoll Feb 17, 2023

sherifnada Feb 21, 2023

clnoll Feb 22, 2023

sherifnada Feb 17, 2023

clnoll Feb 17, 2023

girarda Feb 18, 2023

sherifnada Feb 21, 2023

sherifnada Feb 17, 2023

clnoll Feb 20, 2023

sherifnada Feb 17, 2023

github-actions bot commented Feb 23, 2023 •

edited

Loading

clnoll commented Feb 24, 2023 •

edited by github-actions bot

Loading

clnoll commented Feb 24, 2023 •

edited by github-actions bot

Loading

clnoll commented Feb 24, 2023 •

edited by github-actions bot

Loading

clnoll commented Feb 24, 2023 •

edited by github-actions bot

Loading

		@@ -0,0 +1,75 @@
		#!/usr/bin/env sh

		USAGE="$(basename "$0") [-h] [-c connector1,connector2,...] -- Run connector acceptance tests (CATs) against the local CDK, if relevant.\n

-if [ -z "$connectors" ]; then
+if [ -z "$connectors" ]; then
+  echo "You did not specify any connectors using the -c flag which means this script will run for ALL connectors"
+  echo "You almost certainly did not mean to do this. Exit now would be my advice. But what do I know? I'm just a script."
+  echo "Hope you have 128 cores on this machine!"

Run CATs with local CDK #23084

Run CATs with local CDK #23084

Conversation

clnoll commented Feb 15, 2023 • edited Loading

What

How

Recommended reading order

github-actions bot commented Feb 15, 2023 • edited Loading

Airbyte Code Coverage

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

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

sherifnada 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2023 • edited Loading

Affected Connector Report

clnoll commented Feb 24, 2023 • edited by github-actions bot Loading

Build Failed

clnoll commented Feb 24, 2023 • edited by github-actions bot Loading

Build Failed

clnoll commented Feb 24, 2023 • edited by github-actions bot Loading

Build Failed

clnoll commented Feb 24, 2023 • edited by github-actions bot Loading

Build Passed

clnoll commented Feb 15, 2023 •

edited

Loading

github-actions bot commented Feb 15, 2023 •

edited

Loading

github-actions bot commented Feb 23, 2023 •

edited

Loading

clnoll commented Feb 24, 2023 •

edited by github-actions bot

Loading

clnoll commented Feb 24, 2023 •

edited by github-actions bot

Loading

clnoll commented Feb 24, 2023 •

edited by github-actions bot

Loading

clnoll commented Feb 24, 2023 •

edited by github-actions bot

Loading