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

Add skip-duplicate-actions action #1114

Merged
merged 3 commits into from
Mar 20, 2022
Merged

Add skip-duplicate-actions action #1114

merged 3 commits into from
Mar 20, 2022

Conversation

niccoloraspa
Copy link
Member

@niccoloraspa niccoloraspa commented Mar 18, 2022

Closes: #1100

Description

This PR improves the test workflow by skipping duplicate actions.
This is done using skip-duplicate-actions.

The improvements made:

1. Avoid running the test workflow twice on pull_request and push events

⚠️ IMPORTANT
Test will ALWAYS run on pull_requests events in any case.

This means that:

If Alice is working on branch A and commits some changes to the code, then the test workflow will be triggered.
If Alice waits for the test workflow to be over (the expected behaviour) and then opens a PR to merge the branch to main, the same test workflow will run again.
This is good because you are sure that any new PR code is always tested, and it is necessary to run a pull_request workflow to let codecov post the report in the comment.

The "downside" of this:

If Alice is working on branch A and commits some changes to the code, then the test workflow will be triggered.
If Alice doesn't wait for the test workflow to be over and opens the PR immediately, two duplicated jobs will run (one for the push event and one for the pull_request event).
This is the only case (which I am aware of) in which there will be duplicated runs of the test workflow.

The good part:

  • Any following commits to the PR will only RUN once. The workflow triggered by the push event will be skipped.
  • When the PR gets merged, the test workflow is skipped again.

2. Run the workflow only when go files are edited

This is a nice optimization to avoid running tests when it's not needed.
If Alice commits don't change any: *.go *.mod and *.sum, then the test workflow will be skipped.

This is controlled by https://github.com/nikever/osmosis/blob/fix/remove-duplicate-ci-trigger/.github/workflows/test.yml#L26

Am I missing any relevant scenario where we would like to run tests?

In pull_requests tests will always run.

3. Cancel outdated runs

This is another optimization, which is slightly more "aggressive" than the other.

Let me know if you want to keep it:

This is controlled by https://github.com/nikever/osmosis/blob/fix/remove-duplicate-ci-trigger/.github/workflows/test.yml#L23

If Alice is working on branch A and commits some changes to the code, then the test workflow will be triggered.
While the workflow is running, if Alice commits another change to the same branch, the previous test workflow will be cancelled in favour of the new test workflow.

Before merging

I have run multiple tests in my local repository to test different scenarios, but CIs are always tricky, and it would be great if someone from the dev team could confirm that the workflow behaves as expected.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@niccoloraspa
Copy link
Member Author

And (as before) with PRs coming from remote forks such as this one now the test workflow is not triggered twice since there are no push events.

I will update the CHANGELOG.md so we can check the behaviour.

@niccoloraspa
Copy link
Member Author

niccoloraspa commented Mar 18, 2022

Even if I have modified only the CHANGELOG.md and not any *.go files the workflow still runs as expected.

This might be unnecessary but I think it's good practise.
It is better to be safe and always avoid merging possibly untested code.

@codecov-commenter
Copy link

Codecov Report

Merging #1114 (2168fef) into main (dbe2e1e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1114   +/-   ##
=======================================
  Coverage   20.94%   20.94%           
=======================================
  Files         199      199           
  Lines       25429    25429           
=======================================
  Hits         5326     5326           
  Misses      19140    19140           
  Partials      963      963           

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 dbe2e1e...2168fef. Read the comment docs.

@p0mvn
Copy link
Member

p0mvn commented Mar 18, 2022

In 1) you mentioned a case where it is possible to have 2 duplicate jobs on open PRs.

I see that we have concurrent_skipping: 'same_content'. Does this not help mitigate that duplication?

I'm assuming the above setting is overwritten by do_not_skip: '["pull_request",...] ?

Thanks for such a detailed description btw!

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for adding this and the extremely detailed description

@niccoloraspa
Copy link
Member Author

niccoloraspa commented Mar 18, 2022

I see that we have concurrent_skipping: 'same_content'. Does this not help mitigate that duplication?

I'm assuming the above setting is overwritten by do_not_skip: '["pull_request",...] ?

It doesn't help unfortunately. You can skip jobs that are triggered simultaneously.
In the case of scenario 1, the events happen sequentually (First you push, then you open the PR).

The solution would be to cancel the first push triggered workflow when a corresponding pull_request triggered workflow runs.

However this is not super simple to do but I don't think this would be necessary since:

  1. Ideally, you should wait for the push triggered workflow to complete, before opening a PR

  2. Both workflow runs only PR opens and it takes some time to review the PR. Remember that you should always run PR workflow in any case. The time delta increase it's tolerable.

@ValarDragon
Copy link
Member

ValarDragon commented Mar 19, 2022

I think the duplicate push run when opening PR isn't worth worrying about

The big value add was improving the situation for every subsequent push, as typically we fix some merge conflict before wanting to hit merge, and thats the flow breaking latency, which this PR solves!

@ValarDragon ValarDragon merged commit 71ddd57 into osmosis-labs:main Mar 20, 2022
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

CI: Make PRs not run tests twice on each commit
4 participants