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.

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

Run CI only when useful #1372

Closed
wants to merge 1 commit into from
Closed

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Dec 20, 2023

Description of proposed changes

See commit messages.

Related issue(s)

Checklist

  • Checks pass
  • Run only when pushing to certain paths? (thread)
  • Add concurrency limit? (thread)
  • If making user-facing changes, add a message in CHANGES.md summarizing the changes in this PR no functional changes

Concurrent runs often happen due to iterative pushes to a PR. In those
cases, only the most recent commit is useful for CI to run.

Given current concurrency limitations, I've been in situations where the
most recent commit to a PR was waiting for CI runs on older commits to
finish before it could even start, due to the FIFO nature of GitHub
Actions workflow run scheduling. In those situations, I've manually
canceled the older CI runs to allow the most recent CI to run. This
change automates that process.

Concurrent CI runs on the default branch are useful, so those are still
allowed.
@victorlin victorlin self-assigned this Dec 20, 2023
@corneliusroemer
Copy link
Member

Unrelated to those changes here: Would be great to have a per workflow opt out option in case we know a certain workflow doesn't work with certain CI steps, like rsv with conda.

Copy link
Member

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

I might be wrong about these files changing test outcome, but could you have a look?

@victorlin victorlin force-pushed the victorlin/no-unnecessary-ci branch from 9c85aab to ea5104f Compare December 20, 2023 22:27
Copy link
Member Author

Choose a reason for hiding this comment

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

Re #1372 (comment):

Unrelated to those changes here: Would be great to have a per workflow opt out option in case we know a certain workflow doesn't work with certain CI steps, like rsv with conda.

Isn't that what we're doing with 724e207, or do you mean something else?

Copy link
Member

Choose a reason for hiding this comment

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

It disables both docker and conda if I'm not mistaken. But not a big deal, we have lots of redundancy so disabling both is fine.

@victorlin victorlin marked this pull request as ready for review December 20, 2023 22:30
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7474a2d) 66.14% compared to head (ea5104f) 66.13%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1372      +/-   ##
==========================================
- Coverage   66.14%   66.13%   -0.01%     
==========================================
  Files          68       68              
  Lines        7221     7222       +1     
  Branches     1777     1777              
==========================================
  Hits         4776     4776              
- Misses       2176     2177       +1     
  Partials      269      269              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +16 to +19
# Prevent concurrent runs on the same branch, except for the default branch.
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}-${{ github.ref_type == 'branch' && github.ref_name == github.event.repository.default_branch && github.run_id }}
cancel-in-progress: true
Copy link
Member Author

Choose a reason for hiding this comment

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

I have a counter-argument to ea5104f, describing a situation where concurrency is beneficial:

Sometimes, it's useful to show a difference in CI results between two commits. I have found myself pushing one commit immediately followed by another and relying on CI to run without interruption on both. With the proposed change, the same effect can only be accomplished in two ways:

  1. Wait for the first commit's CI run to finish before pushing the next.
  2. Temporarily remove this concurrency group in the PR.

I would rather cancel unwanted concurrent runs manually than use either of those approaches. Since ea5104f is the only commit left in this PR, I'm just going to close it.

@victorlin victorlin closed this Dec 26, 2023
@victorlin victorlin deleted the victorlin/no-unnecessary-ci branch December 26, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants