-
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: Add upgrade cdk command #33313
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/commands.py
Outdated
Show resolved
Hide resolved
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/commands.py
Outdated
Show resolved
Hide resolved
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/pipeline.py
Outdated
Show resolved
Hide resolved
new_version: str, | ||
): | ||
super().__init__(context) | ||
self.repo_dir = repo_dir |
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.
repo_dir
is already an attribute of context
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/pipeline.py
Outdated
Show resolved
Hide resolved
set_cdk_version_result = await set_cdk_version.run() | ||
steps_results.append(set_cdk_version_result) | ||
final_repo_dir = set_cdk_version_result.output_artifact | ||
await og_repo_dir.diff(final_repo_dir).export(str(git.get_git_repo_path())) |
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.
Shall we move the export logic back to the SetCDKVersion so that a failed export fails the step (and the report)?
final_repo_dir = set_cdk_version_result.output_artifact | ||
await og_repo_dir.diff(final_repo_dir).export(str(git.get_git_repo_path())) |
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 we should export the connector directory and not the full repo directory. This will improve performances.
def connector_with_poetry(): | ||
return Connector("destination-duckdb") |
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'd be more stable to not rely on a single connector. I suggest to iterate on the all_connectors
fixture and return a connector which has a pyproject.toml
file and skip otherwise.
def connector_with_poetry(): | |
return Connector("destination-duckdb") | |
def connector_with_poetry(all_connectors): | |
for connector in all_connectors: | |
if (connector.code_directory / "pyproject.toml").exists(): | |
return connector | |
pytest.skip("No connector using poetry found") |
Thanks for your comments @alafanechere - I adjusted the code and wrote some tests, could you take another look? |
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've a couple of additional minor suggestions, nothing blocking.
Could you please bump the airbyte-ci version in README.md
and pyproject.toml
🙏
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/pipeline.py
Outdated
Show resolved
Hide resolved
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/pipeline.py
Outdated
Show resolved
Hide resolved
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/pipeline.py
Outdated
Show resolved
Hide resolved
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/pipeline.py
Show resolved
Hide resolved
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/upgrade_cdk/commands.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.
Nothing blocking and minor remaining suggestions 👍 👏 👏 👏
Please increment the version by a minor bump before merging
async def _run(self) -> StepResult: | ||
context: ConnectorContext = self.context | ||
og_connector_dir = await context.get_connector_dir() | ||
if not "setup.py" in await og_connector_dir.entries(): |
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 we return self.skip(reason="Connector is not a Python connector")
when the connector is a Java connector?
This PR adds a new command to airbyte-ci that allows updating the CDK version in connectors. It's mostly meant to be called in an automated fashion from Github Action workflows to auto-upgrade connectors after an upgrade.
Prod version of #32721