[airbyte-ci]: new check on python certified connector to validate their base image use #30527

alafanechere · 2023-09-18T14:46:20Z

What

Closes #30239

We want to make sure a certified python connectors does not pass the test if:

it has a Dockerfile
it is not defining a connectorBuildOptions.baseImage in its metadata.yaml

How

Add a new step to the python connectors test pipeline: CheckBaseImageIsUsed
Run it at the end of the pipeline

alafanechere · 2023-09-18T14:46:36Z

Current dependencies on/for this PR:

master
- PR [airbyte-ci] build python connectors from base images #30456

This comment was auto-generated by Graphite.

bnchrch · 2023-09-18T20:41:43Z

airbyte-ci/connectors/pipelines/pipelines/tests/common.py

@@ -282,3 +282,28 @@ async def _build_connector_acceptance_test(self, connector_under_test_image_tar:
            )

        return cat_container.with_unix_socket("/var/run/docker.sock", self.context.dagger_client.host().unix_socket("/var/run/docker.sock"))
+
+
+class CheckBaseImageIsUsed(Step):


😍 a simple work of art! Loved reading this.

vercel · 2023-09-20T11:11:16Z

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		Oct 20, 2023 9:56am

erohmensing · 2023-09-21T14:26:43Z

airbyte-ci/connectors/pipelines/pipelines/tests/common.py

+        is_certified = self.context.connector.metadata.get("supportLevel")
+        if not is_certified:
+            self.skip("Connector is not certified, it does not require the use of our base image.")


@alafanechere I think I'm missing something here - does this actually give us whether the connector is certified? As far as I can tell this will give us certified, community or None (since it's optional).

If the supportLevel is set to community, then is_certified = 'community' if not is_certified = False and we run the check on community connectors 🤔

ahah nice catch, thank you

erohmensing · 2023-09-21T14:31:16Z

airbyte-ci/connectors/pipelines/pipelines/tests/common.py

+        if not is_using_base_image:
+            return StepResult(
+                self,
+                StepStatus.FAILURE,
+                stdout="Connector is certified but does not use our base image. Please set connectorBuildOptions.baseImage in the connector metadata.",
+            )
+        has_dockerfile = "Dockerfile" in await (await self.context.get_connector_dir(include="Dockerfile")).entries()
+        if has_dockerfile:
+            return StepResult(
+                self,
+                StepStatus.FAILURE,
+                stdout="Connector is certified but is still using a Dockerfile. Please remove the Dockerfile and set connectorBuildOptions.baseImage in the connector metadata.",
+            )


Nit: we'll first tell people to set the baseImage. Then if they run again with both, we tell them to remove the dockerfile and set the base image. They shouldn't be able to reach this second conditional if the base image is already set, so we can remove and set connectorBuildOptions.baseImage in the connector metadata from the stdout

I added a suggestion in the error message to run the migration command. It will make sure that metadata is using the base image and that the dockerfile is removed.

erohmensing · 2023-09-21T14:55:08Z

airbyte-ci/connectors/pipelines/pipelines/tests/common.py

+                StepStatus.FAILURE,
+                stdout="Connector is certified but does not use our base image. Please set connectorBuildOptions.baseImage in the connector metadata.",
+            )
+        has_dockerfile = "Dockerfile" in await (await self.context.get_connector_dir(include="Dockerfile")).entries()


Can we reuse the property from this PR here?

We could but it's. a bit less reliable.
When we call the has_dockerfile property on the Connector object it examines the current filesystem to check for the existence of a dockerfile.
When we check with get_connector_dir we check within a Directory object that is build by the context. The get_connector_dir implementation can change in the future to fetch the connector code from elsewhere (another container or a remote git branch returned by Dagger as a Directory).
This is why we have an explicit check here.
But unfortunately this rule is not consistently used in the rest of the codebase, has accessing the Connector attribute is far more simple.

Would it be possible/worth it to to make the attribute call get_connector_dir as well?

github-actions · 2023-10-14T13:48:28Z

Coverage report for source-postgres

File	Coverage [90.14%]	🍏
InitialSyncCtidIterator.java	90.14%	🍏

Total Project Coverage	71.71%	🍏

airbyte-oss-build-runner · 2023-10-14T13:55:08Z

source-google-sheets test report (commit `d88e2997ea`) - ❌

⏲️ Total pipeline duration: 03mn25s

Step	Result
Build source-google-sheets docker image for platform(s) linux/x86_64	✅
Unit tests	✅
Acceptance tests	✅
Check our base image is used	❌
Code format checks	✅
Validate metadata for source-google-sheets	✅
Connector version semver check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-google-sheets test

airbyte-oss-build-runner · 2023-10-14T13:56:28Z

source-file test report (commit `d88e2997ea`) - ✅

⏲️ Total pipeline duration: 05mn15s

Step	Result
Build source-file docker image for platform(s) linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅
Check our base image is used	✅
Code format checks	✅
Validate metadata for source-file	✅
Connector version semver check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-file test

alafanechere · 2023-10-14T13:58:10Z

☝️ These test results confirm the correct behavior of the new check.

…ir base image use

