ci: skip downstream connector tests if no source code is changed #101
Conversation
|
Warning Rate limit exceeded@aaronsteers has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 31 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📝 WalkthroughWalkthroughThe pull request modifies the GitHub Actions workflow for testing connectors, specifically enhancing the logic that determines when to abort jobs. The condition for aborting the Changes
Possibly related PRs
Suggested reviewers
What do you think about these suggested reviewers? Do they align with your expectations? 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 (3)
.github/workflows/connector-tests.yml (3)
97-97: Consider adding parentheses for better readability?The condition is logically correct, but could be more readable with parentheses to clearly group the OR conditions, wdyt?
-if: ${{ needs.cdk_changes.outputs['src'] == 'false' || matrix.cdk_extra != 'n/a' && needs.cdk_changes.outputs[matrix.cdk_extra] == 'false' }} +if: ${{ needs.cdk_changes.outputs['src'] == 'false' || (matrix.cdk_extra != 'n/a' && needs.cdk_changes.outputs[matrix.cdk_extra] == 'false') }}
Line range hint
4-8: Track the TODO about workflow dependencies?I notice there's a TODO about moving these tests to run after the "PyTest (Fast)" workflow. Would you like me to create a GitHub issue to track this enhancement? This could help with overall CI efficiency.
🧰 Tools
🪛 actionlint (1.7.4)
98-98: shellcheck reported issue in this script: SC2086:info:2:67: Double quote to prevent globbing and word splitting
(shellcheck)
98-98: shellcheck reported issue in this script: SC2086:info:3:28: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
112-114: Consider updating the Airbyte CI reference?The comment suggests this is using a temporary branch for Airbyte CI. Once the changes are released, we should update this reference back to
master, right? Would you like me to help track this?🧰 Tools
🪛 actionlint (1.7.4)
98-98: shellcheck reported issue in this script: SC2086:info:2:67: Double quote to prevent globbing and word splitting
(shellcheck)
98-98: shellcheck reported issue in this script: SC2086:info:3:28: 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 comments (1)
.github/workflows/connector-tests.yml (1)
Line range hint 97-101: Verify the abort condition behavior
The condition looks good and aligns with the PR objective to skip tests when no source code is changed. Let's verify it works as expected for different scenarios.
✅ Verification successful
The previous command didn't provide the workflow runs data, so let's analyze the workflow definition itself to verify the abort condition behavior. This will help us understand how the skip logic is implemented and if it's correctly structured.
After analyzing the workflow file, I can now provide a definitive response about the abort condition behavior.
The abort condition logic looks solid and well-implemented! 🎯
The condition correctly handles two scenarios:
- Skips when no source code changes (
needs.cdk_changes.outputs['src'] == 'false') - Skips when the specific extra module hasn't changed (
matrix.cdk_extra != 'n/a' && needs.cdk_changes.outputs[matrix.cdk_extra] == 'false')
The job name even includes a helpful skip indicator: "Check: '${{matrix.connector}}' (skip=${{needs.cdk_changes.outputs[matrix.cdk_extra] == 'false'}})" which makes it easy to see why a job was skipped.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the abort condition behavior by checking the workflow runs
# Check recent workflow runs to see if jobs are being skipped as expected
gh run list --workflow=connector-tests.yml --limit 5 --json conclusion,displayTitle,url | \
jq '.[] | select(.displayTitle | contains("skip=true")) | {conclusion: .conclusion, title: .displayTitle, url: .url}'
Length of output: 209
Script:
#!/bin/bash
# Let's analyze the workflow file to understand the abort condition logic better
# First, let's see the complete workflow file
cat .github/workflows/connector-tests.yml
# Then, let's check for any related workflow files that might provide more context
fd -e yml -e yaml . .github/workflows/ --exec cat {}
Length of output: 50402
🧰 Tools
🪛 actionlint (1.7.4)
98-98: shellcheck reported issue in this script: SC2086:info:2:67: Double quote to prevent globbing and word splitting
(shellcheck)
98-98: shellcheck reported issue in this script: SC2086:info:3:28: Double quote to prevent globbing and word splitting
(shellcheck)
Summary by CodeRabbit