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

fix: fix updating comments #1

Merged
merged 1 commit into from
Oct 23, 2024
Merged

fix: fix updating comments #1

merged 1 commit into from
Oct 23, 2024

Conversation

gmaclennan
Copy link
Member

@gmaclennan gmaclennan commented Oct 22, 2024

This action did not correctly update the comment with the lockfile changes if it was run with a custom token (e.g. not the default github action token).

Finding the comment made by the action matched the first line of the comment (comment header) and checked that the comment author had the login github-action[bot]. If this action is called with a custom token as input, then the comment is created by the user that owns the custom token (e.g. a Github App), so it did not find the existing comment and instead created a new comment.

This PR changes how an existing comment to update is found. It adds some hidden metadata to the comment (<!-- npm-lockfile-changes-action comment -->) and uses that to find an existing comment.

I looked into somehow matching the login name of the token owner to the comment author login, but I could not figure out the token permissions needed to read this data, and this seemed to add complexity. Using the hidden comment identifier seems to a robust solution which is fairly simple, and does not require the token to have any permissions other than PR write.

@gmaclennan gmaclennan closed this Oct 22, 2024
@gmaclennan gmaclennan reopened this Oct 22, 2024
Updating comments was broken when comments are created with a user other than the github-actions[bot] user.
Copy link

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM.

I wonder if we could submit this upstream, either as a PR or as an issue. (Maybe you already did this.)

@gmaclennan
Copy link
Member Author

I wonder if we could submit this upstream, either as a PR or as an issue

rvanvelzen#3

@gmaclennan gmaclennan merged commit 614dfc3 into main Oct 23, 2024
2 checks passed
@gmaclennan gmaclennan deleted the fix/update-comments branch October 23, 2024 08:47
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