Add pypi section to connector metadata #33529

flash1293 · 2023-12-15T13:49:42Z

To control the publishing of python connectors to pypi, this PR introduces a new flag to opt into this publishing.

The actual publishing logic as part of airbyte-ci will be implemented in a separate PR.

vercel · 2023-12-15T13:49:48Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 19, 2023 11:02am

alafanechere

Can we this nest down under a something like remotePackageIndex, which have field for pypi / maven / npm whatever

…pypi

flash1293 · 2023-12-18T10:17:26Z

Thanks for the review @alafanechere , could you take another look?

alafanechere

I'd appreciate an additional review from @bnchrch .
Can we also add a validator to make sure that Pypi is only used for python connector? (A metadata validation for a java connector would fail if your new fields are declared in there)

alafanechere · 2023-12-18T11:07:51Z

...tors/metadata_service/lib/metadata_service/models/generated/ConnectorMetadataDefinitionV0.py

+        extra = Extra.forbid
+
+    enabled: bool
+


What do you think about adding the url to the pypi package?

Seems in line with the "documentation url" and so on, added!

Since our goal here is to publish, and tooling generally expects package name, could we call this the PyPi package_name? I think that would be my vote. (E.g. airbyte-source-apify instead of https://pypi.org/project/airbyte-source-apify/)

PyPi URL just isn't very helpful because (to my knowlege at least), the full URL can't be passed to pip install or to the publish operation.

Fwiw, I do see "pip url" used frequently, but that is generally an alias for a PyPi package name or a Git ref, if the package isn't on PyPi.

alafanechere · 2023-12-18T11:10:42Z

...tors/metadata_service/lib/metadata_service/models/generated/ConnectorMetadataDefinitionV0.py

+
+    pypi: Optional[Pypi] = None
+
+


Shall we go "polymorphic" here?

Suggested change

pypi: Optional[Pypi] = None

index_name: # Would be an enum like Pypi | Maven etc

url: str # Would be the url to the package

As we will need different implementations for each of the package indices and maybe different options a while down the road, an explicit separate object seems more future proof, wdyt?

True, what do you think about using RemoteRegistries instead of RemotePackageIndexes:

remoteRegistries: pypi: url: <url to pypi package> DockerHub: url: <url to dockerHub images> imageAdress: dockerio. maven: ...

Dockerhub, maven etc. can come later of course.

No strong opinion on the wording, changing to remoteRegistries

flash1293 · 2023-12-18T12:29:40Z

Can we also add a validator to make sure that Pypi is only used for python connector? (A metadata validation for a java connector would fail if your new fields are declared in there)

Good point, added that. It's checking for the language tag

aaronsteers · 2023-12-19T07:35:50Z

...tors/metadata_service/lib/metadata_service/models/generated/ConnectorMetadataDefinitionV0.py

+        extra = Extra.forbid
+
+    enabled: bool
+


Since our goal here is to publish, and tooling generally expects package name, could we call this the PyPi package_name? I think that would be my vote. (E.g. airbyte-source-apify instead of https://pypi.org/project/airbyte-source-apify/)

PyPi URL just isn't very helpful because (to my knowlege at least), the full URL can't be passed to pip install or to the publish operation.

Fwiw, I do see "pip url" used frequently, but that is generally an alias for a PyPi package name or a Git ref, if the package isn't on PyPi.

aaronsteers · 2023-12-19T07:37:19Z

...yte-ci/connectors/metadata_service/lib/metadata_service/models/generated/RemoteRegistries.py

+from pydantic import BaseModel, Extra, Field
+
+
+class Pypi(BaseModel):


Suggested change

class Pypi(BaseModel):

class PyPi(BaseModel):

aaronsteers · 2023-12-19T07:37:30Z

...yte-ci/connectors/metadata_service/lib/metadata_service/models/generated/RemoteRegistries.py

+    class Config:
+        extra = Extra.forbid
+
+    pypi: Optional[Pypi] = None


Suggested change

pypi: Optional[Pypi] = None

pypi: Optional[PyPi] = None

flash1293 · 2023-12-19T11:01:11Z

Makes sense to me @aaronsteers - adjusted.

alafanechere · 2023-12-19T11:06:36Z

...tors/metadata_service/lib/metadata_service/models/generated/ConnectorMetadataDefinitionV0.py

+    enabled: bool
+    packageName: str = Field(..., description="The name of the package on PyPi.")


For a followup PR once the publish to PyPi step is implemented:

We should add a validator that makes sure that if enabled is True the current connector version is available on PyPi.
It's basically what we do for docker images: before uploading the metadata file to GCS we validate that the docker image is available on DockerHub.

you mean as a post-upload validator? If yes that makes a ton of sense to me, thanks for mentioning

Yes when we run metadata_service validate <config file path>

In this module: https://github.com/airbytehq/airbyte/blob/master/airbyte-ci/connectors/metadata_service/lib/metadata_service/validators/metadata_validator.py#L1

alafanechere

Approving to not be blocking - 👍 Feel free to add validation that PyPi is only set for python connectors in a follow up PR or this one.

bnchrch

Just got caught up on all this.

Great and practical discussion.

Ive got nothing more to add besides LGTM!

bnchrch · 2023-12-19T18:44:32Z

docs/connector-development/connector-metadata-file.md

@@ -151,3 +151,19 @@ The supported scope types are listed below.
 | Scope Type | Value Type | Value Description |
 |------------|------------|------------------|
 | stream     | `list[str]`  | List of stream names |
+
+#### `remoteRegistries`


bnchrch · 2023-12-19T18:45:30Z

airbyte-ci/connectors/metadata_service/lib/metadata_service/validators/metadata_validator.py

@@ -171,12 +171,29 @@ def validate_metadata_base_images_in_dockerhub(
    return True, None


+def validate_pypi_only_for_python(


Joe Reuter added 2 commits December 15, 2023 14:37

add pypi metadata section

4c78aa3

add pypi metadata section

9573dde

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Dec 15, 2023

vercel bot deployed to Preview December 15, 2023 13:49 View deployment

flash1293 marked this pull request as ready for review December 15, 2023 16:41

flash1293 requested a review from a team December 15, 2023 16:41

alafanechere requested changes Dec 15, 2023

View reviewed changes

Joe Reuter added 2 commits December 18, 2023 11:05

Merge remote-tracking branch 'origin/master' into flash1293/metadata-…

475f1e3

…pypi

review comments

109e6a4

Joe Reuter added 2 commits December 18, 2023 11:31

update docs

2cd6ffe

format

e2696b8

alafanechere reviewed Dec 18, 2023

View reviewed changes

review comments

44b5f3d

vercel bot deployed to Preview December 18, 2023 12:31 View deployment

review comments

848ece9

vercel bot deployed to Preview December 18, 2023 14:13 View deployment

Merge branch 'master' into flash1293/metadata-pypi

639c61f

vercel bot deployed to Preview December 18, 2023 14:27 View deployment

fix validator

aff6094

aaronsteers reviewed Dec 19, 2023

View reviewed changes

review comment

f1b0754

vercel bot deployed to Preview December 19, 2023 11:02 View deployment

alafanechere reviewed Dec 19, 2023

View reviewed changes

alafanechere approved these changes Dec 19, 2023

View reviewed changes

bnchrch reviewed Dec 19, 2023

View reviewed changes

bnchrch approved these changes Dec 19, 2023

View reviewed changes

flash1293 merged commit e3fa594 into master Dec 20, 2023
28 checks passed

flash1293 deleted the flash1293/metadata-pypi branch December 20, 2023 09:53

aaronsteers added a commit that referenced this pull request Jan 8, 2024

declare PyPi publish in style of #33529

06c7d38

aaronsteers mentioned this pull request Jan 8, 2024

Source Faker: Add support for PyPi and AirbyteLib entrypoints #34033

Merged

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

Add pypi section to connector metadata (airbytehq#33529)

0dd86e9

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

Add pypi section to connector metadata (airbytehq#33529)

ccd6261

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pypi section to connector metadata #33529

Add pypi section to connector metadata #33529

flash1293 commented Dec 15, 2023

vercel bot commented Dec 15, 2023 •

edited

Loading

alafanechere left a comment

flash1293 commented Dec 18, 2023

alafanechere left a comment

alafanechere Dec 18, 2023

flash1293 Dec 18, 2023

aaronsteers Dec 19, 2023

alafanechere Dec 18, 2023

flash1293 Dec 18, 2023

alafanechere Dec 18, 2023

flash1293 Dec 18, 2023

flash1293 commented Dec 18, 2023

aaronsteers Dec 19, 2023

aaronsteers Dec 19, 2023

aaronsteers Dec 19, 2023

flash1293 commented Dec 19, 2023

alafanechere Dec 19, 2023

flash1293 Dec 19, 2023

alafanechere Dec 19, 2023

alafanechere Dec 19, 2023

alafanechere left a comment •

edited

Loading

bnchrch left a comment

bnchrch Dec 19, 2023

bnchrch Dec 19, 2023

-    pypi: Optional[Pypi] = None
+    index_name:  # Would be an enum like Pypi | Maven etc
+    url: str # Would be the url to the package

		from pydantic import BaseModel, Extra, Field


		class Pypi(BaseModel):

		enabled: bool
		packageName: str = Field(..., description="The name of the package on PyPi.")

		@@ -171,12 +171,29 @@ def validate_metadata_base_images_in_dockerhub(
		return True, None


		def validate_pypi_only_for_python(

Add pypi section to connector metadata #33529

Add pypi section to connector metadata #33529

Conversation

flash1293 commented Dec 15, 2023

vercel bot commented Dec 15, 2023 • edited Loading

alafanechere left a comment

Choose a reason for hiding this comment

flash1293 commented Dec 18, 2023

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere left a comment • edited Loading

Choose a reason for hiding this comment

bnchrch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Dec 15, 2023 •

edited

Loading

alafanechere left a comment •

edited

Loading