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

x/build: make a bot review CLs for common mistakes (like Tricorder) #18548

Open
bradfitz opened this Issue Jan 6, 2017 · 61 comments

Comments

Projects
None yet
@bradfitz
Member

bradfitz commented Jan 6, 2017

Tracking bug to write a bot to review CLs for common mistakes:

  • properly-formatted references to CLs and bugs
  • conventional subject ("pkg/name: lowercase verb, not period at end")
  • gofmt

And whatever else we add in the future.

(forked from email thread https://groups.google.com/forum/#!topic/golang-dev/KpHMoePhg6c)

/cc @kevinburke @rsc

@bradfitz bradfitz added this to the Unreleased milestone Jan 6, 2017

@dsnet

This comment has been minimized.

Member

dsnet commented Jan 6, 2017

What about lint and vet?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 6, 2017

What about lint and vet?

Sure. I imagine we'd start with some basic things and once we have it automated, we can add more to it later. I wouldn't make lint & vet a requirement for the initial deployment.

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Jan 6, 2017

IIRC @josharian has been working on getting golang/go to pass vet, but it doesn't currently, and relies on an "exemption" file for places/test files that don't pass vet on purpose.

@mvdan

This comment has been minimized.

Member

mvdan commented Jan 6, 2017

gofmt

Perhaps with -s?

properly-formatted references to CLs and bugs

Github has various accepted formats for these (https://help.github.com/articles/closing-issues-via-commit-messages/). I assume you want to limit it to "Fixes #n", though?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 6, 2017

@mvdan, the two common mistakes I'm thinking of:

  1. people write "Fixes #nnn" when they're hacking in a subrepo, and thus Github requires the fully-qualifed "Fixes golang/go#nnn" format, since Go only uses one massive bug tracker, instead of per-repo bug trackers.
  2. people sometimes accidentally refer to CL numbers with the # sign, spamming ancient bugs. They should write "CL nnnn" or golang.org/cl/nnnn instead of "CL #nnn" or "change #nnn".
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jan 6, 2017

Another common mistake is using a git SHA when they should use a CL number.

@mvdan

This comment has been minimized.

Member

mvdan commented Jan 6, 2017

Another thing that comes to mind is gerrit/github links that should be golang.org/issue/X or golang.org/cl/Y.

@minux

This comment has been minimized.

Member

minux commented Jan 6, 2017

@minux

This comment has been minimized.

Member

minux commented Jan 6, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 6, 2017

I also remember one time we rewrote the git history to
remove one accidentally committed binary file.

I have no memory of this and am pretty sure we have never rewritten the Git history.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jan 6, 2017

We rewrote people's memory along with the git history, to simplify matters going forward.

@minux

This comment has been minimized.

Member

minux commented Jan 6, 2017

@LionNatsu

This comment has been minimized.

Contributor

LionNatsu commented Jan 6, 2017

Maybe we can add some pre-check in git-codereview? After change and before mail

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Jan 6, 2017

@LionNatsu from the email thread:

We could have a bot that leaves comments on Gerrit for common mistakes. I'd prefer that over CLI tooling. (Not everybody uses git-codereview, especially if we start accepting Github PRs)

@LionNatsu

This comment has been minimized.

Contributor

LionNatsu commented Jan 6, 2017

@kevinburke Oh, indeed.

@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

I'm interested in working on this (I wrote Go Report Card https://goreportcard.com/ with @hermanschaaf which does a lot of this too).

Edit: the Go Report Card reference was just to mention that I've worked on a similar problem before and could maybe borrow some code from there.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 7, 2017

@shawnps, the first thing to figure out is where this runs and how, and how it interacts with Gerrit.

We already have one process polling Gerrit (the build coordinator's "watcher"), and if we revamp dev.golang.org that would be a second. This would be a third. I suspect we'll want to make this bot interact with the proposed dev.golang.org all-Github-and-Gerrit-in-RAM server, and then this bot could long-poll our new "Github/Gerrit Model Server" waiting for changes (new or updated CLs).

But in the meantime I suppose it could run manually every N minutes. Please use https://godoc.org/golang.org/x/build/gerrit which we already use. If something's missing, modify that package as needed.

You'll want to store state somewhere. It's easy to find CLs the bot's already left comments on, but the bot should probably be silent and not +1 the bug if there's nothing to say, which means you'll need to store state about which bugs you've seen and are happy about. That should probably go into Google Cloud Datastore (https://cloud.google.com/datastore/) which we already use in the Go build system (see "git grep datastore" in src/golang.org/x/build/cmd/coordinator), as opposed to files on the local filesystem. (Filesystems come and go as we destroy and recreate the world)

@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

@bradfitz thanks, I was just about to ask about many of the details you just listed. Taking a look at https://godoc.org/golang.org/x/build/gerrit now.

That should probably go into Google Cloud Datastore (https://cloud.google.com/datastore/) which we already use in the Go build system (see "git grep datastore" in src/golang.org/x/build/cmd/coordinator)

Found datastore reference here:

https://github.com/golang/build/blob/master/cmd/coordinator/gce.go#L119

So this particular app (coordinator) "runs on CoreOS on Google Compute Engine and manages builds using Docker containers and/or VMs as needed" - is the suggestion that the new bot would run on GCE or GAE in a similar manner to this one?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 7, 2017

Yes, this would run on GCE somewhere.

I would start by making it a non-package main package that you can instantiate and run. Then make a tiny tiny package main runner program for it for development and local testing.

Once it's working, we can integrate that bot into something.

I also imagine that some helper environment will need to be described by a Dockerfile, since you'll want things like golint and go vet available.

@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

Okay thanks. As a first step I'm currently trying to figure out how to grab files for a specific change on Gerrit in order to run gofmt on them.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 7, 2017

Okay thanks. As a first step I'm currently trying to figure out how to grab files for a specific change on Gerrit in order to run gofmt on them.

Click "Download" in the web UI for a hint:

git fetch https://go.googlesource.com/crypto refs/changes/79/34779/8 && git cherry-pick FETCH_HEAD

You can have the bot do a shallow git clone of the parent git commit, and then chrery-pick atop that.

Alternatively, there's an API to get files from Gerrit at a specific revision as a tarball. See the cmd/coordinator code:

func getSourceTgzFromGerrit(repo, rev string) (tgz []byte, err error) {
        return getSourceTgzFromURL("gerrit", repo, rev, "https://go.googlesource.com/"+repo+"/+archive/"+rev+".tar.gz")
}
@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

@bradfitz thanks, I had gotten as far as getting the FetchInfo:

package main

import (
	"fmt"
	"log"

	"golang.org/x/build/gerrit"
)

func main() {
	c := gerrit.NewClient("https://go-review.googlesource.com", gerrit.NoAuth)
	cis, err := c.QueryChanges("34871", gerrit.QueryChangesOpt{
		N: 5000,
		Fields: []string{
			"CURRENT_FILES",
			"CURRENT_REVISION",
		},
	})
	if err != nil {
		log.Fatal(err)
	}
	for _, r := range cis[0].Revisions {
		for _, f := range r.Fetch {
			fmt.Println(f)
		}
	}
}

(I know the cis[0] could panic if nothing is returned, this was just a quick test to see how far I could get right now)

I guess I can use this to get this piece: refs/changes/79/34779/8 and then do the cloning bit as you mentioned.

@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

@bradfitz let me know if that looks like I'm on the right track, will be offline soon but I'll pick this back up in the morning. I appreciate all the feedback and help, thanks!

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 7, 2017

Rather than do a query for "34871", you probably just want to load the detail of a single change using https://godoc.org/golang.org/x/build/gerrit#Client.GetChangeDetail

@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

@bradfitz any idea if there's a reason why GetChangeDetail doesn't take an optional gerrit.QueryChangesOpt? I was planning to go the tar.gz route but that requires the revision, which doesn't seem to be accessible unless you specify "CURRENT_REVISION".

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 7, 2017

IIRC, GetChangeDetail returns everything already, so there's no need to selectively say what you want?

If not, we can modify that package as needed.

@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

@bradfitz I will dig a bit more but I think I need the current revision in order to get the tarball, and current revision only appears in ChangeInfo if you request it:

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

see "current_revision":

"Only set if the current revision is requested or if all revisions are requested."

But maybe this is my misunderstanding of how revisions work in Gerrit.

@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

@bradfitz I was able to get the revision info with this patch:

diff --git a/gerrit/gerrit.go b/gerrit/gerrit.go
index fbdc7a8..b983d90 100644
--- a/gerrit/gerrit.go
+++ b/gerrit/gerrit.go
@@ -345,9 +345,19 @@ func (c *Client) QueryChanges(q string, opts ...QueryChangesOpt) ([]*ChangeInfo,
 // GetChangeDetail retrieves a change with labels, detailed labels, detailed
 // accounts, and messages.
 // For the API call, see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-change-detail
-func (c *Client) GetChangeDetail(changeID string) (*ChangeInfo, error) {
+func (c *Client) GetChangeDetail(changeID string, opts ...QueryChangesOpt) (*ChangeInfo, error) {
+	var opt QueryChangesOpt
+	switch len(opts) {
+	case 0:
+	case 1:
+		opt = opts[0]
+	default:
+		return nil, errors.New("only 1 option struct supported")
+	}
 	var change ChangeInfo
-	err := c.do(&change, "GET", "/changes/"+changeID+"/detail")
+	err := c.do(&change, "GET", "/changes/"+changeID+"/detail", urlValues{
+		"o": opt.Fields,
+	})
 	if err != nil {
 		return nil, err
 	}

without this, I've found ci.Revisions to be empty

@shawnps

This comment has been minimized.

Member

shawnps commented Jan 7, 2017

CL made here for the above diff: https://go-review.googlesource.com/#/c/34922/

@shawnps

This comment has been minimized.

Member

shawnps commented Mar 22, 2017

@bradfitz sounds good, so it looks like I would define methods on gopherbot?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 22, 2017

Yup. Put it in a new file, like codereview.go. For now don't try to make it automatic. I have plans for that later. For now just make it a command line option, like:

$ gopherbot --review=1234

And make it review the current version of 1234.

So the method would be like:

func (b *gopherbot) reviewGerritCL(num int) error { ... }
@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 22, 2017

(or just review.go, since codereview.go sounds like the name of the git-codereview tool)

@shawnps

This comment has been minimized.

Member

shawnps commented Mar 22, 2017

@bradfitz I'm reusing getSourceTgzFromGerrit from cmd/coordinator/coordinator.go, should that be exported, put into an internal package, or just copied?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 22, 2017

Just copy it for now.

@shawnps

This comment has been minimized.

Member

shawnps commented Mar 22, 2017

@bradfitz I've been doing some manual testing trying to find a CL where one or more files weren't gofmted. It seems like a lot of files in go/test are intentionally not gofmted for testing purposes. Should we ignore that dir when checking gofmt?

@josharian

This comment has been minimized.

Contributor

josharian commented Mar 22, 2017

@shawnps for now, maybe match git-codereview? See https://github.com/golang/review/blob/master/git-codereview/gofmt.go#L343. Yet another reason to consider having git-codereview and this bot use a common package.

@shawnps

This comment has been minimized.

Member

shawnps commented Mar 22, 2017

I have a CL here:

https://go-review.googlesource.com/c/38285/

It's difficult to test locally. I had to comment a lot of stuff out. First I had this error:

2017/03/22 15:32:20 open /Users/shawn/keys/github-gobot: no such file or directory

But adding a GitHub personal access token there didn't help. I commented out GitHub client instantiation and got this panic:

2017/03/22 15:33:40 Reloading data from log *maintner.DiskMutationLogger ...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x1266ba0]

goroutine 5 [running]:
golang.org/x/build/maintner.(*DiskMutationLogger).GetMutations.func1.1(0xc4200c2ba0, 0x1a, 0x0, 0x0, 0x14bd640, 0xc4200e4030, 0x30, 0x1331080)
	/Users/shawn/mygo/src/golang.org/x/build/maintner/logger.go:89 +0x70
path/filepath.Walk(0xc4200c2ba0, 0x1a, 0xc4200e4000, 0x0, 0x0)
	/Users/shawn/go/src/path/filepath/path.go:396 +0x8f
golang.org/x/build/maintner.(*DiskMutationLogger).GetMutations.func1(0xc4200c2ba0, 0x1a, 0xc42001a5a0, 0x14c2780, 0xc4200182b8)
	/Users/shawn/mygo/src/golang.org/x/build/maintner/logger.go:138 +0xca
created by golang.org/x/build/maintner.(*DiskMutationLogger).GetMutations
	/Users/shawn/mygo/src/golang.org/x/build/maintner/logger.go:143 +0xf1

So I added a check for if fi != nil and the code runs. If anyone has suggestions on how to do this properly on my local please let me know.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 22, 2017

Don't get distracted on a refactoring mission right now. If you want code, copy it. Leave TODOs sprinkled around where you think things can be shared in the future.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 22, 2017

Try just mkdir $HOME/var/maintner for now. I have a fix coming for that crash.

And if you want the github/git data, here's a snapshot of the maintner data to put in that directory:
https://storage.googleapis.com/go-builder-data/maintner-2017-03-22.proto.gz

But that'll currently add a few seconds start-up time to your program, which might be annoying for development. (loading isn't optimized yet)

@shawnps

This comment has been minimized.

Member

shawnps commented Mar 22, 2017

@bradfitz thanks, that worked (the dir is var/maintnerd with a trailing d though) along with using the -dry-run flag to skip the GitHub client instantiation stuff.

@hirochachacha

This comment has been minimized.

Contributor

hirochachacha commented Dec 15, 2017

Any updates?

@bradfitz bradfitz changed the title from tools: make a bot review CLs for common mistakes to x/build: make a bot review CLs for common mistakes (like Tricorder) Feb 14, 2018

@gopherbot gopherbot added the Builders label Feb 14, 2018

@bradfitz bradfitz added the NeedsFix label Feb 15, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 15, 2018

We had a long-running dup bug that I just closed, but contains discussion and links: #20648

@gopherbot

This comment has been minimized.

gopherbot commented May 30, 2018

Change https://golang.org/cl/115356 mentions this issue: os/exec: gofmt

gopherbot pushed a commit that referenced this issue May 30, 2018

os/exec: gofmt
CL 109361 introduced some changes which were not properly gofmt'ed.
Because the CL was sent via Github no gofmt checks were performed
on it (cf. #24946, #18548).

Change-Id: I207065f01161044c420e272f4fd112e0a59be259
Reviewed-on: https://go-review.googlesource.com/115356
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@bradfitz bradfitz assigned dmitshur and unassigned bradfitz and andybons Aug 3, 2018

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