-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
feat(pipelines) Add manifest only build #39906
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: airbyte-ci/connectors/connector_ops/connector_ops/utils.py
Did you find this useful? React with a 👍 or 👎 |
4312a30
to
8b9cff3
Compare
32b1f15
to
5057405
Compare
5cbae6b
to
cbb57f7
Compare
ac1f121
to
5360c5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as there's nothing utterly blocking on my end but please:
- Consider sharing as much logic as possible between the "python" connector types. It'll simplify refactor if we have to modify the testing flow or the build flow.
- Can you update all logics in
airbyte-ci
,qa_checks
,connectors_insights
which are based on theConnectorLanguage
? So far I've mainly usedif connector.language in [ConnectorLanguage.PYTHON, ConnectorLanguage.LOW_CODE]
to filter "python" connectors.
@@ -761,6 +761,7 @@ E.G.: running Poe tasks on the modified internal packages of the current branch: | |||
|
|||
| Version | PR | Description | | |||
| ------- | ---------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | | |||
| 4.21.0 | [#TODO](https://github.com/airbytehq/airbyte/pull/TODO) | Add manifest only build pipeline | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 4.21.0 | [#TODO](https://github.com/airbytehq/airbyte/pull/TODO) | Add manifest only build pipeline | | |
| 4.21.0 | [#39906](https://github.com/airbytehq/airbyte/pull/39906) | Add manifest only build pipeline | |
self.logger.info(f"Building connector from base image in metadata for {platform}") | ||
base = self._get_base_container(platform) | ||
|
||
customized_base = await build_customization.pre_install_hooks(self.context.connector, base, self.logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's duplication between _build_from_base_image
in this class vs the python one. Can we keep a common code path for setting entrypoints, labels, calling pre/post hooks etc?
Your could might "just be" considered a pre-install hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alafanechere I was thinking on this.
Im not sure this is the place to DRY up the code completely and have a common build for python and manifest only connectors.
The reasons for that are
- The similarities between the manifest and python
_build_from_base_image
are that they use pre and post commit hooks. After that the python build does a lot more inbetween with two stage build - We are likely going to deviate more from python as we look at how we add support for custom components, potentially in other languages.
I'm almost more inclined to
- Remove the hooks for manifest only
- Add a helper function for adding the docker labels that the two builds can share
from pipelines.helpers.execution.run_steps import STEP_TREE, StepToRun | ||
|
||
|
||
def get_test_steps(context: ConnectorTestContext) -> STEP_TREE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 I'm wondering if we could find a way to have a common tree for low_code/manifest/python connectors and just add unit tests / integration tests for low_code and python connectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
Looking ahead I see
- Low-code being replaced by manifest only
- Manifest only deviating from python (perhaps we allow js custom components? or we don't allow any third party dependencies? or if we do we need to reconcile them
Then I think its best to keep them separate and not share to much logic or test coverage.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but these changes are in the rather distant future, aren't they? I imagine we'll be reworking the step tree long before then. Anyway, it's not blocking for me, so I'll let you do what you think is wise :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theyre not too far away!
Also the way were likely to do "upload for pyairbyte" changes.
So the types of tests we run are even different
https://airbytehq-team.slack.com/archives/C02TL38U5L7/p1719448571131109
Thanks for the push here btw.
customized_base = await build_customization.pre_install_hooks(self.context.connector, base, self.logger) | ||
entrypoint = build_customization.get_entrypoint(self.context.connector) | ||
|
||
manifest_file_name = "manifest.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 This can be stored in the const
module
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/publish/pipeline.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why an update of the poetry lock was required.
5057405
to
a01f5a2
Compare
5360c5e
to
9c15fef
Compare
a01f5a2
to
c68951f
Compare
9c15fef
to
2aef347
Compare
c68951f
to
5082709
Compare
2aef347
to
6665351
Compare
5082709
to
2715f39
Compare
6665351
to
ca3daf0
Compare
@@ -35,9 +43,10 @@ async def _run(self, *args: Any) -> StepResult: | |||
build_results_per_platform = {} | |||
for platform in self.build_platforms: | |||
try: | |||
connector = await self._build_connector(platform, *args) | |||
connector_container = await self._build_connector(platform, *args) | |||
connector_container = apply_airbyte_docker_labels(connector_container, self.context.connector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alafanechere I had an idea to move the application of the docker labels to a generic place for all connectors.
WDYT?
ca3daf0
to
d22ea61
Compare
2715f39
to
4b329f0
Compare
d22ea61
to
247685c
Compare
247685c
to
fb8df7d
Compare
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
1f472fa
to
bfd0d0f
Compare
bfd0d0f
to
1719f64
Compare
TL;DR
The following changes were made to the Airbyte CI to add support for manifest-only connectors.
What changed?
MANIFEST_ONLY
as a new connector language inConnectorLanguage
enum._manifest_only_path
method inconnector_ops/utils.py
to handle paths for manifest-only connectors.manifest_path
property inconnector_ops/utils.py
to resolve manifest paths based on file existence.manifest_only_connectors.py
inbuild_image/steps
to define build steps for manifest-only connectors, and added support for it inpipeline.py
and__init__.py
.manifest_only_connectors.py
intest/steps
to define test steps and updatedpipeline.py
and__init__.py
accordingly.test_manifest_only_connectors.py
intests/test_build_image
for testing the new functionality.connector_ops
andpipelines
packages.How to test?
Run CI pipeline to ensure all tests pass.