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] new commands for migration to base image #30520

Merged

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Sep 18, 2023

Closes #30238

What

This PR adds useful airbyte-ci commands to make some connector changes:

  • bump-version: Will edit the changelog and metadata.yaml of the selected connector
    airbyte-ci connectors --name=<my-connector> bump-version <{patch|minor|major> <PULL_REQUEST_NUMBER> <my changelog entry>
  • upgrade-base-image: Will update the baseImage to the latest version in the metadata.yaml
    airbyte-ci connectors --name=<my-connector> upgrade-base-image
  • migrate-to-base-image: Will perform a minor version bump, delete dockerfile / build.gradle and update the connector documentation to share the new build instructions.
    airbyte-ci connectors --name=<my-connector> migrate-to-base-image <PULL_REQUEST_NUMBER>

Bonus

Delete the format command

@alafanechere alafanechere force-pushed the augustin/09-15-make_QA_checks_tolerate_connector_without_dockerfile branch from 9029852 to e6bcea4 Compare September 18, 2023 14:16
@alafanechere alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch from 9536d3f to 7b65144 Compare September 18, 2023 14:17
@alafanechere alafanechere force-pushed the augustin/09-15-make_QA_checks_tolerate_connector_without_dockerfile branch from e6bcea4 to fb1a22b Compare September 18, 2023 14:46
@alafanechere alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch from 0ff5759 to 1d87e9a Compare September 18, 2023 14:46
@alafanechere alafanechere marked this pull request as ready for review September 18, 2023 15:34
@octavia-squidington-iii octavia-squidington-iii removed the area/connectors Connector related issues label Sep 18, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 18, 2023
@alafanechere alafanechere force-pushed the augustin/09-15-make_QA_checks_tolerate_connector_without_dockerfile branch from fb1a22b to e3a6a77 Compare September 18, 2023 16:02
@alafanechere 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 alafanechere requested a review from a team September 18, 2023 18:07
@alafanechere alafanechere force-pushed the augustin/09-15-make_QA_checks_tolerate_connector_without_dockerfile branch from e3a6a77 to fc5519a Compare September 19, 2023 15:28
@alafanechere 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 alafanechere force-pushed the augustin/09-15-make_QA_checks_tolerate_connector_without_dockerfile branch from fc5519a to 981fc3e Compare September 19, 2023 16:29
@alafanechere 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 alafanechere force-pushed the augustin/declare-first-base-image branch from 1de4960 to 1533e75 Compare September 21, 2023 20:09
@alafanechere alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch from c69888e to 0cf2c46 Compare September 21, 2023 20:09
Base automatically changed from augustin/declare-first-base-image to master September 22, 2023 10:26
@alafanechere alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch 6 times, most recently from 161d658 to f5f8bd5 Compare October 13, 2023 17:39
@alafanechere alafanechere marked this pull request as ready for review October 13, 2023 17:39
@alafanechere
Copy link
Contributor Author

Tests are not running due to our current rate limiting issue.

@@ -509,8 +516,10 @@ def list(


@connectors.command(name="format", cls=DaggerPipelineCommand, help="Autoformat connector code.")
@click.option("--commit-and-push", default=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

❓I dont see this being used?

@click.pass_context
def format_code(ctx: click.Context) -> bool:
def format_code(ctx: click.Context, commit_and_push, export_to_host) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 What if we just remove this instead of update since were relying on gradlew format atm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these new options a remainder of an attempt to consolidate these command changing connector code and making them commit and push. I'll revert this. And will open a separate PR to remove format

"""Bump a connector version: update metadata.yaml, changelog and delete legacy files."""

connectors_contexts = [
ConnectorContext(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a whole connector context?

return context.report


async def run_connector_migration_to_base_image_pipeline(context: ConnectorContext, semaphore, pull_request_number: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

❓Why a pipeline and not a script?

Fine for it to stay as a pipeline but we likely want to add a note in a comment for when we can delete this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is a short lived command, as long as community connector use Dockerfile it has a reason to be considered a pipeline and not adhoc script.

@@ -38,3 +38,5 @@
DOCKER_HOST_NAME = "global-docker-host"
DOCKER_HOST_PORT = 2375
DOCKER_TMP_VOLUME_NAME = "shared-tmp"
REPO = git.Repo(search_parent_directories=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

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

Few comments but nothing blocking

@alafanechere alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch 2 times, most recently from 51a8fa1 to 16354fe Compare October 14, 2023 13:22
@alafanechere alafanechere force-pushed the augustin/09-17/airbyte-ci-new-commands-for-migration branch from 16354fe to e19a55b Compare October 14, 2023 13:46
@alafanechere alafanechere merged commit 99b9fc9 into master Oct 14, 2023
19 of 20 checks passed
@alafanechere alafanechere deleted the augustin/09-17/airbyte-ci-new-commands-for-migration branch October 14, 2023 14:06
ariesgun pushed a commit to ariesgun/airbyte that referenced this pull request Oct 20, 2023
ariesgun pushed a commit to ariesgun/airbyte that referenced this pull request Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document in connector's README how our connector are built
3 participants