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

airbyte-ci: Add pypi publishing logic #34111

Merged
merged 60 commits into from
Jan 24, 2024

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jan 10, 2024

Add pypi publish functionality as a standalone command and as step within connector publish:

Standalone command is in a group poetry:

This is how to publish a poetry project like airbyte lib:

airbyte-ci poetry --package-path=airbyte-lib publish --pypi-token="api-token" --test-pypi

It can also publish legacy projects (this is the functionality used by the connector publish pipeline):

airbyte-ci poetry --package-path=airbyte-integrations/connectors/source-apify-dataset publish --publish-name=airbyte-source-apify-dataset --publish-version="2.1.2" --pypi-token="api-token" --test-pypi

During connector publish, it checks whether the pypi remote registry is enabled - if yes, it checks whether the current version is published already. If no, it will run the same logic as the poetry publish command from above, with the package name set to airbyte-{connector-name} and version set to the docker image tag.

Internally, the package name and version provided to the step will override whatever is defined in the setup.py or pyproject.toml file of the package.

Open questions:

  • How to pass the pypi api token to the connector publish step? Currently, it's just reading from os.environ deep in the code, a cleaner approach would probably be nice, but putting this very specific thing on the top level of airbyte-ci options also feels wrong. Happy for expert opinions here
  • Is the basic setup any good or should it be structured differently?
  • What image should be used to perform the publish? I just went with an image that has poetry pre-installed, but maybe we have some more defined opinions on it. Ideally I would like to not block on this for the initial release of this feature to unblock the functionality first as this is a critical part
  • Integration of passing the pypi api token via CI - I need someone with admin rights to configure this on the repo I think
  • Testing - what are the best strategies to test this? We can of course mock stuff away but it would be nice to stay as close as possible to the "real" thing. One thing that's difficult with this is that we can't publish the same version multiple times, pypi doesn't allow this

Test connector publish pipeline via https://github.com/airbytehq/airbyte/actions/workflows/publish_connectors.yml

Copy link

vercel bot commented Jan 10, 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 Jan 24, 2024 3:12pm

@flash1293 flash1293 changed the title WIP: airbyte-ci: Add poetry publish command airbyte-ci: Add pypi publishing logic Jan 12, 2024
@flash1293 flash1293 marked this pull request as ready for review January 20, 2024 12:29
@flash1293 flash1293 requested a review from a team January 20, 2024 12:29
@octavia-squidington-iii octavia-squidington-iii removed the area/connectors Connector related issues label Jan 20, 2024
@flash1293
Copy link
Contributor Author

@alafanechere

I addressed your comments, it's ready for another look.

During testing I think I noticed a weird error with the caching. To test the connector publish, I added the remoteRegistries section for pokeapi and ran the workflow

The only thing I changed was the airbyte-ci code, because of this I think there is something cached that shouldn't be cached. It's possible this is only an issue during development because it's running airbyte-ci in dev mode, but I wanted to highlight it - could you take a look at this please to make sure it will work correctly in production?

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Nothing blocking but additional feedback following your latest changes.
I'm wondering to what extent we could remove pypi occurences from the code and replace them by python_registry. I'd love to understand which logic is pypi specific. If we have some pypi specific logic I'd appreciate clearly isolating them so that supporting private / other python registry is easy.
I'll check your caching issue now.

@alafanechere
Copy link
Contributor

alafanechere commented Jan 23, 2024

@alafanechere

I addressed your comments, it's ready for another look.

During testing I think I noticed a weird error with the caching. To test the connector publish, I added the remoteRegistries section for pokeapi and ran the workflow

The only thing I changed was the airbyte-ci code, because of this I think there is something cached that shouldn't be cached. It's possible this is only an issue during development because it's running airbyte-ci in dev mode, but I wanted to highlight it - could you take a look at this please to make sure it will work correctly in production?

I'm pretty sure this problems comes from an issue on this switch.
On your first run the airbyte-ci binary built on master was used, explaining why your new logic was not called.
On the second run airbyte-ci is installed from the branch code.
I believe the "install airbyte-ci dev" works only if you modified airbyte-ci code on your head commit, not on the branch.

@alafanechere
Copy link
Contributor

@flash1293 I successfully tested a source-pokeapi pre-release with both Pypi disabled and enabled.

@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Jan 24, 2024
@octavia-squidington-iii octavia-squidington-iii removed the area/connectors Connector related issues label Jan 24, 2024
@flash1293 flash1293 enabled auto-merge (squash) January 24, 2024 15:14
@flash1293 flash1293 merged commit d289534 into master Jan 24, 2024
23 checks passed
@flash1293 flash1293 deleted the flash1293/airbyte-ci-python-publish branch January 24, 2024 15:27
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants