fix(manifest-connectors): resolve parsing errors during version comparisons #38
Conversation
📝 Walkthrough## Walkthrough
The pull request introduces updates to the GitHub Actions workflow configuration for testing connectors in the `.github/workflows/connector-tests.yml` file. It specifically adds the `source-chargebee` connector to the testing matrix of the `connectors_ci` job, which runs on `ubuntu-latest` with a timeout of 360 minutes. The structure of the `cdk_changes` job remains unchanged, but it continues to conditionally trigger the `connectors_ci` job based on modifications detected in relevant directories and files.
## Changes
| File Path | Change Summary |
|----------------------------------------------|-------------------------------------------------------------|
| `.github/workflows/connector-tests.yml` | Added `source-chargebee` connector to the `connectors_ci` job matrix. |
| `airbyte_cdk/sources/declarative/manifest_declarative_source.py` | Removed `re` module import; added `packaging.version` for version parsing; replaced `_get_version_parts` with `_parse_version` for better version validation. |
| `unit_tests/sources/declarative/test_manifest_declarative_source.py` | Added new test cases for manifest version validation; enhanced error handling and refactored existing tests for clarity. |
## Possibly related PRs
- #42: The changes in the `connectors_ci` job of the `.github/workflows/connector-tests.yml` file in both PRs involve modifications to the job's matrix, specifically the addition of connectors for testing, indicating a direct relationship in the context of workflow configuration.
## Suggested labels
`ci`How does that look? Anything you'd like to adjust or add? 😊 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Outside diff range and nitpick comments (1)
.github/workflows/connector-tests.yml (1)
73-74: Consider adding test timeouts for individual connectors?Currently, all connectors share the global 90-minute timeout. Would it make sense to add connector-specific timeouts in the matrix for better resource management? For example:
- connector: source-chargebee cdk_extra: n/a + timeout: 45 # or whatever is appropriate based on typical test durationThis could help catch performance regressions earlier. WDYT? 🤔
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/connector-tests.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/connector-tests.yml (2)
73-74: LGTM! The matrix configuration looks good.
The addition of source-chargebee with cdk_extra: n/a follows the same pattern as other similar connectors. This will ensure it's tested consistently with the rest of the suite.
73-74: Verify test stability for source-chargebee
Let's check the test history to ensure we're not adding a flaky test to the required set.
aaronsteers
changed the title
|
/autofix
|
|
The manifest connector runs a version comparison that was not handling the pre-release part of a version number. 
Resolving by parsing with |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)
257-273: Simplify version comparisons by comparing Version objects directly?Instead of unpacking and comparing version components as tuples, we can compare
Versionobjects directly since they support rich comparison operations. This can make the code more concise and maintainable. Wdyt?Apply this diff to simplify the version comparison logic:
- if (cdk_version.major, cdk_version.minor, cdk_version.micro) == (0, 0, 0): + if cdk_version == Version("0.0.0"): # Skipping version compatibility check on unreleased dev branch pass - elif (cdk_version.major, cdk_version.minor) < ( - manifest_version.major, - manifest_version.minor, - ): + elif cdk_version < manifest_version: raise ValidationError( f"The manifest version {manifest_version!s} is greater than the airbyte-cdk package version ({cdk_version!s}). Your " f"manifest may contain features that are not in the current CDK version." ) - elif (manifest_version.major, manifest_version.minor) < (0, 29): + elif manifest_version < Version("0.29.0"): raise ValidationError( "The low-code framework was promoted to Beta in airbyte-cdk version 0.29.0 and contains many breaking changes to the " f"language. The manifest version {manifest_version!s} is incompatible with the airbyte-cdk package version " f"{cdk_version!s} which contains these breaking changes." )
276-287: Update the docstring to reflect the new implementation?The docstring mentions splitting the version into a tuple and not returning the prerelease part, but the method now returns a
Versionobject. Should we update the docstring to match the current behavior? Wdyt?Suggested updated docstring:
"""Parses a semantic version string into a Version object. Args: version (str): The semantic version string to parse. version_type (str): A descriptor for the version type used in error messages. Returns: Version: The parsed version object. Raises: ValidationError: If the version string is not a valid semantic version. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/manifest_declarative_source.py(2 hunks)
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
11-11: Should we add 'packaging' to the project's dependencies?
The packaging module is being imported, but let's ensure it's included in the project's dependencies to avoid any import errors. Wdyt?
Run the following script to check if packaging is listed in the dependencies:
aaronsteers
changed the title
aaronsteers
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
unit_tests/sources/declarative/test_manifest_declarative_source.py (2)
673-679: Is theversionparameter necessary in the function signature?It appears that the
versionparameter might not be used within thetest_manifest_versionsfunction. Removing it could simplify the code. What do you think?Apply this diff to remove the unused parameter:
def test_manifest_versions( self, - version, cdk_version, manifest_version, expected_error, ) -> None:
Line range hint
955-1070: Consider moving helper functions outside the test function for reusabilityThe helper functions
load_yaml,extract_refs,resolve_pointer, andvalidate_refsare defined within thetest_declarative_component_schema_valid_ref_linksfunction. Moving them to the module level could improve readability and allow for reuse in other tests. Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/connector-tests.yml(1 hunks)unit_tests/sources/declarative/test_manifest_declarative_source.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/connector-tests.yml
🔇 Additional comments (1)
unit_tests/sources/declarative/test_manifest_declarative_source.py (1)
647-662: Additional test cases improve version comparison coverage
It's great to see test cases added for release candidates and versions with extra version parts. This enhances the robustness of the version comparison tests. Nice work!
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)
248-272: The version validation logic looks solid! Would you consider a few enhancements? 🤔The logic is working well, but we could make it even more robust. What do you think about:
- Using Version's built-in comparison operators to simplify the code? For example:
-elif (cdk_version.major, cdk_version.minor) < ( - manifest_version.major, - manifest_version.minor, -): +elif cdk_version.release[:2] < manifest_version.release[:2]:
- Making the error message more specific about version requirements? For example:
- f"The manifest version {manifest_version!s} is greater than the airbyte-cdk package version ({cdk_version!s}). Your " - f"manifest may contain features that are not in the current CDK version." + f"The manifest version {manifest_version!s} requires airbyte-cdk version >={manifest_version.major}.{manifest_version.minor}.0, " + f"but found version {cdk_version!s}. Please upgrade your CDK version."wdyt? 🙂
276-295: The implementation looks great! Small suggestion about the docstring 📝The docstring mentions "splits it into a tuple" and "fourth part (prerelease) is not returned in the tuple", but the method actually returns a
Versionobject. Would you like to update it to better reflect the actual behavior? Maybe something like:- """Takes a semantic version represented as a string and splits it into a tuple. - - The fourth part (prerelease) is not returned in the tuple. + """Parses a semantic version string into a Version object. + + Args: + version: A string representing the semantic version + version_type: A string indicating the type of version (for error messages) Returns: Version: the parsed version object + Raises: + ValidationError: if the version string is not a valid semantic version """wdyt? 🤔
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/manifest_declarative_source.py(2 hunks)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)
10-11: Nice choice using packaging.version! 👍
Using packaging.version is indeed the most reliable way to handle semantic versioning in Python. It's the same library that pip uses internally.
287-295: Verify test coverage for version parsing edge cases
The error handling looks good! Let's make sure we have test coverage for various version formats.
Summary by CodeRabbit
New Features
source-chargebeeconnector to the testing workflow, enhancing the range of connectors tested.Bug Fixes
Improvements