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

feat: do not skip merge group #329

Merged
merged 3 commits into from
Oct 9, 2023

Conversation

wahtique
Copy link
Contributor

  • add merge_group as workflow trigger
  • add merge_group to default value sfor do-not-skip triggers : else, worfklow will be skipped when the PR is added to a merge queue since it just ran before

- add `merge_group` as workflow trigger
- add `merge_group` to default value sfor `do-not-skip` triggers : else, worfklow will be skipped when the PR is added to a merge queue since it just ran before
@wahtique wahtique changed the title feat: do not skip merge gorup feat: do not skip merge group Jun 30, 2023
@koppor
Copy link

koppor commented Sep 21, 2023

There are some checks failing. If using this PR ( `uses: wahtique/skip-duplicate-actions@2e9b67b``), I get

Error: Elements in 'do_not_skip' must be one of "pull_request", "push", "workflow_dispatch", "schedule", "release"

@fkirc
Copy link
Owner

fkirc commented Sep 29, 2023

Thank you for submitting the PR!

@fkirc
Copy link
Owner

fkirc commented Sep 29, 2023

I cannot merge it without the compiled JS, we need the compiled JS updated for this to work

@fkirc fkirc merged commit f75f66c into fkirc:master Oct 9, 2023
6 checks passed
@fkirc
Copy link
Owner

fkirc commented Oct 9, 2023

Merged, thank you for updating the JS!

@paescuj
Copy link
Collaborator

paescuj commented Oct 9, 2023

Thanks for the PR @wahtique!

Sorry for my late & post-merge comment, but

  • add merge_group to default value sfor do-not-skip triggers : else, worfklow will be skipped when the PR is added to a merge queue since it just ran before

doesn't it make sense that it is skipped? 😄

@paescuj
Copy link
Collaborator

paescuj commented Oct 11, 2023

@fkirc WDYT about this #329 (comment)? If the tree hash is the same in merge group it makes sense to skip it IMO?
However, I totally agree with the addition of merge_group to do_not_skip 👍

@wahtique
Copy link
Contributor Author

wahtique commented Oct 11, 2023

In response to #329 (comment)

That was some time ago but use case was :

  • open a PR with some checks in a workflow
  • checks pass
  • review pass
  • PR added to merge queue
  • ... and nothing happens the check is skipped
  • merge queue is never merged

Possible fixes would be

  • run no check on either PR or merge queue => big nope
  • update this action to have special behaviour for the merge queue => sorry that's above my skills 😅
  • write 2 workflows with the same checks, one for PRs and one for merge queue which can be done buuuut...I dunno I don't like it, it does not feel DRY even if we mutualise the checks definitions and only do workflow calls from our duplicated workflows
  • the solution implemented in this PR : run it twice, PR is checked, merge queue code ( which can contain more than one PR ) is checked and everyone except my company's finances is happy

@fkirc
Copy link
Owner

fkirc commented Oct 14, 2023

@fkirc WDYT about this #329 (comment)? If the tree hash is the same in merge group it makes sense to skip it IMO?
However, I totally agree with the addition of merge_group to do_not_skip 👍

I would agree to skip if a tree hash is in the same merge group, but I do not know how to implement it at the moment.
So right now I believe the „do_not_skip“ is the easiest workaround.

1 similar comment
@fkirc
Copy link
Owner

fkirc commented Oct 14, 2023

@fkirc WDYT about this #329 (comment)? If the tree hash is the same in merge group it makes sense to skip it IMO?
However, I totally agree with the addition of merge_group to do_not_skip 👍

I would agree to skip if a tree hash is in the same merge group, but I do not know how to implement it at the moment.
So right now I believe the „do_not_skip“ is the easiest workaround.

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.

4 participants