-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: ensure older tests get cancelled on new commit #979
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 0fb65d6 in 37 seconds
More details
- Looked at
34
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_qhRVmSVkK0PZ7tRl
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
.github/workflows/common.yml
Outdated
@@ -17,6 +17,9 @@ jobs: | |||
os: [ubuntu-latest] | |||
python-version: ["3.10"] | |||
timeout-minutes: 10 | |||
concurrency: | |||
group: 'test:common:${{ github.ref }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concurrency group name 'test:common:${{ github.ref }}' is used for all jobs. This might cause unintended cancellations across different jobs. Consider using unique group names for each job, like 'lock_check', 'linter_checks', etc.
.github/workflows/common.yml
Outdated
@@ -77,6 +83,9 @@ jobs: | |||
os: [ubuntu-latest] | |||
python-version: ["3.9", "3.10", "3.11"] | |||
timeout-minutes: 20 | |||
concurrency: | |||
group: 'test:common:${{ github.ref }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making the concurrency group name more specific by including the job name and Python version to allow parallel runs of different jobs:
group: 'test:${{ github.job }}:${{ matrix.python-version }}:${{ github.ref }}'
This would allow different Python versions to run in parallel while still cancelling redundant runs of the same version.
Code Review SummaryOverall AssessmentThe changes look good and implement a useful optimization for the CI pipeline. The addition of concurrency controls will help prevent resource waste and provide clearer test results by cancelling outdated runs. Positive Aspects✅ Consistent implementation across all jobs Suggestions for Improvement
VerdictThe changes are well-implemented and will improve the CI pipeline's efficiency. Approved with minor optional suggestions for enhancement. |
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
Important
Add concurrency control to GitHub Actions workflows to cancel older runs on new commits in
common.yml
.concurrency
tolock_check
,linter_checks
, andtest
jobs incommon.yml
.group: 'test:common:${{ github.ref }}'
to group jobs by branch.cancel-in-progress: true
ensures older runs are cancelled on new commits.This description was created by for 0fb65d6. It will automatically update as commits are pushed.