Skip to content

feat: update bot comment when Jenkins finishes #259

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

Closed
wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

Note: this is a rough draft of a implementation of the "Edit Comment" feature described in #251. I haven't set up my machine to test the bot yet, so this code is completely untested at this point, but it reflects what I'm thinking.

I'm looking for comments on the flow instead of the code (since the code wasn't tested yet). The flow is as follow:

  • When a Jenkins job starts, the bot will leave a comment on the respective PR
    • The comment will have two placeholders created using html comments
      • One placeholder next to the CI url, for the build status
      • One placeholder below the CI url, for a summary of failures when the build fails
    • The placeholders will hold the build ID so that the bot can identify the respective comment later
  • When a Jenkins job finishes, the bot will:
    • Fetch all comments for the respective PR
    • Find the respective comment (if it can't find, skip the next steps)
    • Replace the first placeholder with the build status
    • If the build failed:
      • Run ncu-ci to get a summary of changes, in markdown format
      • Wrap the output with <details> tag.
      • Replace the second placeholder with the wrapped output
    • The bot updates the comment using GitHub API

The result from this workflow will have minimum visual impact on Pull Requests while also adding a rich summary of failures, and will allow us to search for flaky tests easier (if a test failed on multiple PRs and we search for that test on the PRs tab, all PRs where the test failed will show up, making it easier to determine if a test is flaky or not).

There are some requirements for this workflow to work. We need to change our post-build-status-update on Jenkins to publish the build number as well. We also need to ensure ncu-ci can consume the credentials via environment variables (I don't think it can today). As a bonus, if ncu-ci could expose an API to get the markdown results of ncu-ci that'd be great, as we wouldn't need to spawn another process to do that, but I don't think it'll be possible in the near future.


Update the bot comment on a PR when the corresponding Jenkins build
finishes. The updated comment will have the build status, and if the
build failed, it will have a summary of failures.

To identify a comment corresponding to a PR, we leave placeholders on
the comment as HTML commnets, so those placeholders can be replaced
later.

Signed-off-by: Matheus Marchini [email protected]

Update the bot comment on a PR when the corresponding Jenkins build
finishes. The updated comment will have the build status, and if the
build failed, it will have a summary of failures.

To identify a comment corresponding to a PR, we leave placeholders on
the comment as HTML commnets, so those placeholders can be replaced
later.

Signed-off-by: Matheus Marchini <[email protected]>
@mmarchini mmarchini marked this pull request as draft April 16, 2020 23:43
@mmarchini
Copy link
Contributor Author

cc @phillipj

@phillipj
Copy link
Member

Sorry for the late reply on this @mmarchini! Been wanting to have a looksie, but pretty chaotic lately at home due to covid19 & having to kindergarten my kids most of the day..

I'll get to review this as soon as I can.

@phillipj
Copy link
Member

I like what your suggesting here 👍

In case you're not aware, we did edit comment in a short periode in 2019. Back then it was mostly to reduce the amount of noise the github bot brings to the PRs in general. There were split opinions amongst core collaborators whether or not that change was good or bad -- some appreciate new comments from the github bot on builds as a ping, others don't. It also tripped some node-core-utils tools which we didn't consider beforehand. Ended up getting reverted in #240.

You're also aiming for only one comment to be posted (and then edited) from the github bot, right? Or a mix where the first github bot comment seen in a PR contains "last status" from the latest job, in addition to several individual comments representing each and every job that has been kicked off related to that PR that does not get edited?

will allow us to search for flaky tests easier

Sounds really valuable if we could utilise these comments for this purpose!

Have you had the chance to share and get feedback about this with core collaborators yet? As hinted at before, although well intentioned, there might downsides we are not considering. Would be a shame if we ended up having to revert this effort like we did with the last edit comment experiment 😬

@mmarchini
Copy link
Contributor Author

You're also aiming for only one comment to be posted (and then edited) from the github bot, right?

No, it will search for the appropriate comment for that particular CI run.

Have you had the chance to share and get feedback about this with core collaborators yet?

Yes, no objections so far: nodejs/node#32306

I'm working on something else now, but I didn't forget this. I'll try to get back to it at some point, maybe breaking this PR into smaller pieces.

@phillipj
Copy link
Member

Cool! 👍

Heads up about this PR that changes quite a few files that I'm assuming might be affected by what you're working on: #258. Planning to land that soon-ish.

@Trott
Copy link
Member

Trott commented Aug 25, 2022

I'm going to close this as stagnant but by all means, re-open if anyone plans on picking this up.

@Trott Trott closed this Aug 25, 2022
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