feat(connector-builder): add version constraint and wildcard support to manifest migrations #569
Conversation
lazebnyi
changed the title
📝 WalkthroughWalkthroughThe changes enhance the manifest migration system by improving version specifier handling, updating documentation with explicit version examples, refining migration application logic, and adjusting test fixtures and migration registry entries to use more flexible versioning. Imports and exports for migration classes are clarified, and tests now patch the migration registry for precise control. Changes
Sequence Diagram(s)sequenceDiagram
participant ManifestMigrationHandler
participant MigrationRegistry
participant MigrationClass
ManifestMigrationHandler->>MigrationRegistry: Get migrations for manifest version
loop For each migration
ManifestMigrationHandler->>ManifestMigrationHandler: _version_is_valid_for_migration(version, specifier)
alt can_apply_migration
ManifestMigrationHandler->>MigrationClass: Apply migration
alt should_bump_version
ManifestMigrationHandler->>Manifest: Bump manifest version
end
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Would you like to consider adding an example migration entry in the documentation that demonstrates both the wildcard and a PEP 440 range side-by-side for extra clarity, wdyt? ✨ 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
🧹 Nitpick comments (1)
airbyte_cdk/manifest_migrations/migration_handler.py (1)
115-142: Robust implementation of the flexible version constraint system!The rewritten
_version_is_valid_for_migrationmethod elegantly handles the three distinct version constraint scenarios:
- Wildcard (
"*"): Always matches, no version bump needed - perfect for universal migrations- PEP 440 operators: Uses
SpecifierSetfor proper spec validation, no version bump - great for range constraints- Plain versions: Traditional concrete version comparison with version bumping - maintains backward compatibility
The comprehensive docstring with clear rules makes this logic very maintainable. One small suggestion: WDYT about adding some input validation to handle edge cases like empty strings or malformed version specifiers? This could prevent unexpected errors during migration processing.
def _version_is_valid_for_migration( self, manifest_version: str, migration_version: str, ) -> Tuple[bool, bool]: + if not migration_version or not manifest_version: + return False, False + if migration_version == "*": return True, False if migration_version.startswith(("=", "!", ">", "<", "~")): - spec = SpecifierSet(migration_version) - return spec.contains(Version(manifest_version)), False + try: + spec = SpecifierSet(migration_version) + return spec.contains(Version(manifest_version)), False + except Exception: + return False, False - return Version(manifest_version) <= Version(migration_version), True + try: + return Version(manifest_version) <= Version(migration_version), True + except Exception: + return False, False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
airbyte_cdk/manifest_migrations/README.md(1 hunks)airbyte_cdk/manifest_migrations/migration_handler.py(3 hunks)airbyte_cdk/manifest_migrations/migrations/__init__.py(1 hunks)airbyte_cdk/manifest_migrations/migrations/registry.yaml(1 hunks)unit_tests/manifest_migrations/conftest.py(4 hunks)unit_tests/manifest_migrations/test_manifest_migration.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/manifest_migrations/migration_handler.py (2)
unit_tests/manifest_migrations/conftest.py (1)
_process_manifest(1222-1223)airbyte_cdk/manifest_migrations/manifest_migration.py (1)
_process_manifest(94-134)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (17)
airbyte_cdk/manifest_migrations/migrations/registry.yaml (1)
6-6: Nice implementation of the wildcard version support!The change from a fixed version
"6.48.3"to the wildcard"*"effectively demonstrates the new flexible versioning capability. This means these migrations will apply to all manifest versions, which seems appropriate for these foundational HTTP requester migrations. The wildcard approach aligns well with the enhanced version matching logic described in the PR objectives. WDYT about this being the right approach for these particular migrations?airbyte_cdk/manifest_migrations/README.md (1)
23-24: Excellent documentation enhancement!These explicit examples of valid version specifiers are really helpful for users implementing migrations. The examples cover all the major PEP 440 specifier types: wildcards (
"*"), exact matches ("==6.48.3"), compatible releases ("~=1.2"), version ranges (">=1.0.0,<2.0.0"), and simple version strings ("6.48.3"). This makes it much clearer how to use the new flexible versioning system. WDYT about also adding a brief explanation of when to use each type?airbyte_cdk/manifest_migrations/migrations/__init__.py (1)
5-19: Clean package structure for migration exports!The explicit imports and
__all__definition follow Python packaging best practices and make these migration classes properly accessible. The imported classes match the migrations referenced in the registry, which maintains good consistency across the codebase. WDYT about this approach for centralizing access to the migration classes?airbyte_cdk/manifest_migrations/migration_handler.py (2)
9-11: Good addition of necessary imports!The
Tupletype import andSpecifierSetfrompackaging.specifiersare exactly what's needed for the enhanced version handling logic. These imports support the new tuple return type and PEP 440 specifier validation respectively. WDYT about the placement and necessity of these imports?
81-89: Excellent separation of migration application and version bumping logic!The unpacking of the tuple return from
_version_is_valid_for_migrationintocan_apply_migrationandshould_bump_versioncreates a clean separation of concerns. This allows migrations to be applied without necessarily bumping the version (like with wildcards or PEP 440 specifiers), which is exactly what the PR objectives described. The conditional version bumping on line 87-88 is a nice touch. WDYT about this approach for handling different version constraint behaviors?unit_tests/manifest_migrations/conftest.py (6)
9-9: LGTM on the import addition.The ManifestMigration import is correctly added to support the new DummyMigration class defined later in the file.
15-15: Version update aligns with enhanced migration versioning.The change from "0.0.0" to "6.48.3" reflects the new migration versioning scheme that supports flexible version constraints. This creates a more realistic test scenario, wdyt?
498-509: Migration metadata correctly demonstrates version range support.The updated metadata with
from_version: "6.48.3"andto_version: ">=6.48.2,<6.50.0"effectively demonstrates the new version range capabilities mentioned in the PR objectives. This shows how the enhanced system handles compatible release versioning, wdyt?
837-837: Version reversion supports different test scenarios.The change back to "0.0.0" in this fixture creates a different test scenario, which helps ensure the migration system works correctly across various starting versions.
1200-1212: Wildcard versioning demonstrates enhanced flexibility.The use of
"*"forto_versioneffectively demonstrates the wildcard support mentioned in the PR objectives. This shows how the system can now handle more flexible version matching beyond exact versions, wdyt?
1221-1232: Clean test stub implementation.The DummyMigration class provides a minimal but complete implementation of the ManifestMigration interface. The stub methods with minimal logic are perfect for controlled testing scenarios. Nice approach to isolate test behavior, wdyt?
unit_tests/manifest_migrations/test_manifest_migration.py (6)
2-2: Standard copyright year update.The copyright year update to 2025 is appropriate for the current changes.
4-20: Import additions support enhanced testing infrastructure.The new imports (patch, migrations_registry, specific migration classes, and DummyMigration) are all necessary for the updated testing approach that provides precise control over migration behavior. Clean organization of the imports, wdyt?
26-36: Excellent test isolation with patch decorator.The
patch.dictapproach provides precise control over which migrations are applied during testing. Usingclear=Trueensures clean test isolation, and the version range">=6.48.2,<6.50.0"demonstrates the enhanced version constraint support. Great testing practice, wdyt?
46-48: Inline resolver instantiation improves test isolation.The change from using the shared
resolverinstance to inlineManifestReferenceResolver()instantiation provides better test isolation by ensuring each test gets a fresh resolver instance. This prevents potential state sharing issues, wdyt?
73-73: Consistent test isolation pattern.The patch decorator follows the same clean pattern as the first test, mapping "0.0.0" to the DummyMigration for controlled testing. This ensures the test behavior is predictable and isolated, wdyt?
82-82: Consistent resolver instantiation pattern.The inline instantiation maintains consistency with the first test's approach, providing the same test isolation benefits throughout the test suite.
📝 WalkthroughWalkthroughThis update refines the manifest migration framework by enhancing version matching logic, updating migration registry version specifications, improving documentation with explicit version examples, and adjusting related test fixtures and test logic to align with these changes. It also introduces a dummy migration class for testing. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Handler as ManifestMigrationHandler
participant Migration as ManifestMigration
Test->>Handler: _handle_migration(manifest, migration, migration_version)
Handler->>Handler: _version_is_valid_for_migration(manifest_version, migration_version)
Handler-->>Handler: (can_apply_migration, should_bump_version)
alt can_apply_migration == True
Handler->>Migration: should_migrate(manifest)
alt should_migrate == True
Handler->>Migration: migrate(manifest)
alt should_bump_version == True
Handler->>manifest: Update manifest version
end
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code Graph Analysis (2)airbyte_cdk/manifest_migrations/migrations/__init__.py (3)
airbyte_cdk/manifest_migrations/migration_handler.py (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (17)
✨ 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 (
|
|
@lazebnyi think you can address Ben's comment and then we can get this merged by EOD Monday? |
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
airbyte_cdk/manifest_migrations/migration_handler.py (1)
30-30: Consider making the wildcard pattern more specific, wdyt?I noticed there's a past review comment from bnchrch suggesting to change this to
*.*.*and make it a const for future regex matches. The current pattern".*"matches any string, but for version wildcards, shouldn't we be more specific about the wildcard character itself?Maybe something like:
-WILDCARD_VERSION_PATTERN = ".*" +WILDCARD_VERSION_PATTERN = r"^\*(\.\*)*$"This would match
*,*.*,*.*.*, etc., which seems more aligned with version wildcard conventions. What do you think?
🧹 Nitpick comments (1)
airbyte_cdk/manifest_migrations/migration_handler.py (1)
120-143: Excellent implementation of the three-rule version matching system!The logic is clear and well-documented. I particularly like how you've structured it:
- Wildcard check first (most permissive)
- PEP 440 operators (structured constraints)
- Fallback to concrete version comparison
The return tuple
(can_apply_migration, should_bump_version)makes the intent very clear. One small question though - should we add any input validation to ensuremigration_versionandmanifest_versionare valid strings before processing them, wdyt?Consider adding basic validation:
+ if not migration_version or not manifest_version: + return False, False + if re.match(WILDCARD_VERSION_PATTERN, migration_version):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/manifest_migrations/migration_handler.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/manifest_migrations/migration_handler.py (2)
unit_tests/manifest_migrations/conftest.py (1)
_process_manifest(1222-1223)airbyte_cdk/manifest_migrations/manifest_migration.py (1)
_process_manifest(94-134)
🪛 Pylint (3.3.7)
airbyte_cdk/manifest_migrations/migration_handler.py
[warning] 86-86: Access to a protected member _process_manifest of a client class
(W0212)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (3)
airbyte_cdk/manifest_migrations/migration_handler.py (3)
8-8: LGTM! New imports look well-structured.The added imports (
re,Tuple,SpecifierSet) are properly aligned with the new functionality for version constraint handling. Good job organizing these logically!Also applies to: 10-10, 12-12
82-89: Nice clean separation of concerns with the tuple unpacking!The logic for separating migration application from version bumping is well-designed. This gives you fine-grained control over when to apply migrations vs when to update the version, which is exactly what the wildcard support needed.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 86-86: Access to a protected member _process_manifest of a client class
(W0212)
86-86: Static analysis hint about protected member access can be safely ignored.The pylint warning about accessing
_process_manifestis a false positive here - this is internal CDK code where the migration handler legitimately needs to call the protected method of the migration instance. This is the intended design pattern for the migration system.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 86-86: Access to a protected member _process_manifest of a client class
(W0212)
What
Currently, the migration only accepts catalogs with versions that are not higher than the one specified in the
registry.yamlfile. This causes issues when some catalogs cannot be migrated due to stricter version constraints. We need to enhance the version handling to support a broader range of version definitions such as*,==6.48.3,~=1.2,>=1.0.0,<2.0.0, and6.48.3.Original slack thread - https://airbytehq-team.slack.com/archives/C08GNBT7AV7/p1747700015297039
How
Extended the
_version_is_valid_for_migrationmethod to support various version definition formats. Updated_handle_migrationto adjust the version if necessary. Corresponding tests have been updated accordingly.Summary by CodeRabbit