🐛 migrate-to-manifest-only: conditionally propagate $parameters when resolving manifest #44557
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
ChristoGrab
changed the title
| @@ -198,7 +198,7 @@ async def _run(self) -> StepResult: | |||
| _update_inline_schema(schema_loader, json_streams, stream_name) | |||
|
|
|||
| write_yaml(data, manifest_path) | |||
| await format_prettier([manifest_path], logger=logger) | |||
| # await format_prettier([manifest_path], logger=logger) | |||
There was a problem hiding this comment.
Commented out the formatting as it was causing the command to hang regardless of success or failure.
There was a problem hiding this comment.
That's tolerable if you just do format fix all separately before pushing up PRs. Single responsibility principle!
| from typing import Any, Mapping | ||
| import logging | ||
| from typing import Any, Mapping, Set, Type | ||
| from .declarative_component_schema import * |
There was a problem hiding this comment.
I just copy/pasted the existing models from the CDK into the module so they could be accessible to the tool. It's hacky and not sustainable in anything but the short term since the models in this file are now static. Ideally would have liked to dynamically load the models at runtime, but this is the "it's easy and works short-term" approach. Happy to refactor to a more elegant solution!
| @@ -121,7 +184,9 @@ def propagate_types_and_parameters( | |||
| # Parameters should be applied to the current component fields with the existing field taking precedence over parameters if | |||
| # both exist | |||
| for parameter_key, parameter_value in current_parameters.items(): | |||
| propagated_component[parameter_key] = propagated_component.get(parameter_key) or parameter_value | |||
| if parameter_key in valid_fields: | |||
There was a problem hiding this comment.
By conditionally propagating the parameter only if it is an expected field returned from the model, we avoid adding unexpected fields that break the builder's UI mode.
| @@ -159,12 +159,14 @@ async def _run(self) -> StepResult: | |||
| ## 2. Update the version in manifest.yaml | |||
| try: | |||
| manifest = read_yaml(root_manifest_path) | |||
| manifest["version"] = "4.3.0" | |||
| manifest["version"] = "4.3.2" | |||
| manifest["type"] = "DeclarativeSource" | |||
There was a problem hiding this comment.
Use version 4.3.2 since that's the current version used in builder. Adding the DeclarativeSource type is required to ensure propagate_types_and_parameters can recurse through the component tree
| @@ -198,7 +198,7 @@ async def _run(self) -> StepResult: | |||
| _update_inline_schema(schema_loader, json_streams, stream_name) | |||
|
|
|||
| write_yaml(data, manifest_path) | |||
| await format_prettier([manifest_path], logger=logger) | |||
| # await format_prettier([manifest_path], logger=logger) | |||
There was a problem hiding this comment.
That's tolerable if you just do format fix all separately before pushing up PRs. Single responsibility principle!
...nes/pipelines/airbyte_ci/connectors/migrate_to_manifest_only/declarative_component_schema.py
Outdated
Show resolved
Hide resolved
…s/migrate_to_manifest_only/declarative_component_schema.py
|
@ChristoGrab merge it! I just used it on another migration for a new connector that someone had in a PR and it worked. |
|
@natikgadzhi Woot glad to hear it! I also created a dev account for NYTimes and verified that the migration worked both as a builder project and in Cloud 🎉 |
...s/pipelines/airbyte_ci/connectors/migrate_to_manifest_only/manifest_component_transformer.py
Outdated
Show resolved
Hide resolved
…s/migrate_to_manifest_only/manifest_component_transformer.py
What
Fixes the manifest-only migration pipeline by removing formatting from 'migrate-to-inline_schemas' and applying conditional propagation of $parameters when resolving the manifest.
How
migrate-to-inline_schemas. This step is causing the command to hang regardless of success or failure status. Since it's not a widely used command and is a required step in the migration to manifest-only, this feels like an acceptable quickfix to unblock migrations.DeclarativeSourceto the manifest before passing it to the function for propagating$parameters. Without an assigned type, this step was hitting an early with no propagation applied.$parametersonly when a component expects said field (ie,HttpRequesterexpects apathbut not aprimary_key). This prevents the scenario where, by propagating all parameters across the entire component tree, we end up with a manifest that cannot be parsed for the builder's UI mode.User Impact
None, these changes are meant to fix internal tools for migrating connectors to manifest-only format
Can this PR be safely reverted and rolled back?