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

[low-code cdk] break resolving reference preprocessing into its own class so it can be reused #19517

Merged
merged 8 commits into from
Nov 17, 2022

Conversation

brianjlai
Copy link
Contributor

@brianjlai brianjlai commented Nov 17, 2022

What

Today we identified a bug in the connector builder server where we were not properly dereferencing values that came into the manifest request to the server. That was because Yaml parsing and dereferencing is coupled together in the YamlParser. This change move reference resolution of a manifest into the ManifestDeclarativeSource instead of the YamlDeclarativeSource, because this resolution.

How

Extracts the dereferencing and preprocessing logic out of the YamlParser into it's own class ManifestReferenceResolver. And this preprocessing step is now part of the initialization of a manifest declarative source. I also got rid of the separate config parsers and consolidated the literal string parsing under the YamlDeclarativeSource because yaml parsing became trivial w/o the resolving.

These changes will be uptaken by the connector builder server which should only require a version bump which will be in a follow up review

Recommended reading order

  1. manifest_reference_resolver.py
  2. yaml_declarative_source.py
  3. manifest_declarative_source.py
  4. various tests which remain roughly the same

@brianjlai brianjlai requested a review from a team as a code owner November 17, 2022 00:28
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Nov 17, 2022
@brianjlai brianjlai changed the title break resolving reference preprocessing into its own class so it can be reused [low-code cdk] break resolving reference preprocessing into its own class so it can be reused Nov 17, 2022
@girarda
Copy link
Contributor

girarda commented Nov 17, 2022

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3491314968
https://github.com/airbytehq/airbyte/actions/runs/3491314968

@girarda
Copy link
Contributor

girarda commented Nov 17, 2022

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3491372663
https://github.com/airbytehq/airbyte/actions/runs/3491372663

@girarda girarda merged commit e97ece5 into master Nov 17, 2022
@girarda girarda deleted the brian/separate_yaml_parsing_from_resolving_references branch November 17, 2022 21:15
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
…lass so it can be reused (#19517)

* break resolving reference preprocessing into its own class so it can be reused

* move reference resolution into the ManifestDeclarativeSource and deprecate the parser

* formatting

* last formatting i promise

* rename

* bump version

Co-authored-by: Alexandre Girard <alexandre@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants