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

Fixed infra-860, bot did not delete previous comments #4

Merged
merged 1 commit into from
Apr 15, 2021
Merged

Conversation

bee-san
Copy link

@bee-san bee-san commented Apr 15, 2021

Approved in #2

See my comments on the original forked repo:
telia-oss#245 (comment)
Copied below:

Hey 👋🏻

I had a similar issue, specifically:

  • My user was a GitHub Bot App
  • It didn't delete comments
  • Using OAuth on a personal account it did.
    The issue occurs here:

https://github.com/telia-oss/github-pr-resource/blob/9ec47e2d9f28d13a4738ff48f2f40d2a570b251a/github.go#L398

When evaluating, it comes out to:

+ if "githubApp" == "githubApp[bot]"
- if e.Node.Author.Login == getComments.Viewer.Login {

Specifically, getComments.Viewer.Login returns the name with [bot] but e.Node.Author.Login returns it without bot.

We fixed this with an if statement checking for bots :-)

https://mondough.atlassian.net/secure/RapidBoard.jspa?rapidView=411&modal=detail&selectedIssue=INFRA-860

@bee-san bee-san changed the title Fixed infra-860 Fixed infra-860, bot did not delete previous comments Apr 15, 2021
@bee-san bee-san merged commit eca1832 into monzo-master Apr 15, 2021
@bee-san bee-san deleted the test branch April 15, 2021 12:53
bgandon pushed a commit to cloudfoundry-community/github-pr-resource that referenced this pull request May 11, 2024
Indeed, in order to delete the correct comments, the golang code makes sure
that comment's `Author.Login` is the same as current `Viewer.Login`. But with
bot users, viewer's login is "githubApp[bot]" and author login is "githubApp".

As a consequence, for the comparison to succeed, we need to remove any
trailing "[bot]" suffix from both parties, and compare the resulting prefixes.

This isue was originally submitted as telia-oss#245 and
fixed later in monzo#4.
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.

1 participant