-
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
airbyte-ci: implement migrate-to-poetry connectors command #35583
airbyte-ci: implement migrate-to-poetry connectors command #35583
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @alafanechere and the rest of your teammates on Graphite |
6a3feb8
to
7bd4dc2
Compare
7bd4dc2
to
a4f2992
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.
LGTM!
I have a couple of connectors on my mind who could try and test it out.
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.
looks great! Only request is for surfacing what the migration entails and potentially revisiting the content of connectors' README file
@@ -0,0 +1,91 @@ | |||
# {{ connector.name.title()}} source 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.
should we move this doc to a centralized file since the instructions should be the same for most (all?) API source connectors in the repo?
We could instead have a single markdown file in our docs and just point to it from the connector's readme
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.
@girarda are you thinking about creating a symlink to a static README file which would serve all connectors? Why not!
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 was thinking about a simpler template that just has a link to the actual doc, but a symlink is a good idea
|
||
context: ConnectorContext | ||
|
||
title = "Check if the connector is a candidate for migration to poetry." |
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.
is there a high level description of what it takes for a connector to be a candidate? Can we either point to it from this file or add a comment?
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'll add a comment 👍
def get_package_info(self, package_info: str) -> dict: | ||
package_info_dict = {} | ||
for line in package_info.splitlines(): | ||
if ":" not in line: |
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.
what edge case is this catching? can you add a comment? a log might also be useful in case we do find a package without ":"
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.
It's to ignore locally installed packages - I'll add a comment. All these parsing logics are very clumsy, sorry
extras = extras.split(",") | ||
if name in latest_dependencies_for_hard_pin: | ||
version = f"=={latest_dependencies_for_hard_pin[name]}" | ||
elif "~=" in deps: |
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.
can you add a comment on this block? maybe more generally, it would be helpful to surface what the migration to poetry entails either in-code in in a readme
) | ||
|
||
|
||
class RegressionTest(Step): |
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.
it would be worth documenting what is being tested
@alafanechere let's get this merged and shipped when you have a moment, just for my sanity and clarity of what's in progress. |
a4f2992
to
95e1a77
Compare
@alafanechere, File "pipelines/airbyte_ci/connectors/migrate_to_poetry/pipeline.py", line 454, in run_connector_migration_to_poetry_pipeline
step=AddChangelogEntry(
File "pipelines/airbyte_ci/connectors/bump_version/pipeline.py", line 51, in __init__
self.pull_request_number = int(pull_request_number)
ValueError: invalid literal for int() with base 10: 'TBD' |
What
Closes https://github.com/airbytehq/airbyte-internal-issues/issues/2903
This PR adds a
migrate-to-poetry
command toairbyte-ci connectors
command group.It was used to migrate our certified connectors to poetry
How
The pipeline is composed of the following steps:
pyproject.toml
and apoetry.lock
file according to the dependencies declared insetup.py
. We pin main dependencies to the version used in the previous connector version via apip freeze
setup.py
****N.B.: This code is not in its cleanest form as the migration was mainly an ad-hoc process, we can improve it if this command gets adopted by the community to migrate community connectors to poetry.