CI: update test workflows #26
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several changes to the project's GitHub Actions workflows and the Changes
Possibly related PRs
Suggested labels
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: 3
🧹 Outside diff range and nitpick comments (12)
.github/workflows/pytest_fast.yml (3)
3-16: Consider expanding path triggers for test-related changesHey! The current path triggers look good, but what do you think about including test-related files? This would ensure the workflow runs when test files are modified. Maybe something like this, wdyt? 🤔
on: push: branches: - main paths: - 'airbyte_cdk/**' - 'poetry.lock' - 'pyproject.toml' + - 'unit_tests/**' + - '.github/workflows/pytest*.yml' pull_request: paths: - 'airbyte_cdk/**' - 'poetry.lock' - 'pyproject.toml' + - 'unit_tests/**' + - '.github/workflows/pytest*.yml'
35-35: Consider optimizing dependency installationHey! I noticed we're installing all extras. For test runs, we might not need everything. Would it make sense to install only test-related extras to speed up the workflow? Something like this perhaps? 🚀
- run: poetry install --all-extras + run: poetry install --extras "test"
56-71: Consider adding coverage thresholds and artifact retentionThe coverage setup looks great! A couple of suggestions to make it even better:
- Should we add a minimum coverage threshold? We could fail the build if coverage drops below a certain percentage.
- Maybe we should set a retention period for the coverage artifacts to manage storage?
- name: Upload coverage to GitHub Artifacts if: always() uses: actions/upload-artifact@v4 with: name: fasttest-coverage path: htmlcov/ + retention-days: 14And for coverage threshold:
- name: Print Coverage Report if: always() - run: poetry run coverage report + run: poetry run coverage report --fail-under=80What do you think about these numbers? 📊
.github/workflows/pytest_matrix.yml (4)
14-22: Consider expanding path triggers for comprehensive coverageThe path triggers look good for catching direct changes, but what about test files? Would it make sense to also include
tests/**in the paths to catch changes in test files themselves? wdyt? 🤔paths: - 'airbyte_cdk/**' - 'poetry.lock' - 'pyproject.toml' + - 'tests/**'
Line range hint
31-33: Track Python 3.12 compatibilityThe comment indicates Pendulum is blocking Python 3.12 support. Should we create an issue to track this and set a reminder to revisit once Pendulum adds support? Happy to help create one! 🎯
Line range hint
35-37: Document Windows testing statusThe comment mentions Windows is excluded from the test matrix. Should we add a TODO with context or create an issue to track when/if Windows support will be added back? I can help document this if you'd like! 📝
Line range hint
44-46: Track UTF-8 encoding enforcementThere's a TODO about enforcing UTF-8 encoding within PyAirbyte itself. Would you like me to create an issue to track this improvement? It could help remove this environment-specific workaround! 🔧
.github/workflows/test-connectors.yml (3)
22-23: Consider adding more defensive checks to the job conditionThe current condition only checks for workflow success. What do you think about also checking if the workflow run exists when it's a workflow_run trigger? Something like this:
- if: ${{ github.event.workflow_run.conclusion == 'success' }} + if: ${{ github.event_name != 'workflow_run' || (github.event.workflow_run && github.event.workflow_run.conclusion == 'success') }}wdyt? 🤔
Line range hint
65-73: Question about the commented-out connectorI notice
source-zendesk-supportis commented out with a note about CI not passing. Would it be helpful to create an issue to track this and remove the commented code? I can help create that issue if you'd like!
Line range hint
61-63: Consider reviewing the timeout valueThe 6-hour timeout seems quite generous. Based on the 90-minute timeout for individual connector tests, perhaps we could reduce this? What about starting with 3 hours (180 minutes) and adjusting based on actual run times?
pyproject.toml (2)
Line range hint
1-4: Consider adding a comment about the versioning strategy? 🤔I see you're using poetry-dynamic-versioning with a placeholder "0.0.0" version. While there's a link to the docs, would it be helpful to add a brief comment about which git tags/refs are used for version calculation? This could help other contributors understand the versioning strategy better, wdyt?
Also applies to: 24-25
Line range hint
33-36: Should we pin the versions for better reproducibility? 🔒I notice that some dependencies like
backoff,cachetools,requests, andrequests_cacheuse "*" as their version constraint. While this gives us the latest versions, it might lead to unexpected breaking changes in production. Would it make sense to pin these to specific versions or at least use version ranges (e.g., "^2.0.0") for better reproducibility, wdyt?Also applies to: 42-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/pytest_fast.yml(1 hunks).github/workflows/pytest_matrix.yml(1 hunks).github/workflows/test-connectors.yml(2 hunks)pyproject.toml(1 hunks)
🔇 Additional comments (6)
.github/workflows/pytest_matrix.yml (2)
Line range hint 1-93: Overall changes look great! 👍
The workflow separation and matrix configuration are well thought out. The coverage reporting is robust with proper fallbacks, and the conditions for running on forks vs. main branch are clear.
Line range hint 73-77: Verify test timeout duration
The 60-minute timeout seems quite generous. Should we check the historical run times in GitHub Actions to see if we can tighten this up a bit? I can help analyze the logs if you'd like! ⏱️
.github/workflows/test-connectors.yml (1)
Line range hint 9-17: Nice work on the concurrency configuration! 👍
The concurrency setup is well thought out - it prevents parallel runs for PRs while allowing multiple workflow dispatches. The comments explaining the logic are particularly helpful.
pyproject.toml (3)
Line range hint 143-144: Is ignoring task failures intentional?
I noticed ignore_fail = "return_non_zero" in format-check and format-fix tasks. This means the sequence will continue even if a task fails. While this might be desired for formatting tasks, it could potentially mask important issues. Should we make this behavior more explicit in the task descriptions, wdyt?
#!/bin/bash
# Check if there are any recent CI failures related to these tasks
gh run list --repo airbytehq/airbyte-python-cdk --workflow pytest_matrix.yml --json conclusion,name | \
jq '.[] | select(.conclusion == "failure" and (.name | contains("format")))'Also applies to: 147-148
Line range hint 193-196: Should we document the experimental features? 📝
I see we're filtering out ExperimentalClassWarning. To ensure users are aware of which features are experimental, would it be helpful to add a section in the README or documentation listing these features? This could help users make informed decisions about using them in production, wdyt?
#!/bin/bash
# Look for experimental features in the codebase
rg -l "ExperimentalClassWarning" || echo "No direct references found"Line range hint 127-131: Shall we check if issue #12 is still relevant? 🔍
The TODO comments reference issue #12 for excluding __init__.py files. Let me check if this is still relevant.
✅ Verification successful
Issue #12 is still open and relevant - shall we keep the TODO comments for now? 🤔
The issue about circular imports in __init__.py files is still open and active. The TODO comments in the pyproject.toml are serving as important reminders of this known limitation, where excluding __init__.py files from formatting is a temporary workaround until the circular reference issue is resolved.
- Issue Bug: Root
__init__.pyimport and unstable and cannot be sorted due to circular references #12 "Bug: Root__init__.pyimport and unstable and cannot be sorted due to circular references" is currently open - The exclusion of
__init__.pyfiles in the formatting configuration is a valid workaround
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced issue is still open
gh issue view 12 --repo airbytehq/airbyte-python-cdk || echo "Issue not found or not accessible"
# Check for __init__.py files that might be affected
find . -name "__init__.py" -type f -exec grep -l "^from\|^import" {} \;
Length of output: 781
Script:
#!/bin/bash
# Let's search for any TODO comments in isort and ruff configurations
rg -i "todo.*issue.*#12|issue.*#12.*todo" pyproject.toml
# Let's also check for any __init__.py exclusions in the config
rg "__init__.py" pyproject.toml
# Let's find all __init__.py files with imports to understand the scope
find . -name "__init__.py" -type f | while read -r file; do
if grep -q "^from\|^import" "$file"; then
echo "=== $file ==="
head -n 5 "$file"
echo "..."
fi
done
Length of output: 29059
Script:
#!/bin/bash
# Let's check if issue #12 still exists in the repository
gh issue view 12
# Let's also check the git history of these TODO comments to understand their context
git log -p -- pyproject.toml | grep -A 5 -B 5 "TODO.*issue.*#12"
# And check if there are any recent PRs or discussions about this
gh pr list --search "issue #12 in:title,body"
Length of output: 287
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/connector-tests.yml (4)
8-12: Consider implementing the workflow optimization? 🤔The TODO comment suggests running these tests only after the "PyTest (Fast)" workflow succeeds. This could help reduce CI load by avoiding unnecessary runs. Would you like me to help implement this optimization using
workflow_runtrigger? wdyt?
82-82: Add matrix strategy documentation? 📝The matrix strategy maps connectors to CDK extras, but the relationship isn't immediately clear. Maybe we could add a comment explaining:
- Why certain connectors need specific extras
- What "n/a" means for extras
- Why some connectors are commented out
This would help future maintainers understand the configuration better. wdyt?
94-105: Consider using variables for checkout paths? 🔧The paths 'airbyte-python-cdk' and 'airbyte' are hardcoded. Maybe we could make this more maintainable by defining them as variables at the job level? Something like:
env: CDK_PATH: airbyte-python-cdk MONOREPO_PATH: airbyteThen reference them as
${{ env.CDK_PATH }}and${{ env.MONOREPO_PATH }}. This would make path changes easier to manage. wdyt?
Line range hint
111-116: Fix shell command syntax? 🐛There are some syntax issues in the test command. Here's a suggested fix:
cd airbyte make tools.airbyte-ci-binary.install airbyte-ci connectors \ --name ${{matrix.connector}} \ - test - --global-status-check-context='Connectors Test: ${{matrix.connector}}'" + test \ + --global-status-check-context='Connectors Test: ${{matrix.connector}}'This fixes the line continuation and removes the stray quote. wdyt?
🧰 Tools
🪛 actionlint
111-111: shellcheck reported issue in this script: SC1009:info:6:1: The mentioned syntax error was in this simple command
(shellcheck)
111-111: shellcheck reported issue in this script: SC1073:error:6:71: Couldn't parse this double quoted string. Fix to allow more checks
(shellcheck)
111-111: shellcheck reported issue in this script: SC1072:error:8:1: Expected end of double quoted string. Fix any mentioned problems and try again
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/connector-tests.yml(3 hunks).github/workflows/pytest_fast.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pytest_fast.yml
🧰 Additional context used
🪛 actionlint
.github/workflows/connector-tests.yml
111-111: shellcheck reported issue in this script: SC1009:info:6:1: The mentioned syntax error was in this simple command
(shellcheck)
111-111: shellcheck reported issue in this script: SC1073:error:6:71: Couldn't parse this double quoted string. Fix to allow more checks
(shellcheck)
111-111: shellcheck reported issue in this script: SC1072:error:8:1: Expected end of double quoted string. Fix any mentioned problems and try again
(shellcheck)
Summary by CodeRabbit
New Features
Changes
Bug Fixes