…ir base image use (airbytehq#30527) Co-authored-by: alafanechere <[email protected]>

This was referenced Sep 18, 2023

[airbyte-ci] new commands for migration to base image #30520

Merged

Make certified python connectors use our base image #30528

Closed

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch 2 times, most recently from beef79d to d7c6437 Compare September 18, 2023 15:36

alafanechere marked this pull request as ready for review September 18, 2023 15:53

alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch from a1b0d77 to 2d3ee2e Compare September 18, 2023 16:02

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from 2c2de0c to 26837a3 Compare September 18, 2023 16:02

alafanechere requested a review from a team September 18, 2023 16:13

bnchrch reviewed Sep 18, 2023

View reviewed changes

bnchrch approved these changes Sep 18, 2023

View reviewed changes

alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch from 2d3ee2e to f6e7ab5 Compare September 19, 2023 15:28

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from 26837a3 to 11a3b4a Compare September 19, 2023 15:28

alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch from f6e7ab5 to 4b23511 Compare September 19, 2023 16:29

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from 11a3b4a to 168f10d Compare September 20, 2023 11:11

alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch from 8258a4e to 175e2d0 Compare September 20, 2023 11:21

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from 168f10d to c013a0a Compare September 20, 2023 11:21

alafanechere changed the base branch from augustin/09-17/airbyte-ci-new-commands-for-migration to augustin/09-18-_airbyte-ci_Implement_pre/post_build_hooks September 20, 2023 11:51

alafanechere mentioned this pull request Sep 20, 2023

A single base for all python connectors #30231

Closed

alafanechere force-pushed the augustin/09-18-_airbyte-ci_Implement_pre/post_build_hooks branch from 92be7ae to 276f1b0 Compare September 21, 2023 11:03

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from 8fe96a6 to de9a417 Compare September 21, 2023 11:03

alafanechere force-pushed the augustin/09-18-_airbyte-ci_Implement_pre/post_build_hooks branch from 276f1b0 to 404346d Compare September 21, 2023 11:05

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from de9a417 to 29ded3d Compare September 21, 2023 11:05

erohmensing reviewed Sep 21, 2023

View reviewed changes

alafanechere force-pushed the augustin/09-18-_airbyte-ci_Implement_pre/post_build_hooks branch 4 times, most recently from a728afd to fcecb62 Compare October 11, 2023 13:32

Base automatically changed from augustin/09-18-_airbyte-ci_Implement_pre/post_build_hooks to master October 11, 2023 13:53

alafanechere mentioned this pull request Oct 13, 2023

Remove dockerfiles for certified python connnectors and use the base image #31377

Closed

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from 29ded3d to b30969f Compare October 14, 2023 13:25

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from fc12ce3 to d88e299 Compare October 14, 2023 13:49

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from d88e299 to be10db7 Compare October 14, 2023 13:57

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch 2 times, most recently from 8c05020 to 817e796 Compare October 16, 2023 16:55

[airbyte-ci]: new check on python certified connector to validate the…

787ec57

…ir base image use

alafanechere force-pushed the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch from 817e796 to 787ec57 Compare October 20, 2023 09:41

Automated Commit - Formatting Changes

9ee91ac

alafanechere merged commit e77959c into master Oct 20, 2023
14 checks passed

alafanechere deleted the augustin/09-18-_airbyte-ci_new_check_on_python_certified_connector_to_validate_their_base_image_use branch October 20, 2023 10:14

ariesgun pushed a commit to ariesgun/airbyte that referenced this pull request Oct 23, 2023

[airbyte-ci]: new check on python certified connector to validate the…

d8c985c

…ir base image use (airbytehq#30527) Co-authored-by: alafanechere <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[airbyte-ci]: new check on python certified connector to validate their base image use #30527

[airbyte-ci]: new check on python certified connector to validate their base image use #30527

alafanechere commented Sep 18, 2023 •

edited

Loading

alafanechere commented Sep 18, 2023 •

edited by bnchrch

Loading

bnchrch Sep 18, 2023

vercel bot commented Sep 20, 2023 •

edited

Loading

erohmensing Sep 21, 2023

alafanechere Sep 21, 2023

erohmensing Sep 21, 2023

alafanechere Oct 14, 2023

erohmensing Sep 21, 2023

alafanechere Sep 21, 2023 •

edited

Loading

erohmensing Sep 21, 2023

github-actions bot commented Oct 14, 2023

airbyte-oss-build-runner commented Oct 14, 2023

airbyte-oss-build-runner commented Oct 14, 2023

alafanechere commented Oct 14, 2023 •

edited

Loading

[airbyte-ci]: new check on python certified connector to validate their base image use #30527

[airbyte-ci]: new check on python certified connector to validate their base image use #30527

Conversation

alafanechere commented Sep 18, 2023 • edited Loading

What

How

alafanechere commented Sep 18, 2023 • edited by bnchrch Loading

bnchrch Sep 18, 2023

Choose a reason for hiding this comment

vercel bot commented Sep 20, 2023 • edited Loading

erohmensing Sep 21, 2023

Choose a reason for hiding this comment

alafanechere Sep 21, 2023

Choose a reason for hiding this comment

erohmensing Sep 21, 2023

Choose a reason for hiding this comment

alafanechere Oct 14, 2023

Choose a reason for hiding this comment

erohmensing Sep 21, 2023

Choose a reason for hiding this comment

alafanechere Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

erohmensing Sep 21, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 14, 2023

Coverage report for source-postgres

airbyte-oss-build-runner commented Oct 14, 2023

source-google-sheets test report (commit d88e2997ea) - ❌

airbyte-oss-build-runner commented Oct 14, 2023

source-file test report (commit d88e2997ea) - ✅

alafanechere commented Oct 14, 2023 • edited Loading

alafanechere commented Sep 18, 2023 •

edited

Loading

alafanechere commented Sep 18, 2023 •

edited by bnchrch

Loading

vercel bot commented Sep 20, 2023 •

edited

Loading

alafanechere Sep 21, 2023 •

edited

Loading

source-google-sheets test report (commit `d88e2997ea`) - ❌

source-file test report (commit `d88e2997ea`) - ✅

alafanechere commented Oct 14, 2023 •

edited

Loading