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

airbyte-ci: make airbyte-ci test able to run any poetry run command #33784

Merged
merged 4 commits into from
Jan 3, 2024
+64 −16
Merged

airbyte-ci: make airbyte-ci test able to run any poetry run command #33784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
87665c1
airbyte-ci: make `airbyte-ci test` able to run any poetry run command
alafanechere Dec 26, 2023
a84b5a3
improve logging and parallelization
alafanechere Dec 27, 2023
004cdcc
Update airbyte-ci/connectors/pipelines/README.md
alafanechere Jan 3, 2024
05f5378
update readme with better example
alafanechere Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/workflows/airbyte-ci-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ jobs:
gcs_credentials: ${{ secrets.METADATA_SERVICE_PROD_GCS_CREDENTIALS }}
sentry_dsn: ${{ secrets.SENTRY_AIRBYTE_CI_DSN }}
github_token: ${{ secrets.GH_PAT_MAINTENANCE_OCTAVIA }}
subcommand: "test airbyte-ci/connectors/connector_ops"
subcommand: "test airbyte-ci/connectors/connector_ops --poetry-run-command='pytest tests'"
airbyte_ci_binary_url: ${{ inputs.airbyte_ci_binary_url || 'https://connectors.airbyte.com/airbyte-ci/releases/ubuntu/latest/airbyte-ci' }}
tailscale_auth_key: ${{ secrets.TAILSCALE_AUTH_KEY }}

Expand All @@ -91,7 +91,7 @@ jobs:
gcs_credentials: ${{ secrets.METADATA_SERVICE_PROD_GCS_CREDENTIALS }}
sentry_dsn: ${{ secrets.SENTRY_AIRBYTE_CI_DSN }}
github_token: ${{ secrets.GH_PAT_MAINTENANCE_OCTAVIA }}
subcommand: "test airbyte-ci/connectors/pipelines"
subcommand: "test airbyte-ci/connectors/pipelines --poetry-run-command='pytest tests'"
airbyte_ci_binary_url: ${{ inputs.airbyte_ci_binary_url || 'https://connectors.airbyte.com/airbyte-ci/releases/ubuntu/latest/airbyte-ci' }}
tailscale_auth_key: ${{ secrets.TAILSCALE_AUTH_KEY }}

Expand All @@ -106,7 +106,7 @@ jobs:
gcs_credentials: ${{ secrets.METADATA_SERVICE_PROD_GCS_CREDENTIALS }}
sentry_dsn: ${{ secrets.SENTRY_AIRBYTE_CI_DSN }}
github_token: ${{ secrets.GH_PAT_MAINTENANCE_OCTAVIA }}
subcommand: "test airbyte-ci/connectors/base_images"
subcommand: "test airbyte-ci/connectors/base_images --poetry-run-command='pytest tests'"
airbyte_ci_binary_url: ${{ inputs.airbyte_ci_binary_url || 'https://connectors.airbyte.com/airbyte-ci/releases/ubuntu/latest/airbyte-ci' }}
tailscale_auth_key: ${{ secrets.TAILSCALE_AUTH_KEY }}

Expand All @@ -115,7 +115,7 @@ jobs:
if: steps.changes.outputs.metadata_lib_any_changed == 'true'
uses: ./.github/actions/run-dagger-pipeline
with:
subcommand: "test airbyte-ci/connectors/metadata_service/lib/"
subcommand: "test airbyte-ci/connectors/metadata_service/lib/ --poetry-run-command='pytest tests'"
context: "pull_request"
github_token: ${{ secrets.GITHUB_TOKEN }}
docker_hub_username: ${{ secrets.DOCKER_HUB_USERNAME }}
Expand All @@ -128,7 +128,7 @@ jobs:
if: steps.changes.outputs.metadata_orchestrator_any_changed == 'true'
uses: ./.github/actions/run-dagger-pipeline
with:
subcommand: "test airbyte-ci/connectors/metadata_service/orchestrator/"
subcommand: "test airbyte-ci/connectors/metadata_service/orchestrator/ --poetry-run-command='pytest tests'"
context: "pull_request"
github_token: ${{ secrets.GITHUB_TOKEN }}
docker_hub_username: ${{ secrets.DOCKER_HUB_USERNAME }}
Expand Down
14 changes: 10 additions & 4 deletions airbyte-ci/connectors/pipelines/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,17 +506,23 @@ This command runs the Python tests for a airbyte-ci poetry package.

| Option | Required | Default | Mapped environment variable | Description |
| ------------------ | -------- | ------- | --------------------------- | ------------------------------------------------------------------------------------------------ |
| `--test-directory` | False | tests | | The path to the directory on which pytest should discover tests, relative to the poetry package. |
| `-c/--poetry-run-command` | True | None | | The command to run with `poetry run` |

#### Example
#### Examples
You can pass multiple `-c/--poetry-run-command` options to run multiple commands.

E.G.: running `pytest` and `mypy`:
`airbyte-ci test airbyte-ci/connectors/pipelines --poetry-run-command='pytest tests' --poetry-run-command='mypy pipelines'`

`airbyte-ci test airbyte-ci/connectors/pipelines --test-directory=tests`
`airbyte-ci tests airbyte-integrations/bases/connector-acceptance-test --test-directory=unit_tests`
E.G.: running `pytest` on a specific test folder:
`airbyte-ci tests airbyte-integrations/bases/connector-acceptance-test --poetry-run-command='pytest tests/unit_tests'`

## Changelog

| Version | PR | Description |
| ------- | ---------------------------------------------------------- | --------------------------------------------------------------------------------------------------------- |
| 2.13.0 | [#33784](https://github.com/airbytehq/airbyte/pull/33784) | Make `airbyte-ci test` able to run any poetry command |
| 2.12.0 | [#33313](https://github.com/airbytehq/airbyte/pull/33313) | Add upgrade CDK command |
| 2.11.0 | [#32188](https://github.com/airbytehq/airbyte/pull/32188) | Add -x option to connector test to allow for skipping steps |
| 2.10.12 | [#33419](https://github.com/airbytehq/airbyte/pull/33419) | Make ClickPipelineContext handle dagger logging. |
| 2.10.11 | [#33497](https://github.com/airbytehq/airbyte/pull/33497) | Consider nested .gitignore rules in format. |
Expand Down
54 changes: 48 additions & 6 deletions airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,48 @@
#
# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
#
from __future__ import annotations

import logging
from pathlib import Path
from typing import TYPE_CHECKING

import asyncclick as click
import asyncer
from pipelines.cli.click_decorators import click_ignore_unused_kwargs, click_merge_args_into_context_obj
from pipelines.consts import DOCKER_VERSION
from pipelines.helpers.utils import sh_dash_c
from pipelines.models.contexts.click_pipeline_context import ClickPipelineContext, pass_pipeline_context

if TYPE_CHECKING:
from typing import List, Tuple

import dagger

## HELPERS
async def run_poetry_command(container: dagger.Container, command: str) -> Tuple[str, str]:
"""Run a poetry command in a container and return the stdout and stderr.
Args:
container (dagger.Container): The container to run the command in.
command (str): The command to run.
Returns:
Tuple[str, str]: The stdout and stderr of the command.
"""
container = container.with_exec(["poetry", "run", *command.split(" ")])
return await container.stdout(), await container.stderr()
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't we be better off returning a dagger.Container here? I'm a bit surprised that we're not interested in the exit code, but I guess that dagger handles failures for us transparently.



@click.command()
@click.argument("poetry_package_path")
@click.option("--test-directory", default="tests", help="The directory containing the tests to run.")
@click.option(
"-c",
"--poetry-run-command",
multiple=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

This means you can pass the option multiple times, not that you can pass multiple arguements to the option, right? An example in the docs of how to pass multiple may be helpful, as well as noting somewhere (new column?) that it can be passed multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right. I added an example to the README.

help="The poetry run command to run.",
required=True,
)
@click_merge_args_into_context_obj
@pass_pipeline_context
@click_ignore_unused_kwargs
Expand All @@ -24,7 +53,10 @@ async def test(pipeline_context: ClickPipelineContext):
pipeline_context (ClickPipelineContext): The context object.
"""
poetry_package_path = pipeline_context.params["poetry_package_path"]
test_directory = pipeline_context.params["test_directory"]
if not Path(f"{poetry_package_path}/pyproject.toml").exists():
raise click.UsageError(f"Could not find pyproject.toml in {poetry_package_path}")

commands_to_run: List[str] = pipeline_context.params["poetry_run_command"]

logger = logging.getLogger(f"{poetry_package_path}.tests")
logger.info(f"Running tests for {poetry_package_path}")
Expand All @@ -47,7 +79,7 @@ async def test(pipeline_context: ClickPipelineContext):

pipeline_name = f"Unit tests for {poetry_package_path}"
dagger_client = await pipeline_context.get_dagger_client(pipeline_name=pipeline_name)
pytest_container = await (
test_container = await (
dagger_client.container()
.from_("python:3.10.12")
.with_env_variable("PIPX_BIN_DIR", "/usr/local/bin")
Expand All @@ -73,10 +105,20 @@ async def test(pipeline_context: ClickPipelineContext):
),
)
.with_workdir(f"/airbyte/{poetry_package_path}")
.with_exec(["poetry", "install"])
.with_exec(["poetry", "install", "--with=dev"])
.with_unix_socket("/var/run/docker.sock", dagger_client.host().unix_socket("/var/run/docker.sock"))
.with_env_variable("CI", str(pipeline_context.params["is_ci"]))
.with_exec(["poetry", "run", "pytest", test_directory])
.with_workdir(f"/airbyte/{poetry_package_path}")
)

await pytest_container
soon_command_executions_results = []
async with asyncer.create_task_group() as poetry_commands_task_group:
for command in commands_to_run:
logger.info(f"Running command: {command}")
soon_command_execution_result = poetry_commands_task_group.soonify(run_poetry_command)(test_container, command)
soon_command_executions_results.append(soon_command_execution_result)

for result in soon_command_executions_results:
stdout, stderr = result.value
logger.info(stdout)
logger.error(stderr)
Comment on lines +114 to +124
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still get the exit on failure behavior that we want? I know in format, when running multiple steps concurrently we had to add explicit error exiting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If an execution fails the async context will throw an ExceptionGroup which will lead to a click command failure and a failed CI pipeline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, great! I guess before it was different because we wanted to continue all the others and collect successes and failures? I suppose that could be helpful here too, but it's not blocking for me, as its all our code that we're checking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right, we could a Dagger exec error handling to better handle success / failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

With multiple commands this will output a jumble of lines which will be very hard to relate to the command itself. Sequentializing the output will require more code. Is the concurrency here really worth it in terms of the added complexity? I'm asking because I don't actually know; however if it's not really worth it then let's execute the poetry commands sequentially. More code => bigger maintenance burden.

2 changes: 1 addition & 1 deletion airbyte-ci/connectors/pipelines/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ build-backend = "poetry.core.masonry.api"

[tool.poetry]
name = "pipelines"
version = "2.12.0"
version = "2.13.0"
description = "Packaged maintained by the connector operations team to perform CI for connectors' pipelines"
authors = ["Airbyte <contact@airbyte.io>"]

Expand Down
Loading