Skip to content

Only cancel new same_content run #112

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

Merged
merged 2 commits into from
May 25, 2021
Merged

Conversation

hugoh
Copy link
Contributor

@hugoh hugoh commented May 24, 2021

Don't cancel all same_content runs.

Fix to issue #90 (untested, sorry).

@fkirc
Copy link
Owner

fkirc commented May 24, 2021

Thanks for submitting the PR.
The logic looks good, but it leads to a conflict if people actually want to skip an older run.
Skipping an older run is a valid use case because of #69 and #34.
If I was merging this PR as is, then it could break those valid use cases.
Therefore, we will probably need yet another config-option to make this a non-breaking change.

@hugoh
Copy link
Contributor Author

hugoh commented May 25, 2021

Hmmm, maybe I'm not fully understanding those issues, but:

Would this basically mean that, for same_content, we'd want an additional flag? Something like concurrent_skip_preferred, which could have the following values:

You obviously have a better understanding of this whole ecosystem of options here, @fkirc.

@fkirc
Copy link
Owner

fkirc commented May 25, 2021

Hmmm, maybe I'm not fully understanding those issues, but:

  • Skip if there are newer runs #69 - if there is a newer run, wouldn't it have different content? And if it didn't, then wouldn't the old and new runs completely identical and hence skipping the new or the old would basically not matter?

Yes, #69 isn't only about same-content-runs. But there might be cases where the ordering matters.

Perhaps it should, but right now, people are using #34 with a combination of same_content and do_not_skip, and I would prefer to not break it.

Would this basically mean that, for same_content, we'd want an additional flag? Something like concurrent_skip_preferred, which could have the following values:

There are many different ways to go forward, but I would propose the following solution:

  • Add a new option concurrent_skipping: same_content_newer or something similar.
  • Implement the new behavior only for the new option.

@hugoh
Copy link
Contributor Author

hugoh commented May 25, 2021

I pushed another round of changes to implement the concurrent_skipping: same_content_newer you suggested.

Copy link
Owner

@fkirc fkirc left a comment

Choose a reason for hiding this comment

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

Well done, thanks for updating the docs as well 👍

core.info(`Skip execution because the exact same files are concurrently checked in ${concurrentDuplicate.html_url}`);
exitSuccess({ shouldSkip: true });
} else if (context.concurrentSkipping === "same_content_newer") {
const concurrentIsOlder = concurrentRuns.find((run) => new Date(run.createdAt).getTime() < new Date(context.currentRun.createdAt).getTime());
Copy link
Owner

Choose a reason for hiding this comment

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

I am not so sure about this concurrentIsOlder. You are checking whether there exists an older run, but you are not checking whether this older run has the same content (the same treeHash)?

@fkirc fkirc merged commit 98d1dc8 into fkirc:master May 25, 2021
zackw added a commit to besser82/libxcrypt that referenced this pull request Jul 4, 2021

Verified

This commit was signed with the committer’s verified signature.
zackw Zack Weinberg
This not-yet-released feature for fkirc/skip-duplicate-actions
(see fkirc/skip-duplicate-actions#112) avoids
a failure mode in which each concurrent duplicate job cancels itself
in favor of the other one, and thus no builds are run at all.
In order to use the feature, we must pin the action to
98d1dc89f43a47f8e4fba8e1c1fb8d6c5fc515ee for now.
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