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

GitHub Comment Coalescing #47

Closed
grahamc opened this Issue Jan 29, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@grahamc
Member

grahamc commented Jan 29, 2018

Ofborg is growing a nice feature set, and with it more things to say on every issue. With two-to-three comments per issue, I think we're at risk of sending too much noise.

One solution is to coalesce replies in to a single comment. This would ideally be one comment per call, not one comment per issue. In other words, if there are two calls on the issue:

image

this would result in two comments, one per call.

what I'd do once we have this

  • Update the issue to say the commands were received and where they will run
  • Update the issue when a build starts running (already can be known via the logs exchange)
  • Provide a link to the log viewer

down sides

  • GitHub doesn't properly propagate comment updates sometimes when the page is already loaded, so maybe people will feel they aren't getting feedback because they don't see anything changing
  • GitHub's comments isn't a transactional database and so if there are multiple poster jobs running simultaneously, comments could be easily lost. Additionally, updates could easily be lost if there is replication lag or cache problems on GitHub.

implementation details

  • when a job is scheduled in the githubcommentfilter job, we should generate a call_id and pass that to the build job
  • the builder should include call_id in the BuildLogStart message
  • when a build is finished, the builder should include the call_id in the BuildResult message
  • the poster job should be rewritten in Rust first
  • the poster job should include the call ID in each comment it leaves
  • the poster job should look for an existing comment with the call ID and append its message to the end
@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus Feb 16, 2018

Just a random idea here: Would it be possible to use PR status for this. Triggering a build would just add new tests with their status to the PR. You could even create entries that remain 'pending' while not triggered by an authorized user. The message of these tests could even be an url to trigger the build, so the messages "@GrahamcOfBorg XXX" would also be removed from the conversation.

layus commented Feb 16, 2018

Just a random idea here: Would it be possible to use PR status for this. Triggering a build would just add new tests with their status to the PR. You could even create entries that remain 'pending' while not triggered by an authorized user. The message of these tests could even be an url to trigger the build, so the messages "@GrahamcOfBorg XXX" would also be removed from the conversation.

@7c6f434c

This comment has been minimized.

Show comment
Hide comment
@7c6f434c

7c6f434c Feb 20, 2018

Member

Is there any API for removing comments from the discussion that allows only removing own comments? If yes, maybe build could post a new comment that has all the previous builds in a collapsed section, and remove the old comments? If no, well, playing with comment removal is scary without a safety net.

Member

7c6f434c commented Feb 20, 2018

Is there any API for removing comments from the discussion that allows only removing own comments? If yes, maybe build could post a new comment that has all the previous builds in a collapsed section, and remove the old comments? If no, well, playing with comment removal is scary without a safety net.

@grahamc

This comment has been minimized.

Show comment
Hide comment
@grahamc

grahamc Mar 19, 2018

Member

I'm not going to do this.

Member

grahamc commented Mar 19, 2018

I'm not going to do this.

@grahamc grahamc closed this Mar 19, 2018

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat May 8, 2018

Member

The comments might now be movable to the new "checks" tab, separating the noise from actual discussion: https://help.github.com/articles/about-status-checks/#checks

Member

vcunat commented May 8, 2018

The comments might now be movable to the new "checks" tab, separating the noise from actual discussion: https://help.github.com/articles/about-status-checks/#checks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment