Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do not run regress workflows on benign changes #5156

Merged
merged 1 commit into from Jan 11, 2023
Merged

Do not run regress workflows on benign changes #5156

merged 1 commit into from Jan 11, 2023

Conversation

mkindahl
Copy link
Contributor

@mkindahl mkindahl commented Jan 9, 2023

If only documentation is changed, the full regression check workflow will still be executed, so this commit will instead skip running the regression workflows if there are only changes to files that will not affect the success of the workflow.

@github-actions github-actions bot requested a review from shhnwz January 9, 2023 12:57
@github-actions
Copy link

github-actions bot commented Jan 9, 2023

@svenklemm, @shhnwz: please review this pull request.

Powered by pull-review

@mkindahl mkindahl force-pushed the conditional-workflows branch 3 times, most recently from 829721d to 4955914 Compare January 9, 2023 13:13
@mkindahl
Copy link
Contributor Author

mkindahl commented Jan 9, 2023

@svenklemm, @shhnwz: please review this pull request.

Powered by pull-review

Strange that it adds reviewers also for draft PRs.

@codecov
Copy link

codecov bot commented Jan 9, 2023 •
edited

Codecov Report

Merging #5156 (e8ea493) into main (396bc6d) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5156      +/-   ##
==========================================
- Coverage   89.26%   89.24%   -0.03%     
==========================================
  Files         225      225              
  Lines       51756    51755       -1     
==========================================
- Hits        46201    46187      -14     
- Misses       5555     5568      +13
Impacted Files Coverage Δ
src/loader/bgw_message_queue.c 86.36% <0.00%> (-2.85%) ⬇️
src/loader/bgw_launcher.c 89.51% <0.00%> (-2.55%) ⬇️
src/bgw/scheduler.c 83.63% <0.00%> (-0.26%) ⬇️
tsl/src/bgw_policy/job.c 87.28% <0.00%> (-0.05%) ⬇️
tsl/src/reorder.c 86.09% <0.00%> (+0.22%) ⬆️
src/bgw/job_stat.c 92.18% <0.00%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 396bc6d...e8ea493. Read the comment docs.

@mkindahl mkindahl marked this pull request as ready for review January 10, 2023 10:19
svenklemm
svenklemm approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we also make this an opt-in in the commit-message/pr comment

@mkindahl
Copy link
Contributor Author

Ideally we also make this an opt-in in the commit-message/pr comment

Would be ideal, but the paths and paths-ignored cannot be made optional, as far as I can tell. I have been conservative with the paths for this reason, so that the tests are only ignored when it definitely cannot affect the outcome.

jnidzwetzki
jnidzwetzki reviewed Jan 11, 2023
View reviewed changes
.github/workflows/linux-32bit-build-and-test.yaml Outdated
- '**.md'
- LICENSE
- NOTICE
- 'bootstrap*'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the bootstrap files excluded? Shouldn't we test the commit when they change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not using the bootstrap file for windows, but we are using it for the linux builds. Fixed.

.github/workflows/linux-32bit-build-and-test.yaml Outdated
@@ -4,7 +4,17 @@ on:
branches:
- main
- prerelease_test
paths-ignore:
- '**.md'
- LICENSE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add LICENSE-TIMESCALE as well to the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added LICENSE*.

.github/workflows/shellcheck-ignored.yaml Outdated
@@ -0,0 +1,16 @@
# Test our shell scripts for bugs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding a comment to the new -ignored workflows, why we need them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment to the beginning of each file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad github still doesn't support early exit: actions/runner#662
We could've done without extra files if we could use early exit and changed paths filter (https://github.com/marketplace/actions/paths-changes-filter)

@mkindahl
Do not run regress workflows on benign changes
e8ea493 
If only documentation is changed, the full regression check workflow
will still be executed, so this commit will instead skip running the
regression workflows if there are only changes to files that will not
affect the success of the workflow.
@mkindahl mkindahl enabled auto-merge (rebase) January 11, 2023 21:32
@mkindahl mkindahl merged commit f36db10 into timescale:main Jan 11, 2023
@mkindahl mkindahl deleted the conditional-workflows branch January 11, 2023 21:45
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Jan 13, 2023
@mkindahl
Add missing ignore files
ca80174 
The workflow ignore files for 32-bit Linux builds and Windows was
missing from timescale#5156 so these are added here.
@mkindahl mkindahl mentioned this pull request Jan 13, 2023
Merged
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Jan 13, 2023
@mkindahl
Add missing ignore files
8c072d3 
The workflow ignore files for 32-bit Linux builds and Windows was
missing from timescale#5156 so these are added here.
mkindahl added a commit that referenced this pull request Jan 13, 2023
@mkindahl
Add missing ignore files
ca9d508 
The workflow ignore files for 32-bit Linux builds and Windows was
missing from #5156 so these are added here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akuzm akuzm akuzm left review comments

@jnidzwetzki jnidzwetzki jnidzwetzki approved these changes

@svenklemm svenklemm svenklemm approved these changes

@shhnwz shhnwz Awaiting requested review from shhnwz

Assignees

@mkindahl mkindahl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mkindahl @jnidzwetzki @svenklemm @akuzm