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.

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

live-tests: implement debug mode #35786

Merged
merged 3 commits into from
Mar 6, 2024
Merged

live-tests: implement debug mode #35786

merged 3 commits into from
Mar 6, 2024

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Mar 4, 2024 •
edited
Loading

What

Closes https://github.com/airbytehq/airbyte-internal-issues/issues/2858
Closes https://github.com/airbytehq/airbyte-internal-issues/issues/2853

This PR introduces a usable debug command under the new live-tests CLI.
It also adds strong re-usable primitives that we will re-use in an upcoming regression-test command (which will use pytest).

How

live-tests debug read \
--connector-image=airbyte/source-pokeapi:dev \
--connector-image=airbyte/source-pokeapi:latest \
--config-path=poke_config.json \
--catalog-path=configured_catalog.json

This command will run the selected connector command on the selected connector images and will dump the command output to files for further manual analysis.

An HTTP proxy is introduced to cache requests on the same connectors.

More details in the README.md

@vercel Vercel
Copy link

vercel bot commented Mar 4, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Mar 5, 2024 9:54pm

@alafanechere Graphite App
Copy link
Contributor Author

alafanechere commented Mar 4, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alafanechere and the rest of your teammates on Graphite Graphite

@alafanechere alafanechere mentioned this pull request Mar 4, 2024
Merged
@alafanechere alafanechere force-pushed the augustin/03-01-regression-test_declare_debug_mode branch from cf7cdb7 to 4e307d5 Compare March 4, 2024 14:22
@alafanechere alafanechere marked this pull request as ready for review March 4, 2024 14:28
@alafanechere alafanechere requested review from a team and clnoll March 4, 2024 14:28
@alafanechere alafanechere force-pushed the augustin/03-01-regression-test_declare_debug_mode branch from 4e307d5 to 38154a8 Compare March 4, 2024 15:47
@alafanechere alafanechere force-pushed the augustin/03-01-regression-test_declare_debug_mode branch from 38154a8 to f9f1668 Compare March 4, 2024 16:14
clnoll
clnoll approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@clnoll clnoll left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few minor comments & questions, no blockers. Excited to get the first version out!

airbyte-ci/connectors/live-tests/README.md Show resolved Hide resolved
airbyte-ci/connectors/live-tests/src/live_tests/commons/backends/file_backend.py Outdated

for stream in messages_by_type["records"] | messages_by_type["states"]:
os.makedirs(f"{self._output_directory}/{stream}", exist_ok=True)
def write(self, airbyte_messages: Iterable[AirbyteMessage]):
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is adapted from my first pass, but I'm concerned about holding everything in memory at once.

WDYT about having _get_messages_by_type be a generator that returns a tuple of (AirbyteMessageType, message_json)?

I know it's late for you there so I'll try this out and see how it goes.

airbyte-ci/connectors/live-tests/src/live_tests/debug/__init__.py Outdated
import dagger
import sys

DAGGER_EXEC_TIMEOUT = dagger.Timeout(60 * 60) # One hour
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about making this overridable, maybe via an environment variable? I think in some cases we'd be okay with it taking longer.

airbyte-ci/connectors/live-tests/src/live_tests/commons/connector_runner.py Outdated
Comment on lines 124 to 126
BASE_IN_CONTAINER_OUTPUT_DIRECTORY = "/tmp"
IN_CONTAINER_OUTPUT_PATH = f"{BASE_IN_CONTAINER_OUTPUT_DIRECTORY}/raw_output.txt"
RELATIVE_ERRORS_PATH = "errors.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BASE_IN_CONTAINER_OUTPUT_DIRECTORY = "/tmp"
IN_CONTAINER_OUTPUT_PATH = f"{BASE_IN_CONTAINER_OUTPUT_DIRECTORY}/raw_output.txt"
RELATIVE_ERRORS_PATH = "errors.txt"

No longer used.

airbyte-ci/connectors/live-tests/src/live_tests/commons/connector_runner.py Outdated
def _connector_under_test_container(self) -> dagger.Container:
return self.connector_under_test.container

def get_full_command(self, command: Command):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we make all the methods except run private?

airbyte-ci/connectors/live-tests/src/live_tests/commons/connector_runner.py Outdated
def get_mitmproxy_dir_cache(self) -> dagger.CacheVolume:
return self.dagger_client.cache_volume(self.MITMPROXY_IMAGE)

async def get_proxy_container(
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

airbyte-ci/connectors/live-tests/src/live_tests/commons/models.py
Comment on lines +231 to +232
# TODO merge ExecutionReport.save_to_disk and Backend.write?
# Make backends use customizable
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious to know what you have in mind here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I basically realized that to classes perform final writes to file system of artifact, could be better to have this centralized.

airbyte-ci/connectors/live-tests/src/live_tests/commons/connector_runner.py Show resolved Hide resolved
@octavia-squidington-iv octavia-squidington-iv requested review from a team March 4, 2024 21:19
@katmarkham katmarkham removed the request for review from a team March 5, 2024 15:19
@octavia-squidington-iv octavia-squidington-iv requested a review from a team March 5, 2024 16:42
@alafanechere alafanechere force-pushed the augustin/03-01-regression-test_declare_debug_mode branch from 59efbaa to 5c2669c Compare March 5, 2024 21:10
@alafanechere alafanechere mentioned this pull request Mar 5, 2024
Merged
@alafanechere alafanechere force-pushed the augustin/03-01-regression-test_declare_debug_mode branch from 90587ad to d78a92b Compare March 5, 2024 21:53
@clnoll clnoll merged this pull request into regression-tests-package Mar 6, 2024
32 of 44 checks passed
@clnoll clnoll deleted the augustin/03-01-regression-test_declare_debug_mode branch March 6, 2024 00:24
alafanechere added a commit that referenced this pull request Mar 6, 2024
@alafanechere @clnoll
live-tests: implement debug mode (#35786)
8c0062e 
Co-authored-by: Catherine Noll <clnoll@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clnoll clnoll clnoll 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.
2 participants
@alafanechere @clnoll