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

Workflow should not trigger on own comments #244

Merged
merged 3 commits into from
Jun 22, 2022
Merged

Conversation

remcowesterhoud
Copy link
Contributor

The backport-action bot added comments to PRs contain "/backport". This could result in the backport workflow being triggered again, causing an infinite loop of backport messages. By checking the user id of the comment we can prevent comments from backport-action triggering the workflow.

The backport-action bot added comments to PRs contain "/backport". This could result in the backport workflow being triggered again, causing an infinite loop of backport messages. By checking the user id of the comment we can prevent comments from backport-action triggering the workflow.
@remcowesterhoud remcowesterhoud changed the title doc: workflow should not trigger on own comments Workflow should not trigger on own comments Jun 2, 2022
@remcowesterhoud remcowesterhoud requested a review from korthout June 2, 2022 11:35
@remcowesterhoud
Copy link
Contributor Author

Let me know if you'd rather have the check be github.event.comment.user.login != "backport-action" instead.

Copy link
Owner

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Interesting point. So this PR had /backport in the title or in the branch? So once the new PR was opened, the bot user commented on the original PR and the workflow ran once more. Do I have that right?

This example uses the default GITHUB_TOKEN as the github_token so the github-action user opens the PR. I'm not sure what the id of that user is. In any case, the user depends on the defined value of the github_token option, which makes this more complex.

I'm not sure we should complicate this example even further. It's already rather confusing IMO. I'd rather push for replacing the issue_comment section with a dedicated action like https://github.com/peter-evans/slash-command-dispatch. This dispatching will have the added benefit that the backport-action itself is not triggered for every comment, cleaning up the Actions overview.

WDYT?

@remcowesterhoud
Copy link
Contributor Author

remcowesterhoud commented Jun 22, 2022

/backport was not part of the title or branch. It was only part of the comment placed by the backport action. Example of the loop can be found here: camunda/camunda#9411

In Zeebe we don't use the GITHUB_TOKEN anymore. The reasoning for that is:

In order for the GHA workflows to run as a result of newly opened PRs by the backport action, we need to use a different token than the standard github action's token. This switches the token to a PAT of github user @backport-action

Not using the GITHUB_TOKEN results in a loop as GitHub doesn't realise the comment was made by a workflow this way. I believe the change in Zeebe is valid for all users of this action, so using GITHUB_TOKEN here doesn't make sense to me, but that's a different story.

I do think it is worth avoiding this risk by adding this simple check, or at the very least notifying users of this risk.

@github-actions
Copy link
Contributor

Only merged pull requests can be backported.

Copy link
Owner

@korthout korthout left a comment

Choose a reason for hiding this comment

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

You've convinced me that this doc can be an easy win.

However, I believe we should look towards better slash command handling using a dedicated action in the future.

Thanks for your proposal. Please consider these suggestions to clarify things in more detail.

remcowesterhoud and others added 2 commits June 22, 2022 15:13
Co-authored-by: Nico Korthout <[email protected]>
Co-authored-by: Nico Korthout <[email protected]>
@remcowesterhoud remcowesterhoud requested a review from korthout June 22, 2022 13:14
@remcowesterhoud
Copy link
Contributor Author

I've applied your suggestions. Please squash the commits when merging, or let me know and I'll squash them myself :D

Copy link
Owner

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Thanks @remcowesterhoud LGTM 📘

@korthout korthout merged commit 7a14174 into master Jun 22, 2022
@korthout korthout deleted the remcowesterhoud-patch-1 branch November 23, 2022 20:16
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.

2 participants