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

[Feature Request] Does not overwrite PR comment that from other PR job #2695

Closed
kumada626 opened this issue Apr 12, 2022 · 3 comments
Closed
Labels

Comments

@kumada626
Copy link
Contributor

What happened:
If there are multiple PR builds and they are making PR comments to each other, the PR comment will be overwritten by jobs that finish later.

What you expected to happen:
PR comment only overwrite comment that from the same PR job.

How to reproduce it:

  1. Define multiple PR jobs and set meta.summary in the build to each other.
  2. Make a Pull Request and start PR builds.
  3. Only single PR comment(overwritten by jobs that finish later).
@kumada626 kumada626 changed the title [Feature Request] Do not overwrite PR comment that from other PR job [Feature Request] Does not overwrite PR comment that from other PR job Apr 12, 2022
@tkyi tkyi added the feature label Apr 20, 2022
@ibu1224
Copy link
Contributor

ibu1224 commented May 11, 2022

@jithine @tkyi @kumada626 and everyone

I have been trying to think of ways to add this feature.
Please give me feedback. Once consent is obtained, the development can be done.

The current PR comments work as follows. (addPrComment)

  • Communicate with GitHub to retrieve PR comments.
  • If the PR comment exists and has been written by a bot, overwrite the comment. If not, create a new PR comment.

In other words, if there is a bot comment on the PR, it is overwritten.

Therefore, to create PR comments in multiple PR builds, it is necessary to determine which job the PR comments were written for.

Here is the solution I have come up with.

Solution 1. Add jobId to PR comments.

Add the jobId to PR comments, and when a comment is retrieved from GitHub, parse the contents of the comment to determine which job wrote it.

add_jobId

Modify the following sections.

  • Prepare jobId
  • Parse the jobId in the PR comment.
    • We already have the ability to retrieve PR Comments from GitHub.prComments
    • The body of the PR comments obtained is parsed to determine if the jobId matches.
      • If the jobId matches, you can overwrite it, otherwise you can create a new one.
      • The function to overwrite and the part to make new comments have already been implemented.
  • Write jobId in PR comments.
    • PR comments are formatted in getPrComment and sent to addPrComment.
      • Pass the jobId to getPrcomment and include the jobId in the comment.

Solution 2. Use the Buildid written in the PR comment to get the jobId from the DB.

Same as Solution 1. making the jobId available in addPrComment. The difference is that the jobId is retrieved from the DB using buildId.

Modify the following sections.

  • Prepare jobId
    • Please check Solution 1.
  • Parse buildId from PR comments.
    • The buildId is always written in the PR comments. The set point is getPrComment.
    • Parse the body of the PR comment to obtain the buildId.
  • Use buildFactory to query the DB.
    • You can get the jobId by using buildFactory.get(<buildId>) .
      • Note that in addPrComment, you must import or pass an instance to use buildFactory.
  • If the jobId obtained matches the job currently being executed, overwrite it; if not, make a new comment.

I prefer to develop with solution 1.
The reason is that 2 would require more communication to the DB to match the number of PR comments. This leads to load.

Reference

PR Comment is created with the following source code flow

API(/PUT/builds)
-> model.update()
-> model.updateCommitStatus()
-> scm.addPrComment()

@ibu1224
Copy link
Contributor

ibu1224 commented May 11, 2022

I received a feedback within YJ.
I share solution 3, which is an improvement on solution 1.

Solution 3. Add jobName to PR comments.

The difference from Solution 1 is the use of jobName instead of jobId.
Users are happy to see jobNamerather than jobId.
jobname

Modify the following, which is almost the same as `Solution 1.`
  • Prepare jobName
  • Parse the jobName in the PR comment.
    • We already have the ability to retrieve PR Comments from GitHub.prComments
    • The body of the PR comments obtained is parsed to determine if the jobName matches.
      • If the jobName matches, you can overwrite it, otherwise you can create a new one.
      • The function to overwrite and the part to make new comments have already been implemented.
  • Write jobName in PR comments.
    • PR comments are formatted in getPrComment and sent to addPrComment.
      • Pass the jobName to getPrcomment and include the jobName in the comment.

I think solution3 is good.

@jithine
Copy link
Member

jithine commented May 20, 2022

I agree with solution 3 proposal 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants