airbyte-ci: make airbyte-ci test able to run any poetry run command
#33784
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
|
Warning Soft code freeze is in effect until 2024-01-02. Please avoid merging to master. #freedom-and-responsibility |
9a6d133 to
ab794c1
Compare
3a4bd1e to
6b1ff5e
Compare
There was a problem hiding this comment.
Functionally this seems fine - but I am wondering, why did we decide to do this under airbyte-ci test instead of introducing e.g. a airbyte-ci lint, similar to formatting, or trying to put format and lint in similar places?
To me, test implies running test, while running e.g mypy involves static checking that could for example have a certain list of files to ignore (as format does) or other linting-specific configuration.
I agree it would be more elegant to place this under a different command. Maybe it's time for a |
6b1ff5e to
b8a63f7
Compare
|
@alafanechere thanks! this actually clears things up for me - we're only linting our code, not the whole codebase. So if we wanted to e.g. lint connector code, we would probably be doing that in connectors test, and if we wanted to do it for the cdk... i'm not sure where we'd do it. But I agree that in the way we're doing it now, it is closer to our testing than our formatting. Yes, a follow up PR sounds fine to me. Will do a proper review! |
| `airbyte-ci test airbyte-ci/connectors/pipelines --test-directory=tests` | ||
| `airbyte-ci tests airbyte-integrations/bases/connector-acceptance-test --test-directory=unit_tests` | ||
| `airbyte-ci test airbyte-ci/connectors/pipelines --poetry-run-command='pytest tests'` | ||
| `airbyte-ci tests airbyte-integrations/bases/connector-acceptance-test--poetry-run-command='pytest tests/unit_tests'` |
There was a problem hiding this comment.
| `airbyte-ci tests airbyte-integrations/bases/connector-acceptance-test--poetry-run-command='pytest tests/unit_tests'` | |
| `airbyte-ci tests airbyte-integrations/bases/connector-acceptance-test--poetry-run-command='pytest unit_tests'` |
| @click.option( | ||
| "-c", | ||
| "--poetry-run-command", | ||
| multiple=True, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes you're right. I added an example to the README.
| 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If an execution fails the async context will throw an ExceptionGroup which will lead to a click command failure and a failed CI pipeline.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yes you're right, we could a Dagger exec error handling to better handle success / failure.
Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>
9ed1ae5 to
05f5378
Compare
| for result in soon_command_executions_results: | ||
| stdout, stderr = result.value | ||
| logger.info(stdout) | ||
| logger.error(stderr) |
There was a problem hiding this comment.
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.
| 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() |
There was a problem hiding this comment.
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.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
…airbytehq#33784) Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>
…airbytehq#33784) Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>
What
Relates to #33719
The current
airbyte-ci testcommand is made to run tests on a poetry project.It's currently only running a
pytestcommand.We have to change this command if we want to run other kind of commands to assess the health of the package (e.g.
mypy) .How
--poetry-run-commandoptions (mulitple) that will be run on the container where the poetry package is installed