ci: use new --use-local-cdk method for testing connectors
#40
Conversation
aaronsteers
changed the title
airbyte-ci--use-local-cdk method for testing connectors
|
Warning Rate limit exceeded@aaronsteers has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the GitHub Actions workflow for testing connectors and updates the versioning mechanism in the Changes
Possibly related PRs
Would you like to explore any specific areas of these changes further? 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)
112-112: Consider consolidating related TODOsI see we have two TODOs waiting for the Airbyte CI release. Since both changes (the branch ref and the CI tool) need to be reverted at the same time, maybe we could consolidate these TODOs into a single tracking issue? This would make it easier to manage the transition. What do you think? 🎯
Also applies to: 115-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/connector-tests.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/connector-tests.yml (3)
104-105: Consider tracking the TODO with an issue
Hey! I notice we're temporarily using a feature branch here. To ensure we don't forget to revert this, would it make sense to create a tracking issue? This could help us monitor when the Airbyte CI is released and remind us to switch back to master. wdyt? 🤔
Would you like me to help create a GitHub issue to track this TODO?
123-129: Nice addition of artifact uploads! 👍
Great idea to preserve the pipeline reports! This will make it much easier to debug test failures. The configuration looks solid with the if: always() condition ensuring we capture outputs even on failure.
116-121: Verify impact of skipping QA checks
I notice we're skipping QA checks here. Could you help me understand the reasoning behind this? Are there specific QA checks that we're confident we can skip, or is this temporary while testing the local CDK functionality? 🤔
Run this to see what QA checks we're skipping:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/connector-tests.yml (2)
123-135: Enhance artifact summary step robustnessThe artifact handling looks great! 🎉 For the summary step, might I suggest adding quotes around the variables to prevent potential issues with special characters? Something like:
- echo "## Job Output for ${{matrix.connector}}" >> $GITHUB_STEP_SUMMARY - echo "[Download Job Output](./artifacts/${{matrix.connector}}-job-output)" >> $GITHUB_STEP_SUMMARY + echo "## Job Output for '${{matrix.connector}}'" >> "$GITHUB_STEP_SUMMARY" + echo "[Download Job Output](./artifacts/'${{matrix.connector}}-job-output')" >> "$GITHUB_STEP_SUMMARY"What do you think? 🤔
🧰 Tools
🪛 actionlint
133-133: shellcheck reported issue in this script: SC2086:info:1:51: Double quote to prevent globbing and word splitting
(shellcheck)
133-133: shellcheck reported issue in this script: SC2086:info:2:79: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
1-135: Consider adding workflow documentationThe workflow is well-structured, but given its complexity (matrix strategy, conditional execution, etc.), would it be helpful to add some documentation at the top explaining:
- When this workflow runs
- What it tests
- How the matrix strategy works
- What the different flags do
This could help future maintainers understand the workflow better. What are your thoughts? 📝
🧰 Tools
🪛 actionlint
133-133: shellcheck reported issue in this script: SC2086:info:1:51: Double quote to prevent globbing and word splitting
(shellcheck)
133-133: shellcheck reported issue in this script: SC2086:info:2:79: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/connector-tests.yml(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/connector-tests.yml
133-133: shellcheck reported issue in this script: SC2086:info:1:51: Double quote to prevent globbing and word splitting
(shellcheck)
133-133: shellcheck reported issue in this script: SC2086:info:2:79: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/connector-tests.yml (2)
116-121: Query about skipping QA checks
I see we're skipping the qa_checks step. Could you help me understand the reasoning behind this? Is this temporary or intended to be permanent? Just want to ensure we're not missing important quality checks. 🤔
104-105: Consider adding tracking issue for reverting temporary changes
I notice we're using a development branch and tools here. Would it make sense to create a tracking issue for reverting these temporary changes once the Airbyte CI is released? This could help ensure we don't forget to switch back to the stable versions. wdyt? 🤔
Also applies to: 112-115
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
.github/workflows/connector-tests.yml (1)
149-149: Small suggestion for the download link 🔗Maybe we could make this a bit more robust? What do you think about:
- echo -e "\n[Download Job Output](${{steps.upload_job_output.outputs.artifact-url}})" >> $GITHUB_STEP_SUMMARY + if [[ -n "${steps.upload_job_output.outputs.artifact-url}" ]]; then + echo -e "\n[Download Job Output](${steps.upload_job_output.outputs.artifact-url})" >> "$GITHUB_STEP_SUMMARY" + fipyproject.toml (1)
38-38: Interesting approach with numpy version constraintI see the helpful comment about airbyte-ci and
--use-local-cdkconflicts! This is great documentation. Just wondering - should we consider adding this kind of explicit version constraint documentation for other dependencies that might cause similar conflicts during local development? Could help prevent future headaches! 🎯Also applies to: 73-75
airbyte_cdk/__init__.py (3)
49-49: Consider cleaning up the commented code instead of leaving it?I notice we're keeping the commented import. Since we're moving away from
dunamai, would it make more sense to remove it entirely rather than keeping it commented out? This would help keep the codebase cleaner. wdyt? 🤔
285-291: The docstring might need updating to reflect the new versioning approachThe docstring still references
dunamaiand dynamic versioning, but we've moved to a static approach. Should we update it to something like:-"""Version generated by poetry dynamic versioning during publish. - -When running in development, dunamai will calculate a new prerelease version -from existing git release tag info. -""" +"""Development version string. + +This version string is used during development. During the release process, +it will be replaced with the appropriate version number. +"""What do you think about this change? 🎯
292-299: Should we simplify the version assignment?Since we've moved away from dynamic versioning, we could simplify this section by removing the commented-out code and just keeping the version assignment. Something like:
-# try: -# __version__ = _dunamai.get_version( -# "airbyte-cdk", -# third_choice=_dunamai.Version.from_any_vcs, -# fallback=_dunamai.Version("0.0.0+dev"), -# ).serialize() -# except: __version__ = "0.0.0+dev"This would make the code cleaner and more straightforward. What are your thoughts on this? 🧹
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/connector-tests.yml(1 hunks)airbyte_cdk/__init__.py(2 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/connector-tests.yml
134-134: shellcheck reported issue in this script: SC2086:info:3:18: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:5:15: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:6:20: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:7:60: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2129:style:8:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:8:51: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:9:33: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:10:41: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:12:27: Double quote to prevent globbing and word splitting
(shellcheck)
134-134: shellcheck reported issue in this script: SC2086:info:15:89: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (4)
.github/workflows/connector-tests.yml (3)
123-131: LGTM! Nice addition of artifact uploads! 👍
Good practice using if: always() to ensure we capture outputs even on failure, and using the latest v4 of the upload action.
112-122: Track the transition from dev to production tools
The workflow is using development tools (tools.airbyte-ci-dev.install) and the TODO comment suggests this needs updating. Would it make sense to:
- Document the criteria for when we can switch to production tools?
- Create a tracking issue for this transition?
Also, the new --use-local-cdk flag looks interesting! Have we documented its behavior somewhere for other developers? 🤓
#!/bin/bash
# Check if there's documentation about the new flag
rg -A 5 "use-local-cdk" --type md104-105: Consider adding a tracking issue for reverting the branch reference
The workflow is currently using a feature branch aj/airbyte-ci/update-python-local-cdk-code instead of master. While the TODO comment indicates this is temporary, it would be helpful to track this. What do you think about creating an issue to ensure we don't forget to revert this? 🤔
✅ Verification successful
Let's correct the repository name and check the branch status again 🔍
The branch aj/airbyte-ci/update-python-local-cdk-code exists and was last updated on 2024-11-12. Please ensure there's a tracking issue to revert to master once Airbyte CI is released. wdyt?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced branch exists and when it was last updated
gh api repos/airbytehq/airbyte/branches/aj/airbyte-ci/update-python-local-cdk-code --jq '.commit.commit.author.date'
Length of output: 814
Script:
#!/bin/bash
# Check if the referenced branch exists and when it was last updated
gh api repos/airbytehq/airbyte/branches/aj/airbyte-ci/update-python-local-cdk-code --jq '.commit.commit.author.date'
Length of output: 137
airbyte_cdk/__init__.py (1)
299-299: Verify the version string format
The hardcoded version "0.0.0+dev" follows PEP 440, but let's verify it works with your release process.
✅ Verification successful
Let's generate a script to find all instances of __version__ in the codebase to ensure it's correctly integrated into the release process. wdyt?
Version string format is consistent with PEP 440
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the version string format is consistent with other packages
# Check version format in pyproject.toml
fd pyproject.toml --exec cat {} \; | grep -A 5 '\[tool\.poetry\]'
# Check version format in setup.py/setup.cfg if they exist
fd "setup\.(py|cfg)" --exec cat {} \; | grep -A 5 "version"
Length of output: 308
Script:
#!/bin/bash
# Description: Search for all occurrences of __version__ in the codebase
rg '__version__' .
Length of output: 181
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/connector-tests.yml (3)
104-105: Consider documenting the temporary branch dependency 🤔I notice we're using a temporary branch
aj/airbyte-ci/update-python-local-cdk-codeinstead ofmaster. While there's a TODO comment, would it be helpful to add more context about when this can be reverted, like linking to a tracking issue or PR? This could help future maintainers understand the dependency, wdyt?
112-112: Consider documenting the version bypass purpose 📝The new environment variable
POETRY_DYNAMIC_VERSIONING_BYPASSis set to "0.0.0". Should we add a comment explaining why this bypass is needed and its implications for the build process? This could help other developers understand the versioning strategy, wdyt?
116-122: Consider consolidating the TODO comments 🔄I notice we have similar TODO comments about reverting to production versions in multiple places. Would it make sense to consolidate these into a single tracking item at the top of the file? This could make it easier to track and update all related changes at once, wdyt?
Also, the new flags
--use-local-cdkand--skip-step qa_checksseem significant. Should we document why we're skipping QA checks? 🤔
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/connector-tests.yml(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/connector-tests.yml
135-135: shellcheck reported issue in this script: SC2086:info:3:18: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:5:15: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:6:20: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:7:60: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2129:style:8:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:8:51: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:9:33: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:10:41: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:12:27: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:15:89: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/connector-tests.yml (2)
124-132: LGTM! Nice addition of artifact upload 👍
The new step for uploading job outputs as artifacts is well structured and follows GitHub Actions best practices.
133-150: 🛠️ Refactor suggestion
Shell script could use some hardening, shall we? 🛡️
I see this was mentioned in a previous review, but the issues still exist. Would you like to apply these improvements to make the script more robust?
Here's the suggested fix with explanations:
- json_output_file=$(find airbyte/airbyte-ci/connectors/pipelines/pipeline_reports -name 'output.json' -print -quit)
- job_output=$(cat $json_output_file)
- echo "job_output=$job_output" >> $GITHUB_OUTPUT
- succes=$(echo $job_output | jq -r '.success')
- failed_jobs=$(echo $job_output | jq -r '.failed_steps')
- echo -e "succes=${success}\nfailed_jobs=${failed_jobs}" >> $GITHUB_OUTPUT
- echo "## Job Output for ${{matrix.connector}}" >> $GITHUB_STEP_SUMMARY
- echo "- Success: ${success}" >> $GITHUB_STEP_SUMMARY
- echo "- Failed Jobs: ${failed_jobs}" >> $GITHUB_STEP_SUMMARY
- if [ "$succes" != "true" ]; then
+ # Find and validate output file
+ json_output_file="$(find airbyte/airbyte-ci/connectors/pipelines/pipeline_reports -name 'output.json' -print -quit)"
+ if [ ! -f "${json_output_file}" ]; then
+ echo "Error: output.json not found" >&2
+ exit 1
+ fi
+
+ # Parse job output and extract status
+ job_output="$(cat "${json_output_file}")"
+ echo "job_output=${job_output}" >> "$GITHUB_OUTPUT"
+ success="$(echo "${job_output}" | jq -r '.success')"
+ failed_jobs="$(echo "${job_output}" | jq -r '.failed_steps')"
+
+ # Update outputs and summary (grouped to reduce I/O operations)
+ {
+ echo "success=${success}"
+ echo "failed_jobs=${failed_jobs}"
+ } >> "$GITHUB_OUTPUT"
+
+ {
+ echo "## Job Output for ${{matrix.connector}}"
+ echo "- Success: ${success}"
+ echo "- Failed Jobs: ${failed_jobs}"
+ } >> "$GITHUB_STEP_SUMMARY"
+
+ # Check success status
+ if [ "${success}" != "true" ]; thenChanges include:
- Fixed typo in variable name (
succes→success) - Added quotes around variables to prevent word splitting
- Added file existence check
- Grouped redirects to reduce I/O operations
- Added comments for better readability
- Added error handling for missing file
What do you think about these improvements? 🤔
🧰 Tools
🪛 actionlint
135-135: shellcheck reported issue in this script: SC2086:info:3:18: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:5:15: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:6:20: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:7:60: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2129:style:8:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:8:51: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:9:33: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:10:41: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:12:27: Double quote to prevent globbing and word splitting
(shellcheck)
135-135: shellcheck reported issue in this script: SC2086:info:15:89: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/connector-tests.yml(2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/connector-tests.yml
139-139: shellcheck reported issue in this script: SC2086:info:3:18: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:4:16: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:5:20: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2129:style:6:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:6:51: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:7:33: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:8:40: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:8:60: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:9:43: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:10:89: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (4)
.github/workflows/connector-tests.yml (4)
71-72: LGTM! Matrix addition looks good 👍
The addition of source-faker follows the existing pattern and structure.
92-94: Nice improvement to the skip logic! 🎯
Using exit 0 and adding status output is a better approach for handling skipped jobs, as it:
- Prevents false failure signals
- Provides clear visibility in the step summary
107-108: Should we track the TODO for reverting the branch? 🤔
The comment indicates this branch reference is temporary. Would it be helpful to create a tracking issue to ensure we don't forget to revert it back to master after the Airbyte CI release? wdyt?
I can help create a GitHub issue to track this if you'd like!
115-126: Environment and command changes look good! 🛠️
The switch to airbyte-ci-dev and addition of --use-local-cdk align well with the PR objectives.
Quick question: Is POETRY_DYNAMIC_VERSIONING_BYPASS="0.0.0" the version we want to use consistently? Just want to ensure this aligns with our versioning strategy! 🤔
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/connector-tests.yml (2)
107-108: Track temporary branch dependency 📝There's a TODO to revert the branch reference back to
master. Should we create a tracking issue to ensure this doesn't get forgotten after the Airbyte CI release, wdyt?Would you like me to create a GitHub issue to track this TODO?
119-126: Consider documenting the CI flags 📚The command includes several flags (
--use-local-cdk,--fail-fast,--skip-step). Would it be helpful to add a comment explaining what each flag does, wdyt?airbyte_cdk/__init__.py (1)
292-299: How about catching specific exceptions and adding debug logs? wdyt?The version retrieval implementation looks solid with its multiple fallback mechanisms! 🎯 A couple of suggestions to make it even more robust:
- Consider catching specific exceptions instead of using a bare
exceptclause:try: __version__ = _dunamai.get_version( "airbyte-cdk", third_choice=_dunamai.Version.from_any_vcs, fallback=_dunamai.Version("0.0.0+dev"), ).serialize() -except: +except Exception as e: + import logging + logging.debug(f"Failed to determine version using dunamai: {e}") __version__ = "0.0.0+dev"This would help with debugging if something unexpected goes wrong while still maintaining the fallback behavior. What do you think? 🤔
🧰 Tools
🪛 Ruff
298-298: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/connector-tests.yml(2 hunks)airbyte_cdk/__init__.py(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/connector-tests.yml
139-139: shellcheck reported issue in this script: SC2086:info:3:18: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:4:16: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:5:20: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2129:style:6:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:6:51: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:7:33: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:8:40: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:8:60: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:9:43: Double quote to prevent globbing and word splitting
(shellcheck)
139-139: shellcheck reported issue in this script: SC2086:info:10:89: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 Ruff
airbyte_cdk/__init__.py
298-298: Do not use bare except
(E722)
🔇 Additional comments (3)
.github/workflows/connector-tests.yml (2)
71-72: LGTM: Matrix addition follows consistent pattern! 👍
The addition of source-faker to the test matrix is clean and follows the established pattern.
92-94: Verify intended skip behavior change 🤔
Changed from exit 1 to exit 0 which means skipped jobs will be marked as successful rather than failed. This seems more semantically correct for skips, but worth confirming this is the intended behavior, wdyt?
airbyte_cdk/__init__.py (1)
285-291: LGTM! Clear type annotation and documentation.
The type annotation and docstring effectively communicate the versioning strategy. Nice job documenting both the production and development scenarios! 👍
|
@coderabbitai review |
Testing new
--use-local-cdkbehavior WIP here:--use-local-cdkarg inairbyte-ci, adds--use-cdk-refarg airbyte#48457Summary by CodeRabbit
New Features
source-faker, to enhance testing capabilities.Bug Fixes
Documentation