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.

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: evolutive CI for poetry packages #33880

Closed
alafanechere opened this issue Jan 3, 2024 · 1 comment · Fixed by #34736
Closed

airbyte-ci: evolutive CI for poetry packages #33880

alafanechere opened this issue Jan 3, 2024 · 1 comment · Fixed by #34736

Comments

@alafanechere
Copy link
Contributor

alafanechere commented Jan 3, 2024

We have a couple of internal poetry packages in our code base that we want to run CI for.
Eg:

We currently run pytest suites on these packages with airbyte-ci test <path-to-poetry-package>.
It's fine for now, but if we want to introduce additional CI steps like linting (for mypy checks) we face a challenge to keep a meaningful interface.

Option 1: new command group + subcommands

Create a poetry command group + new subcommands for each new CI step.

  • airbyte-ci poetry <poetry-package-path> lint
  • airbyte-ci poetry <poetry-package-path> test

Pro: Quite straightforward to implement. A consistent way of running CI on poetry packages (same pytest command, same linting command etc.)

Cons: Any package-specific new step will require an airbyte-ci code change.

Option 2: poetry package specific declarative CI [Preferred]

CI steps to run on a package would be defined at the poetry package level.
Running airbyte-ci poetry <poetry-package-path> would read these rules and execute steps accordingly.
Implementation suggestions:

Global pros: CI logic is determined at the package level, and can be customized for each package independently. No code change is required on the airbyte-ci side when package CI has to evolve.
Global cons: We lose the consistency of CI logic across packages. Consistency guarantee could be provided by validation of the package's pyproject.toml or ci.py file.

Option 2.a [Preferred]

A ci.py module stored in the poetry package. This module would declare a hook function in which we would call with_exec on a poetry container where the package is loaded. (a la build_customizations.py for connectors).

Pros: It's very flexible, any with_exec can be run on top of the poetry container.
Cons: Developers would need to understand Dagger APIs to add new CI operations.

A ci.py module in a poetry package:

#
# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
#
from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from dagger import Container

# This is can be a default set at airbyte-ci level
# Which can be overriden at the poetry package level
CODE_PATH = "."

# This is can be a default set at airbyte-ci level
# Which can be overriden at the poetry package level
TEST_COMMANDS = [
    ["pytest", "tests"]
]

LINT_COMMANDS = [
    ["mypy", CODE_PATH]
]

# This can be a default set at airbyte-ci level
# Which can be overriden at the poetry package level
async def test(poetry_container: Container) -> Container:
    for command in TEST_COMMANDS:
        poetry_container = poetry_container.with_exec(command)
    return poetry_container

async def lint(poetry_container: Container) -> Container:
    for command in LINT_COMMANDS:
        poetry_container = poetry_container.with_exec(command)
    return poetry_container

Option 2.b

We add a new [tool.airbyte-ci] section to pyproject.toml.

airbyte-ci parses pyproject.toml and builds the DAG according to the values of tool.airbyte-ci.steps

Pros: a standardized way to declare ci steps without Dagger knowledge required.
Cons: yet another way to declare a CI DAG...

[tool.airbyte-ci.steps.test]
unit = "pytest tests/unit_tests"
integration = "pytest tests/integration_tests"
[tool.airbyte-ci.steps.lint]
mypy = "mypy pipelines --check-untyped-defs"
ruff = "ruff check pipelines --show-files"

Why Augustin prefers Option 2.a above option 2.b

I think it's a more flexible and re-usable option compared to 2.b . 2.b is tied to poetry / toml files.
If we make a global shift to poetry for Python connectors it might be worth adopting 2.b though.

@evantahler
Copy link
Contributor

evantahler commented Jan 3, 2024

Grooming:

  • 2.b is chosen.
    • we are only talking about python tools/packages, so this is OK
    • It's also possible to move to 2.a later if we change our mind.

Notes:

  • We can set this up though normal poetry scripts / commands. We shouldn't need our own exec block. We would need to rename 'airbyte-test' to 'airbyte-foo', and then pass arbitrary command names as an option.

e.g.

Screenshot 2024-01-03 at 10.39.20 AM.png

But... probably under "poe.tasks"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants