-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Open discussion: The desired death of skip-duplicate-actions #59
Comments
Other than pulling features from third-party-actions into native GitHub-features, it would be also helpful to improve the general situation for third-party-actions like this one.
Not all of these pain-points are solvable, but at least some of these pain-points might be solvable.
Of course, this is not a security proposal that is meant for all actions, but it would work for "sandboxed" actions that are only using the GitHub API and nothing else (like this action). |
Never mind, hidden GitHub tokens seem to be already working. |
I wrote
skip-duplicate-actions
not as a long-term program, but as an intermediate solution until GitHub implements those features natively. Therefore, the eventual goal ofskip-duplicate-actions
is to die and be replaced by GitHub.To make this happen, GitHub would need to implement the features that are specified in this issue.
Please let me know if you can think of any improvements; because this is intended as a "living specification" until someone actually implements it.
Skip duplicate runs based on equal tree-hashes of previous successful runs
This should be "rather" simple. However, one gotcha is that this feature needs to be optional because some workflows would definitely break if this was enabled by default.
Another gotcha is that we will likely need a configuration like
do_not_skip: ["workflow_dispatch", "schedule"]
, because we often want to skip runs only for specific triggers.In addition, it might be helpful to specify branches for
do_not_skip
?Another gotcha is that
skip-duplicate-actions
currently does not work smoothly for "matrix-testing": #56Cancel previous runs for the same branch
This should be rather simple as well. Again, I would prefer to make it optional because it could break existing workflows.
Moreover, please don't send mail-notifications for actions that are cancelled in this way.
Concurrent skipping
Now it starts to become fuzzy.
This action implements a "concurrent skipping" feature to prevent parallel duplicate workflow-runs from executing.
However, this feature was plagued by the following security-vulnerabilities that undermine the safety of GitHub's "required checks": #36, #50
For the sake of security, it might be best to either drop this feature or do a ground-up redesign.
An improved "paths-ignore" feature
Again, things are becoming fuzzy.
paths-ignore
isn't as trivial as you might think.In fact, there are multiple options to implement
paths-ignore
, and different projects might prefer different options.Out of my head, I can think of the following options to implement
paths-ignore
:Option 1: Only look at the "current" commit
This is the thing that GitHub is currently doing, and I consider it as insufficient because it doesn't work for "required" checks.
Another problem is that the outcomes can be heavily dependent on which commits were pushed at which time, instead of the actual content that was being pushed.
Nevertheless, this option might stay for people who actually want it to work this way.
Option 2: Look at PR-diffs
This option was proposed by #58, and it is probably implemented by https://github.com/dorny/paths-filter.
PR-diffs are simple to understand, but they do not work for the general case, so this is not an option for everybody.
Option 3: Look for successful checks of previous commits
This is my personal favorite option and this is implemented by this action.
An advantage is that this works regardless of whether you are using PRs or feature-branches, and of course it also works for "required" checks.
An improved "paths" feature
This is somewhat similar to
paths-ignore
, and similar considerations are likely to apply.The text was updated successfully, but these errors were encountered: