gerrit: support contexts
Switch all API methods that make requests to Gerrit to take a
context.Context as their first argument. Adds a package example and
a simple test that we make requests to the correct endpoint and that
the Client can handle correct responses and error responses from the
Gerrit server.
Switches all code in the x/build tree to use the new Gerrit
client. There are several projects outside the tree that import
x/build/gerrit; I'll submit CL's against those to pull in the new
interface once this gets merged.
Documents that the API is unstable.
Fixes golang/go#18742.
Fixes golang/go#18743.
Change-Id: Ifa78cbb058981e23cf5769955f6312fcbe08e174
Reviewed-on: https://go-review.googlesource.com/35559
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/cmd/cl/cl.go b/cmd/cl/cl.go
index fadb762..fefaef7 100644
--- a/cmd/cl/cl.go
+++ b/cmd/cl/cl.go
@@ -81,6 +81,7 @@
import (
"bytes"
+ "context"
"encoding/json"
"flag"
"fmt"
@@ -131,7 +132,9 @@
if strings.Contains(query, " is:") || strings.HasPrefix(query, "is:") {
open = ""
}
- cis, err := c.QueryChanges(open+" -project:scratch -message:do-not-review "+query, gerrit.QueryChangesOpt{
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ cis, err := c.QueryChanges(ctx, open+" -project:scratch -message:do-not-review "+query, gerrit.QueryChangesOpt{
N: 5000,
Fields: []string{
"LABELS",
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
index 88a661a..1cf0c1a 100644
--- a/cmd/coordinator/coordinator.go
+++ b/cmd/coordinator/coordinator.go
@@ -15,6 +15,7 @@
"archive/tar"
"bytes"
"compress/gzip"
+ "context"
"crypto/rand"
"crypto/sha1"
"crypto/tls"
@@ -50,7 +51,6 @@
"golang.org/x/build/internal/singleflight"
"golang.org/x/build/livelog"
"golang.org/x/build/types"
- "golang.org/x/net/context"
"golang.org/x/time/rate"
)
@@ -788,7 +788,7 @@
if inStaging && true {
return nil
}
- cis, err := gerritClient.QueryChanges("label:Run-TryBot=1 label:TryBot-Result=0 status:open", gerrit.QueryChangesOpt{
+ cis, err := gerritClient.QueryChanges(context.Background(), "label:Run-TryBot=1 label:TryBot-Result=0 status:open", gerrit.QueryChangesOpt{
Fields: []string{"CURRENT_REVISION"},
})
if err != nil {
@@ -966,7 +966,8 @@
func (ts *trySet) notifyStarting() {
msg := "TryBots beginning. Status page: http://farmer.golang.org/try?commit=" + ts.Commit[:8]
- if ci, err := gerritClient.GetChangeDetail(ts.ChangeTriple()); err == nil {
+ ctx := context.Background()
+ if ci, err := gerritClient.GetChangeDetail(ctx, ts.ChangeTriple()); err == nil {
if len(ci.Messages) == 0 {
log.Printf("No Gerrit comments retrieved on %v", ts.ChangeTriple())
}
@@ -981,7 +982,7 @@
}
// Ignore error. This isn't critical.
- gerritClient.SetReview(ts.ChangeTriple(), ts.Commit, gerrit.ReviewInput{Message: msg})
+ gerritClient.SetReview(ctx, ts.ChangeTriple(), ts.Commit, gerrit.ReviewInput{Message: msg})
}
// awaitTryBuild runs in its own goroutine and waits for a build in a
@@ -1087,7 +1088,7 @@
ts.mu.Unlock()
if numFail == 1 && remain > 0 {
- if err := gerritClient.SetReview(ts.ChangeTriple(), ts.Commit, gerrit.ReviewInput{
+ if err := gerritClient.SetReview(bs.ctx, ts.ChangeTriple(), ts.Commit, gerrit.ReviewInput{
Message: fmt.Sprintf(
"Build is still in progress...\n"+
"This change failed on %s:\n"+
@@ -1110,7 +1111,7 @@
score, msg = -1, fmt.Sprintf("%d of %d TryBots failed:\n%s\nConsult https://build.golang.org/ to see whether they are new failures.",
numFail, len(ts.builds), errMsg)
}
- if err := gerritClient.SetReview(ts.ChangeTriple(), ts.Commit, gerrit.ReviewInput{
+ if err := gerritClient.SetReview(bs.ctx, ts.ChangeTriple(), ts.Commit, gerrit.ReviewInput{
Message: msg,
Labels: map[string]int{
"TryBot-Result": score,
diff --git a/cmd/coordinator/gce.go b/cmd/coordinator/gce.go
index 3cdc25e..71be652 100644
--- a/cmd/coordinator/gce.go
+++ b/cmd/coordinator/gce.go
@@ -8,6 +8,7 @@
package main
import (
+ "context"
"encoding/json"
"errors"
"fmt"
@@ -28,7 +29,6 @@
"golang.org/x/build/dashboard"
"golang.org/x/build/gerrit"
"golang.org/x/build/internal/lru"
- "golang.org/x/net/context"
"golang.org/x/oauth2"
"golang.org/x/oauth2/google"
compute "google.golang.org/api/compute/v1"
diff --git a/cmd/coordinator/kube.go b/cmd/coordinator/kube.go
index 230d669..b57677d 100644
--- a/cmd/coordinator/kube.go
+++ b/cmd/coordinator/kube.go
@@ -5,6 +5,7 @@
package main
import (
+ "context"
"crypto/tls"
"crypto/x509"
"encoding/base64"
@@ -23,7 +24,6 @@
"golang.org/x/build/dashboard"
"golang.org/x/build/kubernetes"
"golang.org/x/build/kubernetes/api"
- "golang.org/x/net/context"
"golang.org/x/oauth2"
container "google.golang.org/api/container/v1"
)
diff --git a/cmd/coordinator/log.go b/cmd/coordinator/log.go
index d999e52..153dc0d 100644
--- a/cmd/coordinator/log.go
+++ b/cmd/coordinator/log.go
@@ -5,6 +5,7 @@
package main
import (
+ "context"
"fmt"
"log"
"time"
@@ -12,7 +13,6 @@
"cloud.google.com/go/datastore"
"golang.org/x/build/types"
- "golang.org/x/net/context"
)
// Process is a datastore record about the lifetime of a coordinator process.
diff --git a/cmd/coordinator/remote.go b/cmd/coordinator/remote.go
index 1cdd4d1..8313c4c 100644
--- a/cmd/coordinator/remote.go
+++ b/cmd/coordinator/remote.go
@@ -8,6 +8,7 @@
import (
"bytes"
+ "context"
"encoding/json"
"fmt"
"html"
@@ -21,7 +22,6 @@
"golang.org/x/build/buildlet"
"golang.org/x/build/dashboard"
- "golang.org/x/net/context"
)
var (
diff --git a/cmd/coordinator/reverse.go b/cmd/coordinator/reverse.go
index bd6ee6d..483cb16 100644
--- a/cmd/coordinator/reverse.go
+++ b/cmd/coordinator/reverse.go
@@ -29,6 +29,7 @@
import (
"bytes"
+ "context"
"errors"
"fmt"
"io"
@@ -44,7 +45,6 @@
"golang.org/x/build/buildlet"
"golang.org/x/build/dashboard"
"golang.org/x/build/revdial"
- "golang.org/x/net/context"
)
const minBuildletVersion = 1
diff --git a/gerrit/demo.go b/gerrit/demo.go
index 340bcde..9eec921 100644
--- a/gerrit/demo.go
+++ b/gerrit/demo.go
@@ -14,8 +14,10 @@
"os"
"path/filepath"
"strings"
+ "time"
"golang.org/x/build/gerrit"
+ "golang.org/x/net/context"
)
func main() {
@@ -25,7 +27,9 @@
}
c := gerrit.NewClient("https://go-review.googlesource.com",
gerrit.BasicAuth("git-gobot.golang.org", strings.TrimSpace(string(gobotPass))))
- cl, err := c.QueryChanges("label:Run-TryBot=1 label:TryBot-Result=0 project:go status:open", gerrit.QueryChangesOpt{
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ cl, err := c.QueryChanges(ctx, "label:Run-TryBot=1 label:TryBot-Result=0 project:go status:open", gerrit.QueryChangesOpt{
Fields: []string{"CURRENT_REVISION"},
})
if err != nil {
@@ -34,7 +38,7 @@
v, _ := json.MarshalIndent(cl, "", " ")
os.Stdout.Write(v)
- log.Printf("SetReview = %v", c.SetReview("I2383397c056a9ffe174ac7c2c6e5bb334406fbf9", "current", gerrit.ReviewInput{
+ log.Printf("SetReview = %v", c.SetReview(ctx, "I2383397c056a9ffe174ac7c2c6e5bb334406fbf9", "current", gerrit.ReviewInput{
Message: "test test",
Labels: map[string]int{
"TryBot-Result": 0,
diff --git a/gerrit/example_test.go b/gerrit/example_test.go
new file mode 100644
index 0000000..2161c47
--- /dev/null
+++ b/gerrit/example_test.go
@@ -0,0 +1,21 @@
+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package gerrit_test
+
+import (
+ "fmt"
+
+ "golang.org/x/build/gerrit"
+ "golang.org/x/net/context"
+)
+
+func Example() {
+ c := gerrit.NewClient("https://go-review.googlesource.com", gerrit.NoAuth)
+ info, err := c.GetProjectInfo(context.TODO(), "go")
+ if err != nil {
+ panic(err)
+ }
+ fmt.Println(info.Description)
+}
diff --git a/gerrit/gerrit.go b/gerrit/gerrit.go
index b983d90..cf3d5e7 100644
--- a/gerrit/gerrit.go
+++ b/gerrit/gerrit.go
@@ -3,6 +3,9 @@
// license that can be found in the LICENSE file.
// Package gerrit contains code to interact with Gerrit servers.
+//
+// The API is not subject to the Go 1 compatibility promise and may change at
+// any time.
package gerrit
import (
@@ -18,6 +21,8 @@
"strconv"
"strings"
"time"
+
+ "golang.org/x/net/context"
)
// Client is a Gerrit client.
@@ -81,7 +86,7 @@
func (urlValues) isDoArg() {}
-func (c *Client) do(dst interface{}, method, path string, opts ...doArg) error {
+func (c *Client) do(ctx context.Context, dst interface{}, method, path string, opts ...doArg) error {
var arg url.Values
var body interface{}
var wantStatus = http.StatusOK
@@ -127,7 +132,7 @@
req.Header.Set("Content-Type", contentType)
}
c.auth.setAuth(c, req)
- res, err := c.httpClient().Do(req)
+ res, err := c.httpClient().Do(withContext(req, ctx))
if err != nil {
return err
}
@@ -324,7 +329,7 @@
// QueryChanges queries changes. The q parameter is a Gerrit search query.
// For the API call, see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-changes
// For the query syntax, see https://gerrit-review.googlesource.com/Documentation/user-search.html#_search_operators
-func (c *Client) QueryChanges(q string, opts ...QueryChangesOpt) ([]*ChangeInfo, error) {
+func (c *Client) QueryChanges(ctx context.Context, q string, opts ...QueryChangesOpt) ([]*ChangeInfo, error) {
var opt QueryChangesOpt
switch len(opts) {
case 0:
@@ -334,7 +339,7 @@
return nil, errors.New("only 1 option struct supported")
}
var changes []*ChangeInfo
- err := c.do(&changes, "GET", "/changes/", urlValues{
+ err := c.do(ctx, &changes, "GET", "/changes/", urlValues{
"q": {q},
"n": condInt(opt.N),
"o": opt.Fields,
@@ -345,7 +350,7 @@
// 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, opts ...QueryChangesOpt) (*ChangeInfo, error) {
+func (c *Client) GetChangeDetail(ctx context.Context, changeID string, opts ...QueryChangesOpt) (*ChangeInfo, error) {
var opt QueryChangesOpt
switch len(opts) {
case 0:
@@ -355,7 +360,7 @@
return nil, errors.New("only 1 option struct supported")
}
var change ChangeInfo
- err := c.do(&change, "GET", "/changes/"+changeID+"/detail", urlValues{
+ err := c.do(ctx, &change, "GET", "/changes/"+changeID+"/detail", urlValues{
"o": opt.Fields,
})
if err != nil {
@@ -377,16 +382,16 @@
// For the API call, see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#set-review
// The changeID is https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id
// The revision is https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#revision-id
-func (c *Client) SetReview(changeID, revision string, review ReviewInput) error {
+func (c *Client) SetReview(ctx context.Context, changeID, revision string, review ReviewInput) error {
var res reviewInfo
- return c.do(&res, "POST", fmt.Sprintf("/changes/%s/revisions/%s/review", changeID, revision),
+ return c.do(ctx, &res, "POST", fmt.Sprintf("/changes/%s/revisions/%s/review", changeID, revision),
reqBody{review})
}
// AbandonChange abandons the given change.
-func (c *Client) AbandonChange(changeID string) error {
+func (c *Client) AbandonChange(ctx context.Context, changeID string) error {
var change ChangeInfo
- return c.do(&change, "POST", "/changes/"+changeID+"/abandon")
+ return c.do(ctx, &change, "POST", "/changes/"+changeID+"/abandon")
}
// ProjectInput contains the options for creating a new project.
@@ -413,7 +418,7 @@
}
// CreateProject creates a new project.
-func (c *Client) CreateProject(name string, p ...ProjectInput) (ProjectInfo, error) {
+func (c *Client) CreateProject(ctx context.Context, name string, p ...ProjectInput) (ProjectInfo, error) {
var pi ProjectInput
if len(p) > 1 {
panic("invalid use of multiple project inputs")
@@ -422,7 +427,7 @@
pi = p[0]
}
var res ProjectInfo
- err := c.do(&res, "PUT", fmt.Sprintf("/projects/%s", name), reqBody{&pi}, wantResStatus(http.StatusCreated))
+ err := c.do(ctx, &res, "PUT", fmt.Sprintf("/projects/%s", name), reqBody{&pi}, wantResStatus(http.StatusCreated))
return res, err
}
@@ -433,9 +438,9 @@
// GetProjectInfo returns info about a project.
// If the project doesn't exist, the error will be ErrProjectNotExist.
-func (c *Client) GetProjectInfo(name string) (ProjectInfo, error) {
+func (c *Client) GetProjectInfo(ctx context.Context, name string) (ProjectInfo, error) {
var res ProjectInfo
- err := c.do(&res, "GET", fmt.Sprintf("/projects/%s", name))
+ err := c.do(ctx, &res, "GET", fmt.Sprintf("/projects/%s", name))
if he, ok := err.(*HTTPError); ok && he.Res.StatusCode == 404 {
return res, ErrProjectNotExist
}
@@ -451,9 +456,9 @@
}
// GetProjectBranches returns a project's branches.
-func (c *Client) GetProjectBranches(name string) (map[string]BranchInfo, error) {
+func (c *Client) GetProjectBranches(ctx context.Context, name string) (map[string]BranchInfo, error) {
var res []BranchInfo
- err := c.do(&res, "GET", fmt.Sprintf("/projects/%s/branches/", name))
+ err := c.do(ctx, &res, "GET", fmt.Sprintf("/projects/%s/branches/", name))
if err != nil {
return nil, err
}
@@ -470,9 +475,9 @@
//
// Note that getting "self" is a good way to validate host access, since it only requires peeker
// access to the host, not to any particular repository.
-func (c *Client) GetAccountInfo(accountID string) (AccountInfo, error) {
+func (c *Client) GetAccountInfo(ctx context.Context, accountID string) (AccountInfo, error) {
var res AccountInfo
- err := c.do(&res, "GET", fmt.Sprintf("/accounts/%s", accountID))
+ err := c.do(ctx, &res, "GET", fmt.Sprintf("/accounts/%s", accountID))
return res, err
}
diff --git a/gerrit/gerrit_ctx.go b/gerrit/gerrit_ctx.go
new file mode 100644
index 0000000..1776fad
--- /dev/null
+++ b/gerrit/gerrit_ctx.go
@@ -0,0 +1,17 @@
+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build go1.7
+
+package gerrit
+
+import (
+ "net/http"
+
+ "golang.org/x/net/context"
+)
+
+func withContext(r *http.Request, ctx context.Context) *http.Request {
+ return r.WithContext(ctx)
+}
diff --git a/gerrit/gerrit_noctx.go b/gerrit/gerrit_noctx.go
new file mode 100644
index 0000000..529a4a1
--- /dev/null
+++ b/gerrit/gerrit_noctx.go
@@ -0,0 +1,18 @@
+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build !go1.7
+
+package gerrit
+
+import (
+ "net/http"
+
+ "golang.org/x/net/context"
+)
+
+func withContext(r *http.Request, ctx context.Context) *http.Request {
+ r.Cancel = ctx.Done()
+ return r
+}
diff --git a/gerrit/gerrit_test.go b/gerrit/gerrit_test.go
new file mode 100644
index 0000000..bfee24a
--- /dev/null
+++ b/gerrit/gerrit_test.go
@@ -0,0 +1,92 @@
+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package gerrit
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "testing"
+ "time"
+
+ "golang.org/x/net/context"
+)
+
+// taken from https://go-review.googlesource.com/projects/go
+var exampleProjectResponse = []byte(`)]}'
+{
+ "id": "go",
+ "name": "go",
+ "parent": "All-Projects",
+ "description": "The Go Programming Language",
+ "state": "ACTIVE",
+ "web_links": [
+ {
+ "name": "gitiles",
+ "url": "https://go.googlesource.com/go/",
+ "target": "_blank"
+ }
+ ]
+}
+`)
+
+func TestGetProjectInfo(t *testing.T) {
+ hitServer := false
+ path := ""
+ s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ hitServer = true
+ path = r.URL.Path
+ w.Header().Set("Content-Type", "application/json; charset=UTF-8")
+ w.WriteHeader(200)
+ w.Write(exampleProjectResponse)
+ }))
+ defer s.Close()
+ c := NewClient(s.URL, NoAuth)
+ info, err := c.GetProjectInfo(context.Background(), "go")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !hitServer {
+ t.Errorf("expected to hit test server, didn't")
+ }
+ if path != "/projects/go" {
+ t.Errorf("expected Path to be '/projects/go', got %s", path)
+ }
+ if info.Name != "go" {
+ t.Errorf("expected Name to be 'go', got %s", info.Name)
+ }
+}
+
+func TestProjectNotFound(t *testing.T) {
+ s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "text/plain; charset=UTF-8")
+ w.WriteHeader(404)
+ w.Write([]byte("Not found: unknown"))
+ }))
+ defer s.Close()
+ c := NewClient(s.URL, NoAuth)
+ _, err := c.GetProjectInfo(context.Background(), "unknown")
+ if err != ErrProjectNotExist {
+ t.Errorf("expected to get ErrProjectNotExist, got %v", err)
+ }
+}
+
+func TestContextError(t *testing.T) {
+ c := NewClient("http://localhost", NoAuth)
+ yearsAgo, _ := time.Parse("2006", "2006")
+ ctx, cancel := context.WithDeadline(context.Background(), yearsAgo)
+ defer cancel()
+ _, err := c.GetProjectInfo(ctx, "unknown")
+ if err == nil {
+ t.Errorf("expected non-nil error, got nil")
+ }
+ uerr, ok := err.(*url.Error)
+ if !ok {
+ t.Errorf("expected url.Error, got %#v", err)
+ }
+ if uerr.Err != context.DeadlineExceeded {
+ t.Errorf("expected DeadlineExceeded error, got %v", uerr.Err)
+ }
+}
diff --git a/godash/gerrit.go b/godash/gerrit.go
index d5207ff..bb6ccac 100644
--- a/godash/gerrit.go
+++ b/godash/gerrit.go
@@ -4,10 +4,13 @@
package godash
-import "golang.org/x/build/gerrit"
+import (
+ "golang.org/x/build/gerrit"
+ "golang.org/x/net/context"
+)
-func fetchCLs(client *gerrit.Client, reviewers *Reviewers, goReleaseCycle int, q string) ([]*CL, error) {
- cis, err := client.QueryChanges("-project:scratch -message:do-not-review "+q, gerrit.QueryChangesOpt{
+func fetchCLs(ctx context.Context, client *gerrit.Client, reviewers *Reviewers, goReleaseCycle int, q string) ([]*CL, error) {
+ cis, err := client.QueryChanges(ctx, "-project:scratch -message:do-not-review "+q, gerrit.QueryChangesOpt{
N: 5000,
Fields: []string{
"LABELS",
diff --git a/godash/godash.go b/godash/godash.go
index fc2a545..e7d86ad 100644
--- a/godash/godash.go
+++ b/godash/godash.go
@@ -83,7 +83,7 @@
}
}
since := d.Now.Add(-(time.Duration(days)*24 + 12) * time.Hour).UTC().Round(time.Second)
- cls, err := fetchCLs(ger, d.Reviewers, d.GoReleaseCycle, "is:open")
+ cls, err := fetchCLs(ctx, ger, d.Reviewers, d.GoReleaseCycle, "is:open")
if err != nil {
return err
}
@@ -97,7 +97,7 @@
}
}
if includeMerged {
- cls, err := fetchCLs(ger, d.Reviewers, d.GoReleaseCycle, "is:merged since:\""+since.Format("2006-01-02 15:04:05")+"\"")
+ cls, err := fetchCLs(ctx, ger, d.Reviewers, d.GoReleaseCycle, "is:merged since:\""+since.Format("2006-01-02 15:04:05")+"\"")
if err != nil {
return err
}
