fix(low-code cdk): Add literal type for ConfigNormalizationRules components#649
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@brian/require_type_on_config_normalization_rules#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch brian/require_type_on_config_normalization_rulesHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughThe changes enforce that the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Manifest
participant SchemaValidator
participant ConfigNormalizationRulesModel
User->>Manifest: Defines config_normalization_rules with type="ConfigNormalizationRules"
Manifest->>SchemaValidator: Validate manifest against schema
SchemaValidator->>Manifest: Check for required type field
SchemaValidator->>ConfigNormalizationRulesModel: Parse config_normalization_rules with type
ConfigNormalizationRulesModel-->>SchemaValidator: Accept only if type="ConfigNormalizationRules"
SchemaValidator-->>User: Validation result
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
unit_tests/sources/declarative/test_manifest_declarative_source.py (4)
2562-2563: Same rationale as the earlier comment – duplication could be reduced by centralising the literal section.
2633-2634: See earlier note about extracting the repeatedconfig_normalization_rulesstanza.
2709-2710: See earlier note about extracting the repeatedconfig_normalization_rulesstanza.
2774-2775: See earlier note about extracting the repeatedconfig_normalization_rulesstanza.
🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
2160-2164: Consider makingtypedefaulted rather than required, to smooth the rollout — wdyt?Adding
type: Literal["ConfigNormalizationRules"]does enforce the presence of the field at validation time. If you instead declare it as
-type: Literal["ConfigNormalizationRules"] +type: Optional[Literal["ConfigNormalizationRules"]] = "ConfigNormalizationRules"Pydantic will still coerce the constant value and ensure it’s correct, but existing manifests that omit the field will keep working. That can be useful while the “add-type” follow-up PRs roll out.
Also, because this file is auto-generated from the YAML schema, any manual edits can be blown away by the next code-gen run. Can you double-check that the YAML source already contains the same change and that this file is purely regenerated?
unit_tests/sources/declarative/test_manifest_declarative_source.py (1)
2491-2492: Consider extracting the new literal block into a shared helper to keep tests DRY, wdyt?
"type": "ConfigNormalizationRules"now appears verbatim in several test manifests. Moving the fullconfig_normalization_rulesdict (with the invarianttypekey) into a small fixture or helper factory would remove duplication and keep future edits in one place.airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
3838-3844: Literaltyperequirement looks great, but should we add a short description for consistency?Other components generally include a
descriptionon the discriminator field—for example, see#/:definitions/ApiKeyAuthenticator/properties/type.
Adding one here would keep docs parity and help connector authors understand why the field is mandated, wdyt?type: + description: Discriminator – must always be "ConfigNormalizationRules". type: string enum: [ConfigNormalizationRules]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 hunks)unit_tests/sources/declarative/test_manifest_declarative_source.py(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
unit_tests/sources/declarative/test_manifest_declarative_source.py (3)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the `declarative_component_schema.py` file is auto-generated from `declarative_component_schema.yaml` and should be ignored in the recommended reviewing order.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py:1093-1102
Timestamp: 2025-01-14T00:20:32.310Z
Learning: In the `airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py` file, the strict module name checks in `_get_class_from_fully_qualified_class_name` (requiring `module_name` to be "components" and `module_name_full` to be "source_declarative_manifest.components") are intentionally designed to provide early, clear feedback when class declarations won't be found later in execution. These restrictions may be loosened in the future if the requirements for class definition locations change.
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
undefined
<retrieved_learning>
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the declarative_component_schema.py file is auto-generated from declarative_component_schema.yaml and should be ignored in the recommended reviewing order.
</retrieved_learning>
<retrieved_learning>
Learnt from: ChristoGrab
PR: #58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the YamlDeclarativeSource class in airbyte_cdk/sources/declarative/yaml_declarative_source.py, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-shopify
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
3838-3840: Heads-up: this will break manifests missing the new field—have downstream PRs landed?Just double-checking: manifests that currently omit
config_normalization_rules.typewill now fail validation.
You mentioned coordinating follow-up PRs; once those are merged, we’re good. Could you confirm all affected connectors have updates queued, or should we gate this behind a minor version bump first, wdyt?
Patrick Nilan (pnilan)
deleted the
brian/require_type_on_config_normalization_rules
branch
2 participants
While implementing config validations for a connector, I noticed that we weren't requiring a
typeon theConfigNormalizationRulescomponent. For consistency we should do so.The one annoying part is that this would technically constitute a breaking change because
typewill now be required on all instances of the type and no existing connectors contain it. Rather than institute a trivial breaking change, I propose that we prepare PRs for the few uses we have of this component that add the type. Merge this as a minor version, then update the base images for those connectors.This should be relatively low risk since it will fail quite loudly and the changes themselves are quite trivial
Summary by CodeRabbit