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

Proposal: Change TryBot failure score from -1 to -2 #37848

Open
sam3000 opened this issue Mar 13, 2020 · 7 comments
Open

Proposal: Change TryBot failure score from -1 to -2 #37848

sam3000 opened this issue Mar 13, 2020 · 7 comments
Labels
Milestone

Comments

@sam3000
Copy link

@sam3000 sam3000 commented Mar 13, 2020

What version of Go are you using (go version)?

n/a

Does this issue reproduce with the latest release?

n/a

What operating system and processor architecture are you using (go env)?

n/a

What did you do?

n/a

What did you expect to see?

go-review gerrits that show failure for the most recent TryBots run are not merged.

What did you see instead?

I observed that:

  1. Sometimes go-review gerrits are being merged despite the most recent TryBot test run showing failures.
  2. The same gerrit is then reverted with a reason stated as being that it causes TryBot to fail.

Comments

Examples can be provided on request (but I suspect that the go team are aware of the workflow issue described above).

I propose that TryBots be given -2 block merge privileges to ensure that the failing test build results are never overlooked. If needed for emergent purposes, a gerrit user with appropriate privileges would still be able to remove the -2 and submit regardless.

@gopherbot gopherbot added this to the Proposal milestone Mar 13, 2020
@gopherbot gopherbot added the Proposal label Mar 13, 2020
@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 13, 2020

We don't want to block changes like doc changes on a broken builder.
Sometimes the reason for trybot failures are known (e.g. linux-blah-blah as a full disk, request out to buildbot owner to fix) and need to be ignored.
But maybe someone that can override a -2 is enough. Not sure.

Can you point to examples of merges that had trybot failures? I don't want to single people out, but it may be instructive to look at what the trybot failures were like.

@sam3000
Copy link
Author

@sam3000 sam3000 commented Mar 13, 2020

Two recent examples that have failed trybots results:

https://go-review.googlesource.com/c/go/+/221603 cmd/compile/internal/syntax: faster and simpler source reader
https://go-review.googlesource.com/c/go/+/222241 Revert "cmd/compile/internal/syntax: faster and simpler source reader" (although this was ultimately abandoned in favour of a root cause fix).

And:
https://go-review.googlesource.com/c/go/+/222978 misc/spectre: add spectre index test
https://go-review.googlesource.com/c/go/+/223378 Revert "misc/spectre: add spectre index test"

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 13, 2020

Those two examples were both modified after the trybots failed.
If anything, those examples would argue that the rule should be that trybots must be run on the latest patch before submission is allowed.
Although I guess the rule proposed by the OP and by me have a lot of overlap. Either would have caught those two examples. My rule would catch in addition CLs which never got a trybot run.

@sam3000
Copy link
Author

@sam3000 sam3000 commented Mar 13, 2020

Those two examples were both modified after the trybots failed.
If anything, those examples would argue that the rule should be that trybots must be run on the latest patch before submission is allowed.
Although I guess the rule proposed by the OP and by me have a lot of overlap. Either would have caught those two examples. My rule would catch in addition CLs which never got a trybot run.

I didn't mean to be overly prescriptive in my suggestion. It was intended just to highlight that there appears to be a workflow problem where trybots errors /appear/ to be being overlooked or not revalidated after a patchset update.

I would suggest considering something that blocks on failure by default but is easily overridden when needed.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 14, 2020

Sometimes the reason for trybot failures are known (e.g. linux-blah-blah as a full disk, request out to buildbot owner to fix) and need to be ignored.

The TryBots generally all run on fresh VMs, so that's not usually an issue.

That said, it's also handy to be able to submit after fixing a couple of typos or comments and only re-building locally (to make sure the typo-fix didn't also introduce a stray edit elsewhere), without waiting on another TryBot run. (For example, see https://golang.org/cl/222277: I submitted PS16, which had the same code as the TryBot+1 PS14 but a few comment and commit-message changes.)

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 14, 2020

https://golang.org/cl/143023 was another recent example of a change with a TryBot -1 and subsequent break (the revert in https://golang.org/cl/220237 was abandoned after a trivial fix in https://golang.org/cl/220277 was submitted).

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 14, 2020

I notice that Gerrit has started to prompt when I hit the Submit button with unresolved comment threads.

Perhaps it could do the same for changes that lack a TryBot +1?

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.