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: sometimes does not remove wait-author #30172

Closed
FiloSottile opened this issue Feb 11, 2019 · 4 comments
Closed

x/build/cmd/gopherbot: sometimes does not remove wait-author #30172

FiloSottile opened this issue Feb 11, 2019 · 4 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

In https://go-review.googlesource.com/c/crypto/+/111335 it didn't remove the wait-author hashtag even if the author replied and uploaded new patch sets.

/cc @andybons @dmitshur
@FiloSottile FiloSottile added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 11, 2019
@gopherbot gopherbot added this to the Unreleased milestone Feb 11, 2019
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Feb 11, 2019
@dmitshur dmitshur changed the title x/build/cmd/gerritbot: sometimes does not remove wait-author x/build/cmd/gopherbot: sometimes does not remove wait-author Feb 11, 2019
@dmitshur dmitshur self-assigned this Feb 11, 2019
@dmitshur
Copy link
Contributor

dmitshur commented Feb 11, 2019 •
edited

It's a logic problem in gopherbot code related to how it does author comparisons, and how Gerrit sometimes represents authors as "Firstname Lastname <1234@...>", but other times as "Gerrit User 1234 <1234@...>".

We should compare Gerrit users by their Gerrit user ID only.


unwait CLs running on CL 111335
CL has tags: wait-author
CL has wait-author tag
waitAuthorIndex: 19
author: Andreas Auernhammer <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 11715 <11715@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 5976 <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 5976 <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 5976 <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 5440 <5440@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 11715 <11715@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 5976 <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 5976 <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
m.Commit.Author.Str: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
hasReplied: false

@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 Feb 11, 2019
@gopherbot
Copy link

Change https://golang.org/cl/161977 mentions this issue: cmd/gopherbot: compare Gerrit users by email only

@gopherbot
Copy link

Change https://golang.org/cl/161901 mentions this issue: cmd/gopherbot: accept if maintner metadata is ahead of Gerrit API

gopherbot pushed a commit to golang/build that referenced this issue Feb 14, 2019
@dmitshur
cmd/gopherbot: accept if maintner metadata is ahead of Gerrit API
05a16de 
The purpose of function onLatestCL is to check that maintner data
isn't behind Gerrit's data (as determined by talking to the Gerrit
REST API directly). However, there are some meta commits that the
Gerrit REST API doesn't return a corresponding message to, so
sometimes maintner data is ahead of "latest Gerrit message".

Since ahead is not _behind_, make onLatestCL report positively in
such situations by checking all cl.Metas, not just the latest one.
Do so in reverse order as an optimization (if there's a match, it's
most likely near the end of cl.Metas slice).

An example of such a meta commit is one that updates an uploaded
patch set and sets the Reviewer field:

	$ git show fc20e3671f7b1ff889d5f947b11d7fda0b608751
	commit fc20e3671f7b1ff889d5f947b11d7fda0b608751 (meta/35/111335/meta)
	Author: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>
	Date:   Mon Feb 11 17:49:09 2019 +0000

	    Update patch set 6

	    Patch-set: 6
	    Reviewer: Gerrit User 14805 <14805@62eb7196-b449-3ce5-99f1-c037f21e1705>

It has no corresponding message with same ID in the output from a Gerrit
REST API call at
https://go-review.googlesource.com/changes/111335/detail?o=MESSAGES,
nor does it show up visually in the Gerrit UI at
https://go-review.googlesource.com/c/crypto/+/111335.

Before this change (with fix in CL 161977):

	2019/02/11 15:02:33 Reloaded data from log *maintner.netMutSource.
	2019/02/11 15:02:34 https://golang.org/cl/111335 -- remove wait-author; reply from Andreas Auernhammer <...@mail.de>
	2019/02/11 15:02:35 onLatestCL: maintner metadata for CL 111335 is behind; skipping action for now.

After this change:

	2019/02/11 15:06:46 Reloaded data from log *maintner.netMutSource.
	2019/02/11 15:06:46 https://golang.org/cl/111335 -- remove wait-author; reply from Andreas Auernhammer <...@mail.de>
	2019/02/11 15:06:46 [dry run] would remove hashtag 'wait-author' from CL 111335

Updates golang/go#30172

Change-Id: I1575ccffbf8c3edb337f55bf1bf2c96a4a04bbdd
Reviewed-on: https://go-review.googlesource.com/c/161901
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@dmitshur
Copy link
Contributor

The fix is deployed now, and it's working as intended. See here.

@golang golang locked and limited conversation to collaborators Feb 14, 2020
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
3 participants
@FiloSottile @dmitshur @gopherbot