Skip to content

fix: (PoC) (do not merge) (CDK) (Manifest) - Migrate manifest fields #463

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

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

bazarnov
Copy link
Contributor

@bazarnov bazarnov commented Apr 8, 2025

What

Resolves:

How

Manifest Migrations:

  • Introduced a framework to handle manifest migrations in the Airbyte CDK to apply transformations on given manifest.

  • Added migration logic to convert url_base and path to url.

    • http_requester_url_base_to_url_v6_45_2__0.py
    • http_requester_path_to_url_v6_45_2__1.py
  • Created migration files with clear versioning and order handling.

  • Registered migration classes dynamically and applied them in order.

Updated Component Models:

  • Modified the HttpRequester to support the new url field and prioritize it.
  • Updated schema definitions, validation logic, and deprecated fields in models.
  • Added logic to handle warnings during component creation (e.g., get_model_deprecations).
  • Ensure the backward compatibility with the deprecated url_base and path fields.

Deprecation of url_base and path fields:

  • Introduced a new HttpRequester.url field to replace the deprecated url_base and path fields.
  • Marked url_base and path as deprecated with deprecation messages.
  • Updated descriptions to guide users towards using url.

Handling Deprecation Warnings:

  • Added a new deprecation_warnings method to the DeclarativeSource and ManifestDeclarativeSource class to collect and process deprecation notices.
  • Updated the _categorise_groups() to handle deprecation warnings alongside message groups.
  • Added logic to collect these warnings in components using BaseModelWithDeprecations.
  • The new airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py module is added to cover the deprecation warnings for the fields marked as deprecated. This class is static (not autogenerated) and re-used when the declarative_component_schema.py is being generated. In this way, there is no need to set additional validations on top of the existing component implementations.

Unit Tests and Documentation:

  • Added unit tests for manifest migrations.
    • unit_tests/sources/declarative/migrations/test_manifest_migration.py
  • Created the README.md in the manifest/migrations directory to document the migration framework.
    • added documentation and examples make it easier to adopt the new changes.

User Impact

  • No impact is expected, this is not a breaking change.
  • The migration execution is hidden under the migrate_manifest: bool = False by default, to not to have any regressions, before we're ready to use it within the UI (/resolve should be having migrate: bool flag to set the migration to True)

Related PR parts:

  1. Add the Manifest Normalizer - fix: (CDK) (Manifest) - Add Manifest Normalization module (reduce commonalities + handle schema $refs) #447
  2. Current PR

Deprecation Warning examples

The deprecation rules applied to the declarative_component_schema.yaml are automatically picked up and propagated to the declarative_component_schema.py (auto-gen models), see the example bellow.

# declarative component schema content

HttpRequester:
  required:
    - url
    # - url_base # removed from the required fields
  # .. the rest of the component content
  properties:
    url_base:
        deprecated: true
        deprecation_message: "Use `url` field instead."
        # .. the rest of the deprecated field content
    url:
       # .. the content of the deprecated field replacement
class HttpRequester(BaseModelWithDeprecations):
    type: Literal["HttpRequester"]
    url_base: Optional[str] = Field(
        None,
        deprecated=True, # deprecation flag is set onto the field
        deprecation_message="Use `url` field instead.", # deprecation message is set onto the field, will be used as a part of the warning.
        description="....",
        examples=[],
        title="API Base URL",
    )

Warning example

When using the deprecated field or creating an instance of the class with the deprecated field.

Python default behavior:

DeprecationWarning: Component type: `HttpRequester`. Field 'url_base' is deprecated. Use `url` field instead.

If multiple fields are marked as deprecated and used - more than 1 deprecation message is shown.

DeprecationWarning: Component type: `HttpRequester`. Field 'url_base' is deprecated. Use `url` field instead.

DeprecationWarning: Component type: `HttpRequester`. Field 'path' is deprecated. Use `url` field instead.

Connector Builder behavior:

Screenshot 2025-04-14 at 21 10 37

@github-actions github-actions bot added bug Something isn't working security labels Apr 9, 2025
@dbgold17
Copy link
Contributor

somewhat minor comment: should this code live outside of the sources/declarative folder since it is not itself part of the declarative schema, but rather a sort of "meta tool" for working with it?

@dbgold17
Copy link
Contributor

another high-level thing. Before you open this PR for review, I wonder if it makes sense to break down the changes into several related PRs since there is a lot happening here. What about something like this?

PR1: introduce the migration framework
PR2: introduce the framework to mark deprecations
PR3: use these to actually make the breaking change around the url param

@bazarnov bazarnov changed the title fix: (CDK) (Manifest) - Migrate manifest fields fix: (PoC) (do not merge) (CDK) (Manifest) - Migrate manifest fields Apr 16, 2025
)


class V_6_45_2_ManifestMigration_HttpRequesterUrlBaseToUrl(ManifestMigration):
Copy link
Contributor

Choose a reason for hiding this comment

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

🙇

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.

3 participants