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

internal poetry packages: declare poe tasks and airbyte-ci sections in pyproject.toml #34735

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Feb 1, 2024

What

Relates to #33880
The motivation behind this change is:

  • I don't want the logic about what should run in CI to be maintained at the GHA workflow or airbyte-ci level.
  • I want poetry packages to self declare what should run in CI.

airbyte-ci and GHA workflow remain orchestrators.
If we want to enforce consistency between internal packages I think we should add static code analysis of pyproject.toml files (in a dedicated new package or in airbyte-ci).

I believe this approach would also be highly beneficial for connectors, but first applying this to our internal packages to experience this pattern is a good enough first step.

How

  • Use poe task runner on our internal poetry packages.
  • In pyproject.toml: Declare test, lint etc. tasks to wrap pytest / mypy etc.
  • In pyproject.toml: Declare a new [tool.airbyte-ci] section which airbyte-ci will consume to run the listed poe task in CI.
  • Declare a pydantic Model in airbyte-ci (pipelines) to deserialize and validate the [tool.airbyte-ci] section in pyproject.toml.

🚨 User Impact 🚨

This change has no effect. It's actually used in the downstream PR on this stack

Copy link

vercel bot commented Feb 1, 2024

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 Feb 7, 2024 2:30pm

@alafanechere alafanechere marked this pull request as ready for review February 1, 2024 15:34
@alafanechere alafanechere requested a review from a team February 1, 2024 15:34
@alafanechere alafanechere requested review from girarda, erohmensing and a team February 1, 2024 15:35
@alafanechere alafanechere force-pushed the augustin/02-01-internal_poetry_packages_declare_poe_tasks_and_airbyte-ci_sections_in_pyproject.toml branch from f29b0cd to c3f2ec3 Compare February 2, 2024 10:08
@natikgadzhi
Copy link
Contributor

Once we do this, and connectors use poetry, do we expect them to also declare tasks in poe, or they will have a special case where airbyte-ci has opinions on how to test them? That part does not change, right? We'll run the full battery of tests without connector pyproject.toml defining them?

Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

this is great! :shipit:

from pydantic import BaseModel, Field, validator


class AirbyteCiPackageConfiguration(BaseModel):
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't used as part of this PR, but it is used in this follow up PR by get_airbyte_ci_package_config, which is itself called by the test command to identify the poe_tasks to run and how to configure the container 👍

@alafanechere alafanechere force-pushed the augustin/02-01-internal_poetry_packages_declare_poe_tasks_and_airbyte-ci_sections_in_pyproject.toml branch from c3f2ec3 to 41ced4b Compare February 7, 2024 13:43
@alafanechere
Copy link
Contributor Author

alafanechere commented Feb 7, 2024

Once we do this, and connectors use poetry, do we expect them to also declare tasks in poe, or they will have a special case where airbyte-ci has opinions on how to test them? That part does not change, right? We'll run the full battery of tests without connector pyproject.toml defining them?

@natikgadzhi in an ideal world I'd love airbyte-ci to have 0 opinion on which test should run.
I'd love to:

  • make connectors declare what should run in the CI from the airbyte-ci config section in pyproject.toml
    • unit test
    • integration test
    • type checks
    • metadata validation
    • CAT
    • etc.
  • have a separate tool (called by airbyte-ci) that will check that the airbyte-ci config section in pyproject.toml matches our requirements for a specific set of connectors (e.g. certified)

This will give more flexibility to connector developers in term of CI configuration and also more visibility about what's running in CI. airbyte-ci is currently too much of a blackbox.

I suggest to start the adoption this pattern for connectors with the introduction of type checks / mypy.
More to come in an upcoming tech spec.

@alafanechere alafanechere force-pushed the augustin/02-01-internal_poetry_packages_declare_poe_tasks_and_airbyte-ci_sections_in_pyproject.toml branch from 41ced4b to 2061f6c Compare February 7, 2024 14:30
@alafanechere alafanechere enabled auto-merge (squash) February 7, 2024 14:36
@alafanechere alafanechere merged commit cf4cb22 into master Feb 7, 2024
21 of 22 checks passed
@alafanechere alafanechere deleted the augustin/02-01-internal_poetry_packages_declare_poe_tasks_and_airbyte-ci_sections_in_pyproject.toml branch February 7, 2024 15:08
xiaohansong pushed a commit that referenced this pull request Feb 13, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 21, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
xiaohansong pushed a commit that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants