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

Determine actor from PAT if possible #231

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Determine actor from PAT if possible #231

merged 1 commit into from
Jan 15, 2021

Conversation

Rylan12
Copy link
Contributor

@Rylan12 Rylan12 commented Nov 29, 2020

Hi there, Homebrew maintainer here. We were recently in the market for a new way to monitor stale issues (our existing configuration stopped working) and figured that this was a great solution. So far, we've only had one minor hiccup that I have aimed to fix in this PR.

This has been reported a few times before, but our issue revolves around the fact that the stale action checks for comments that are not made by a bot or context.actor (which is the user who most recently modified the workflow). I am aware of one attempt at removing this in #192. The response given in that PR makes sense to me:

The user who set up the schedule will be the "bot". In most cases this is desired, since the bot user will add issue labels and other comments on stale and we don't want those to count, even if it introduces the problem of excluding the created user.

This seems reasonable for smaller projects, but larger projects (like Homebrew) tend to have their own "bot user" that differs from the one who set up the workflow. In our specific case, I set up the stale workflow which made me the "actor". However, the repo-token we provide is for @BrewTestBot not for my user. That means that any actions taken are on the behalf of @BrewTestBot. Actions that I take should be treated no differently than those made by another user.


This PR attempts to fix this by trying to determine who the "actor" should be based on the repo-token. This will now run octokit.users.getAuthenticated(); and, if successful, will set the "actor" by accessing .data.login. If the API call fails, it will default back to context.actor as is currently done.

One potential thing to note is that it seems that using the default ${{ secrets.GITHUB_TOKEN }} does not have the appropriate permissions to run the API call. However, a PAT with no additional permissions does seem to have the appropriate scope to run the call. While this is not ideal, it is no worse than the current implementation because it will still fall back on context.actor. I would gladly accept any ideas about how to get around this limitation.


I think this is a reasonable compromise, but if it's not in the desired direction, I would, instead, propose adding a new option to the workflow to set an e.g. ignored-users option to the login of any users whose comments should be ignored when determining if new comments should "un-stale" an issue. This could default to context.actor. That way, projects like Homebrew can specify ignored-users: BrewTestBot and not worry about the context.actor issue.

@jonchang
Copy link

I believe the standard GITHUB_TOKEN has permissions tightly scoped to the repository that's using the token generation. And even if it could access the current user, it would only report that it was the github[bot] user, which isn't very illuminating anyway. So this approach is good IMO.

@hross
Copy link
Contributor

hross commented Jan 15, 2021

I like this approach. Seems reasonable and helpful in the case of bot users. I'm going to merge this to master and test it internally for a week or so before I publish a new release.

@hross hross merged commit a0b4b61 into actions:main Jan 15, 2021
@Rylan12 Rylan12 deleted the determine-actor-from-token branch January 15, 2021 15:06
@Rylan12
Copy link
Contributor Author

Rylan12 commented Jan 15, 2021

Awesome, thanks, @hross!

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.

3 participants