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

Jump to bottom

x/build/cmd/gopherbot: mark Gerrit comments as CI-generated #39828

Closed
FiloSottile opened this issue Jun 24, 2020 · 7 comments
Closed

x/build/cmd/gopherbot: mark Gerrit comments as CI-generated #39828

FiloSottile opened this issue Jun 24, 2020 · 7 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@FiloSottile
Copy link
Contributor

From https://www.gerritcodereview.com/2020-05-06-change-log-experiment.html

All human messages are shown, but for autogenerated messages they are only shown on one patchset. If for example a CI system posts on patchsets 3, 4, 5, 6, 7, and a Linter posts on patchsets 3, 5, 6, then the CI messages are only shown on ps 7, and the Linter messages are only shown on ps 6.

That sounds great!

However, as far as I can tell, Gobot Gobot is just pretending to be a human account, so it does not benefit from this logic. It'd be nice to port it to whatever system it needs to use to be recognized as a CI tool.

It might even have better security properties than a GSuite account.

/cc @golang/osp-team
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Jun 24, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jun 24, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 24, 2020
@dmitshur
Copy link
Contributor

This may have been resolved by placing the Go Bot account (and Gerrit Bot) in a "Service Users" group for Gerrit's Attention Set feature (documented here), see internal issue b/171883377 for details.

Do you think there's more left to do here?

@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 19, 2021
@FiloSottile
Copy link
Contributor Author

I think the "Service Users" group simply exempts Go Bot from attention set logic.

I still see Go Bot entries on older Patch Sets. I think this change would significantly reduce visual clutter.

Finally, moving Go Bot to a service account from a GSuite account sounds like a nice cleanup.

@FiloSottile FiloSottile removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 19, 2021
@FiloSottile
Copy link
Contributor Author

Go Bot's messages are still shown for all past Patch Sets.

image

This is nearly undocumented, but what we need to do is tag those messages with a tag with an autogenerated: prefix.

https://groups.google.com/g/repo-discuss/c/JdTcZSeg-o4

@FiloSottile
Copy link
Contributor Author

Ah, found the right docs. Sending a CL.

https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#review-input

@gopherbot
Copy link

Change https://golang.org/cl/310011 mentions this issue: cmd/coordinator: tag TryBot comments on Gerrit as autogenerated

@gopherbot
Copy link

Change https://golang.org/cl/310015 mentions this issue: cmd/coordinator: post TryBot status as PatchSet comments

gopherbot pushed a commit to golang/build that referenced this issue Apr 16, 2021
@FiloSottile
cmd/coordinator: post TryBot status as PatchSet comments
c328b27 
It's nice to have a thread for the results because it starts unresolved,
it gets marked resolved automatically if TryBots are happy, and it can
be used to discuss the failure.

Dropped the duplicate check on the beginning message because it's useful
to know the TryBots have started again when dropping TryBotResult.

Updates golang/go#39828

Change-Id: I66e8edec6dee10e8e1df2d2a6b501774ef373496
Reviewed-on: https://go-review.googlesource.com/c/build/+/310015
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
Trust: Alexander Rakoczy <alex@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 20, 2021
@gopherbot
Copy link

Change https://golang.org/cl/318129 mentions this issue: cmd/coordinator: correctly thread resolving comments

gopherbot pushed a commit to golang/build that referenced this issue May 8, 2021
@FiloSottile
cmd/coordinator: correctly thread resolving comments
7630227 
Third time's the charm.

For golang/go#39828.

Change-Id: I3eeb17b3c64bbcf27d2ce5e1a4a9207d5daf50d8
Reviewed-on: https://go-review.googlesource.com/c/build/+/318129
Trust: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@cagedmantis @FiloSottile @dmitshur @gopherbot