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

DangerJS in GitHub actions does not overwrite or delete previous comments #936

Closed
jmenestr opened this issue Oct 26, 2019 · 2 comments · Fixed by #1013
Closed

DangerJS in GitHub actions does not overwrite or delete previous comments #936

jmenestr opened this issue Oct 26, 2019 · 2 comments · Fixed by #1013

Comments

@jmenestr
Copy link
Contributor

Like in other CI workflows, I would expect any dangerjs code run in github actions to correctly update/delete previous comments made by previous builds. This currently does not happen, resulting in a new comment being created for every run.

While not functionally blocking, it create a rather poor experience because it results in a lot of noise.

I think I've identified the issue at these lines:
https://github.com/danger/danger-js/blob/master/source/platforms/github/GitHubAPI.ts#L133-L136
It seems that the userID is hardcoded for github actions and always returns the same value rather than looking up the user associated with the current GITHUB_TOKEN.

As a result of the hardcoded id, when the API goes to look for previous comments that are created by danger to update/delete them, there's a check on the userid in a filter and because the hard coded will probably never match the actual userid, it never gets a list of comments to actually update/delete.

https://github.com/danger/danger-js/blob/master/source/platforms/github/GitHubAPI.ts#L85-L96

I'm mot sure if this behavior was intentional, or if it was just something that was missed.

@jmenestr
Copy link
Contributor Author

PR is up at #935

@fbartho
Copy link
Member

fbartho commented Oct 26, 2019

@jmenestr, my team isn’t seeing this behavior, and we are relatively up to date, and use danger with GitHub actions.

I guess if we’re missing something deeper?

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 a pull request may close this issue.

2 participants