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

Airbyte CDK: Test via airbyte CI test poetry packages #36497

Merged
merged 6 commits into from
May 3, 2024
Merged

Airbyte CDK: Test via airbyte CI test poetry packages #36497

merged 6 commits into from
May 3, 2024

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented Mar 26, 2024 •
edited
Loading

Test the python cdk (build, check-ci) via our internal poetry package testing instead of via its own workflow.

Note: this does increase CI runtime for testing the CDK, but improves maintainability as we can treat the cdk as a standard internal python package.

TODO:

  • Remove Python CDK Tests from required status checks (removed)
  • Remove Run Airbyte CI Tests from required status checks (renamed from)
  • Add Internal Poetry Packages CI to required status checks (renamed to)

@vercel Vercel
Copy link

vercel bot commented Mar 26, 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 May 3, 2024 2:55pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Mar 26, 2024
@erohmensing Graphite App
Copy link
Contributor Author

erohmensing commented Mar 26, 2024 •
edited
Loading

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

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

@erohmensing
Configure tool.airbyte_ci to test tasks that we use in CI
03d6c42 
install all extras groups

add to list of internal poetry packages

add to list of internal packages for checking modified

check works
@erohmensing erohmensing changed the base branch from master to ella/airbyte-ci-test-extras March 26, 2024 23:46
@erohmensing erohmensing mentioned this pull request Mar 26, 2024
Merged
@erohmensing erohmensing changed the base branch from ella/airbyte-ci-test-extras to ella/fix-build-scripts March 27, 2024 00:46
@erohmensing erohmensing mentioned this pull request Mar 27, 2024
Merged
@erohmensing erohmensing force-pushed the ella/cdk-test-in-poetry branch 2 times, most recently from 37da8ef to 7ff0920 Compare March 27, 2024 04:02
@erohmensing erohmensing mentioned this pull request Mar 27, 2024
Merged
@erohmensing erohmensing marked this pull request as ready for review March 27, 2024 04:43
@erohmensing erohmensing requested review from a team, alafanechere and girarda March 27, 2024 04:43
@natikgadzhi
Copy link
Contributor

What I would also love to do at this stage or later is to document how to run tests on the CDK, and how to build the CDK in the CDK readme — or perhaps in CONTRIBUTING.md, so we can make this flow transparent for newcomers.

@girarda
Copy link
Contributor

girarda commented Mar 27, 2024

Adding to Natik's comment:
@erohmensing , how do we test these changes? Is there a new airbyte-ci command to build/assemble the CDK and run tests?

Base automatically changed from ella/fix-build-scripts to master March 27, 2024 19:33
alafanechere
alafanechere approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@alafanechere alafanechere left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice!
We should ask CDK developers to update their branch following the merge of this PR.
Please also bump the pipelines version as you modified the list of internal poetry packages in there.

.github/workflows/python_cdk_tests.yml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Python CDK Tests is declared as a required check in the branch protection rules settings to merge to master. We should remove it when merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The workflow goes away total, right? Can we add the airbyte-ci tests as required branch protection rule?

Copy link
Contributor

Choose a reason for hiding this comment

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

@natikgadzhi it'll already be required.
CDK tests will run in the workflow testing all our internal packages, which is a required one.

.github/workflows/airbyte-ci-tests.yml
Comment on lines -51 to +50
name: Run Airbyte CI tests
name: Internal Poetry packages CI
Copy link
Contributor

Choose a reason for hiding this comment

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

If you rename it you should also change the branch protection rules settings (because Run Airbyte CI tests is currently a required check)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do!

airbyte-cdk/python/pyproject.toml
[tool.airbyte_ci]
optional_poetry_groups = ["dev"]
poetry_extras = ["file-based", "sphinx-docs", "vector-db-based"]
poe_tasks = ["build", "check-ci"]
Copy link
Contributor

Choose a reason for hiding this comment

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

check-ci does not run type checks but they do run in python_cdk_tests right? Is it safe to not run type checks in CI for the CDK? ( cc. @girarda )

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we give up on running mypy on modified files? It would feel like a regression to me

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we should run type checks still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type checks currently run and will continue to run as a separate workflow, run-mypy-on-modified-cdk-files.yml

@erohmensing
Copy link
Contributor Author

how do we test these changes? Is there a new airbyte-ci command to build/assemble the CDK and run tests?

You can still use the same poetry run poe scripts to run tests locally if you prefer not to do them in dagger. There is also the airbyte-ci command: airbyte-ci test -p airbyte-cdk/python - this is what is run in CI. it runs build and check-ci.

@erohmensing erohmensing merged commit f23c2e6 into master May 3, 2024
34 checks passed
@erohmensing erohmensing deleted the ella/cdk-test-in-poetry branch May 3, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natikgadzhi natikgadzhi natikgadzhi left review comments

@alafanechere alafanechere alafanechere approved these changes

@girarda girarda Awaiting requested review from girarda

Assignees
No one assigned
Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erohmensing @natikgadzhi @girarda @alafanechere @octavia-squidington-iii