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

Skip duplicated Github actions run #110

Merged
merged 4 commits into from Apr 7, 2021
Merged

Skip duplicated Github actions run #110

merged 4 commits into from Apr 7, 2021

Conversation

dolfinus
Copy link
Contributor

@dolfinus dolfinus commented Mar 20, 2021

Hello.

There is an annoying fact about Github Actions - sometimes it runs actions twice. For example:

  1. You just pushed a commit to a branch - testing workflow will run only once
  2. Someone forked your repo and creates a pull request - same thing
  3. You've created a new branch (feature, bugfix or release branch), pushed a commit here and also created a pull request to the main branch - two copies of workflow will be run, like here Prepare for 1.0.4 release #108:
    image

Running a workflow twice does not make any sense, it just takes twice the time without any useful side effect.

In this PR I've added a separated job of checking if another copy of workflow already started: https://github.com/fkirc/skip-duplicate-actions

This job allows to:

  1. Skip the second copy of workflow in the case 3 (branch in original repo + PR to main branch). Cases 1 and 2 will work just the same as now
  2. Skip workflow if someone changed only files which are not connected to package code, like README or CHANGELOG
  3. Cancel already started workflow in the same PR or branch if someone pushed new commits to it

Also I've found another quite annoying bug - step actions/cache does not update cache item if it was restored using a primary key.
For example, some dependencies like dialyzer or other ones store their files in deps or _build folders (which are cached in the workflow). But when these dependencies perform some updates of these folders, it does not lead to the updating of mix.lock file. File hasn't been changed -> hash is the same -> cache primary key is the same -> caching action will not update the existing cache.
This will cause the increase of workflow run time because the stored cache will not be used by some steps. Until mix.lock will be updated which will lead to updating the cache item.
Here I've appended a commit SHA to the cache primary key, so it will always be updated. But it will not affect cache reading.

Also I've moved documentation check step after running tests because there is no need to check documentation of code which does not work.

What do you think about that?

@dolfinus dolfinus changed the title Skip duplicates Github actions run Skip duplicated Github actions run Mar 20, 2021
@dolfinus
Copy link
Contributor Author

dolfinus commented Apr 2, 2021

@sgerrand Could you please take a look?

@sgerrand
Copy link
Contributor

sgerrand commented Apr 6, 2021

As a general comment, suggest separating your changes into logical commits in future. Reviewing three separate changes in one commit is laborious.

Copy link
Contributor

@sgerrand sgerrand left a comment

Choose a reason for hiding this comment

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

👍 for using fkirc/skip-duplicate-actions to prevent duplicate workflow runs. I can see how this might become frustrating for contributors. I've suggested locking the action to a known version to prevent errors from updates but otherwise it seems fine.

I have posed a number of questions around the other changes which were included in this commit. Would you please respond to them when you can.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
dolfinus and others added 2 commits April 6, 2021 11:34
Co-authored-by: Sasha Gerrand <sgerrand@users.noreply.github.com>
@dolfinus dolfinus requested a review from sgerrand April 6, 2021 09:29
Copy link
Contributor

@sgerrand sgerrand left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@sgerrand sgerrand merged commit abed46c into duffelhq:main Apr 7, 2021
@sgerrand
Copy link
Contributor

sgerrand commented Apr 7, 2021

Thank you @dolfinus for your contribution!

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.

None yet

2 participants