From 994c90e2d2dc3806350f1635839f601e2fac5bb6 Mon Sep 17 00:00:00 2001 From: Adin Scannell Date: Tue, 29 Sep 2020 13:14:52 -0700 Subject: Add nogo check annotations to GitHub. When nogo checks are violated, they will automatically posted as annotations on the specific GitHub commit. This allows us to ensure analysis & style rules and have them called out. PiperOrigin-RevId: 334447285 --- tools/issue_reviver/BUILD | 13 -- tools/issue_reviver/github/BUILD | 25 ---- tools/issue_reviver/github/github.go | 176 ------------------------- tools/issue_reviver/github/github_test.go | 55 -------- tools/issue_reviver/main.go | 100 --------------- tools/issue_reviver/reviver/BUILD | 18 --- tools/issue_reviver/reviver/reviver.go | 192 ---------------------------- tools/issue_reviver/reviver/reviver_test.go | 88 ------------- 8 files changed, 667 deletions(-) delete mode 100644 tools/issue_reviver/BUILD delete mode 100644 tools/issue_reviver/github/BUILD delete mode 100644 tools/issue_reviver/github/github.go delete mode 100644 tools/issue_reviver/github/github_test.go delete mode 100644 tools/issue_reviver/main.go delete mode 100644 tools/issue_reviver/reviver/BUILD delete mode 100644 tools/issue_reviver/reviver/reviver.go delete mode 100644 tools/issue_reviver/reviver/reviver_test.go (limited to 'tools/issue_reviver') diff --git a/tools/issue_reviver/BUILD b/tools/issue_reviver/BUILD deleted file mode 100644 index 35b0111ca..000000000 --- a/tools/issue_reviver/BUILD +++ /dev/null @@ -1,13 +0,0 @@ -load("//tools:defs.bzl", "go_binary") - -package(licenses = ["notice"]) - -go_binary( - name = "issue_reviver", - srcs = ["main.go"], - nogo = False, - deps = [ - "//tools/issue_reviver/github", - "//tools/issue_reviver/reviver", - ], -) diff --git a/tools/issue_reviver/github/BUILD b/tools/issue_reviver/github/BUILD deleted file mode 100644 index 555abd296..000000000 --- a/tools/issue_reviver/github/BUILD +++ /dev/null @@ -1,25 +0,0 @@ -load("//tools:defs.bzl", "go_library", "go_test") - -package(licenses = ["notice"]) - -go_library( - name = "github", - srcs = ["github.go"], - nogo = False, - visibility = [ - "//tools/issue_reviver:__subpackages__", - ], - deps = [ - "//tools/issue_reviver/reviver", - "@com_github_google_go_github_v28//github:go_default_library", - "@org_golang_x_oauth2//:go_default_library", - ], -) - -go_test( - name = "github_test", - size = "small", - srcs = ["github_test.go"], - library = ":github", - nogo = False, -) diff --git a/tools/issue_reviver/github/github.go b/tools/issue_reviver/github/github.go deleted file mode 100644 index 8ffd7e606..000000000 --- a/tools/issue_reviver/github/github.go +++ /dev/null @@ -1,176 +0,0 @@ -// Copyright 2019 The gVisor Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package github implements reviver.Bugger interface on top of Github issues. -package github - -import ( - "context" - "fmt" - "strconv" - "strings" - "time" - - "github.com/google/go-github/github" - "golang.org/x/oauth2" - "gvisor.dev/gvisor/tools/issue_reviver/reviver" -) - -// Bugger implements reviver.Bugger interface for github issues. -type Bugger struct { - owner string - repo string - dryRun bool - - client *github.Client - issues map[int]*github.Issue -} - -// NewBugger creates a new Bugger. -func NewBugger(token, owner, repo string, dryRun bool) (*Bugger, error) { - b := &Bugger{ - owner: owner, - repo: repo, - dryRun: dryRun, - issues: map[int]*github.Issue{}, - } - if err := b.load(token); err != nil { - return nil, err - } - return b, nil -} - -func (b *Bugger) load(token string) error { - ctx := context.Background() - if len(token) == 0 { - fmt.Print("No OAUTH token provided, using unauthenticated account.\n") - b.client = github.NewClient(nil) - } else { - ts := oauth2.StaticTokenSource( - &oauth2.Token{AccessToken: token}, - ) - tc := oauth2.NewClient(ctx, ts) - b.client = github.NewClient(tc) - } - - err := processAllPages(func(listOpts github.ListOptions) (*github.Response, error) { - opts := &github.IssueListByRepoOptions{State: "open", ListOptions: listOpts} - tmps, resp, err := b.client.Issues.ListByRepo(ctx, b.owner, b.repo, opts) - if err != nil { - return resp, err - } - for _, issue := range tmps { - b.issues[issue.GetNumber()] = issue - } - return resp, nil - }) - if err != nil { - return err - } - - fmt.Printf("Loaded %d issues from github.com/%s/%s\n", len(b.issues), b.owner, b.repo) - return nil -} - -// Activate implements reviver.Bugger. -func (b *Bugger) Activate(todo *reviver.Todo) (bool, error) { - id, err := parseIssueNo(todo.Issue) - if err != nil { - return true, err - } - if id <= 0 { - return false, nil - } - - // Check against active issues cache. - if _, ok := b.issues[id]; ok { - fmt.Printf("%q is active: OK\n", todo.Issue) - return true, nil - } - - fmt.Printf("%q is not active: reopening issue %d\n", todo.Issue, id) - - // Format comment with TODO locations and search link. - comment := strings.Builder{} - fmt.Fprintln(&comment, "There are TODOs still referencing this issue:") - for _, l := range todo.Locations { - fmt.Fprintf(&comment, - "1. [%s:%d](https://github.com/%s/%s/blob/HEAD/%s#%d): %s\n", - l.File, l.Line, b.owner, b.repo, l.File, l.Line, l.Comment) - } - fmt.Fprintf(&comment, - "\n\nSearch [TODO](https://github.com/%s/%s/search?q=%%22%s%%22)", b.owner, b.repo, todo.Issue) - - if b.dryRun { - fmt.Printf("[dry-run: skipping change to issue %d]\n%s\n=======================\n", id, comment.String()) - return true, nil - } - - ctx := context.Background() - req := &github.IssueRequest{State: github.String("open")} - _, _, err = b.client.Issues.Edit(ctx, b.owner, b.repo, id, req) - if err != nil { - return true, fmt.Errorf("failed to reactivate issue %d: %v", id, err) - } - - cmt := &github.IssueComment{ - Body: github.String(comment.String()), - Reactions: &github.Reactions{Confused: github.Int(1)}, - } - if _, _, err := b.client.Issues.CreateComment(ctx, b.owner, b.repo, id, cmt); err != nil { - return true, fmt.Errorf("failed to add comment to issue %d: %v", id, err) - } - - return true, nil -} - -// parseIssueNo parses the issue number out of the issue url. -func parseIssueNo(url string) (int, error) { - const prefix = "gvisor.dev/issue/" - - // First check if I can handle the TODO. - idStr := strings.TrimPrefix(url, prefix) - if len(url) == len(idStr) { - return 0, nil - } - - id, err := strconv.ParseInt(strings.TrimRight(idStr, "/"), 10, 64) - if err != nil { - return 0, err - } - return int(id), nil -} - -func processAllPages(fn func(github.ListOptions) (*github.Response, error)) error { - opts := github.ListOptions{PerPage: 1000} - for { - resp, err := fn(opts) - if err != nil { - if rateErr, ok := err.(*github.RateLimitError); ok { - duration := rateErr.Rate.Reset.Sub(time.Now()) - if duration > 5*time.Minute { - return fmt.Errorf("Rate limited for too long: %v", duration) - } - fmt.Printf("Rate limited, sleeping for: %v\n", duration) - time.Sleep(duration) - continue - } - return err - } - if resp.NextPage == 0 { - return nil - } - opts.Page = resp.NextPage - } -} diff --git a/tools/issue_reviver/github/github_test.go b/tools/issue_reviver/github/github_test.go deleted file mode 100644 index a78b230ef..000000000 --- a/tools/issue_reviver/github/github_test.go +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright 2020 The gVisor Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package github - -import ( - "testing" -) - -func TestParseIssueNo(t *testing.T) { - testCases := []struct { - issue string - expectErr bool - expected int - }{ - { - issue: "gvisor.dev/issue/123", - expected: 123, - }, - { - issue: "gvisor.dev/issue/123/", - expected: 123, - }, - { - issue: "not a url", - expected: 0, - }, - { - issue: "gvisor.dev/issue//", - expectErr: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.issue, func(t *testing.T) { - id, err := parseIssueNo(tc.issue) - if err != nil && !tc.expectErr { - t.Errorf("got error: %v", err) - } else if tc.expected != id { - t.Errorf("got: %v, want: %v", id, tc.expected) - } - }) - } -} diff --git a/tools/issue_reviver/main.go b/tools/issue_reviver/main.go deleted file mode 100644 index 47c796b8a..000000000 --- a/tools/issue_reviver/main.go +++ /dev/null @@ -1,100 +0,0 @@ -// Copyright 2019 The gVisor Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package main is the entry point for issue_reviver. -package main - -import ( - "flag" - "fmt" - "io/ioutil" - "os" - "strings" - - "gvisor.dev/gvisor/tools/issue_reviver/github" - "gvisor.dev/gvisor/tools/issue_reviver/reviver" -) - -var ( - owner string - repo string - tokenFile string - path string - dryRun bool -) - -// Keep the options simple for now. Supports only a single path and repo. -func init() { - flag.StringVar(&owner, "owner", "", "Github project org/owner to look for issues") - flag.StringVar(&repo, "repo", "", "Github repo to look for issues") - flag.StringVar(&tokenFile, "oauth-token-file", "", "Path to file containing the OAUTH token to be used as credential to github") - flag.StringVar(&path, "path", ".", "Path to scan for TODOs") - flag.BoolVar(&dryRun, "dry-run", false, "If set to true, no changes are made to issues") -} - -func main() { - // Set defaults from the environment. - repository := os.Getenv("GITHUB_REPOSITORY") - if parts := strings.SplitN(repository, "/", 2); len(parts) == 2 { - owner = parts[0] - repo = parts[1] - } - - // Parse flags. - flag.Parse() - - // Check for mandatory parameters. - if len(owner) == 0 { - fmt.Println("missing --owner option.") - flag.Usage() - os.Exit(1) - } - if len(repo) == 0 { - fmt.Println("missing --repo option.") - flag.Usage() - os.Exit(1) - } - if len(path) == 0 { - fmt.Println("missing --path option.") - flag.Usage() - os.Exit(1) - } - - // The access token may be passed as a file so it doesn't show up in - // command line arguments. It also may be provided through the - // environment to faciliate use through GitHub's CI system. - token := os.Getenv("GITHUB_TOKEN") - if len(tokenFile) != 0 { - bytes, err := ioutil.ReadFile(tokenFile) - if err != nil { - fmt.Println(err.Error()) - os.Exit(1) - } - token = string(bytes) - } - - bugger, err := github.NewBugger(token, owner, repo, dryRun) - if err != nil { - fmt.Fprintln(os.Stderr, "Error getting github issues:", err) - os.Exit(1) - } - rev := reviver.New([]string{path}, []reviver.Bugger{bugger}) - if errs := rev.Run(); len(errs) > 0 { - fmt.Fprintf(os.Stderr, "Encountered %d errors:\n", len(errs)) - for _, err := range errs { - fmt.Fprintf(os.Stderr, "\t%v\n", err) - } - os.Exit(1) - } -} diff --git a/tools/issue_reviver/reviver/BUILD b/tools/issue_reviver/reviver/BUILD deleted file mode 100644 index d262932bd..000000000 --- a/tools/issue_reviver/reviver/BUILD +++ /dev/null @@ -1,18 +0,0 @@ -load("//tools:defs.bzl", "go_library", "go_test") - -package(licenses = ["notice"]) - -go_library( - name = "reviver", - srcs = ["reviver.go"], - visibility = [ - "//tools/issue_reviver:__subpackages__", - ], -) - -go_test( - name = "reviver_test", - size = "small", - srcs = ["reviver_test.go"], - library = ":reviver", -) diff --git a/tools/issue_reviver/reviver/reviver.go b/tools/issue_reviver/reviver/reviver.go deleted file mode 100644 index 2af7f0d59..000000000 --- a/tools/issue_reviver/reviver/reviver.go +++ /dev/null @@ -1,192 +0,0 @@ -// Copyright 2019 The gVisor Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package reviver scans the code looking for TODOs and pass them to registered -// Buggers to ensure TODOs point to active issues. -package reviver - -import ( - "bufio" - "fmt" - "io/ioutil" - "os" - "path/filepath" - "regexp" - "sync" -) - -// regexTodo matches a TODO or FIXME comment. -var regexTodo = regexp.MustCompile(`(\/\/|#)\s*(TODO|FIXME)\(([a-zA-Z0-9.\/]+)\):\s*(.+)`) - -// Bugger interface is called for every TODO found in the code. If it can handle -// the TODO, it must return true. If it returns false, the next Bugger is -// called. If no Bugger handles the TODO, it's dropped on the floor. -type Bugger interface { - Activate(todo *Todo) (bool, error) -} - -// Location saves the location where the TODO was found. -type Location struct { - Comment string - File string - Line uint -} - -// Todo represents a unique TODO. There can be several TODOs pointing to the -// same issue in the code. They are all grouped together. -type Todo struct { - Issue string - Locations []Location -} - -// Reviver scans the given paths for TODOs and calls Buggers to handle them. -type Reviver struct { - paths []string - buggers []Bugger - - mu sync.Mutex - todos map[string]*Todo - errs []error -} - -// New create a new Reviver. -func New(paths []string, buggers []Bugger) *Reviver { - return &Reviver{ - paths: paths, - buggers: buggers, - todos: map[string]*Todo{}, - } -} - -// Run runs. It returns all errors found during processing, it doesn't stop -// on errors. -func (r *Reviver) Run() []error { - // Process each directory in parallel. - wg := sync.WaitGroup{} - for _, path := range r.paths { - wg.Add(1) - go func(path string) { - defer wg.Done() - r.processPath(path, &wg) - }(path) - } - - wg.Wait() - - r.mu.Lock() - defer r.mu.Unlock() - - fmt.Printf("Processing %d TODOs (%d errors)...\n", len(r.todos), len(r.errs)) - dropped := 0 - for _, todo := range r.todos { - ok, err := r.processTodo(todo) - if err != nil { - r.errs = append(r.errs, err) - } - if !ok { - dropped++ - } - } - fmt.Printf("Processed %d TODOs, %d were skipped (%d errors)\n", len(r.todos)-dropped, dropped, len(r.errs)) - - return r.errs -} - -func (r *Reviver) processPath(path string, wg *sync.WaitGroup) { - fmt.Printf("Processing dir %q\n", path) - fis, err := ioutil.ReadDir(path) - if err != nil { - r.addErr(fmt.Errorf("error processing dir %q: %v", path, err)) - return - } - - for _, fi := range fis { - childPath := filepath.Join(path, fi.Name()) - switch { - case fi.Mode().IsDir(): - wg.Add(1) - go func() { - defer wg.Done() - r.processPath(childPath, wg) - }() - - case fi.Mode().IsRegular(): - file, err := os.Open(childPath) - if err != nil { - r.addErr(err) - continue - } - - scanner := bufio.NewScanner(file) - lineno := uint(0) - for scanner.Scan() { - lineno++ - line := scanner.Text() - if todo := r.processLine(line, childPath, lineno); todo != nil { - r.addTodo(todo) - } - } - } - } -} - -func (r *Reviver) processLine(line, path string, lineno uint) *Todo { - matches := regexTodo.FindStringSubmatch(line) - if matches == nil { - return nil - } - if len(matches) != 5 { - panic(fmt.Sprintf("regex returned wrong matches for %q: %v", line, matches)) - } - return &Todo{ - Issue: matches[3], - Locations: []Location{ - { - File: path, - Line: lineno, - Comment: matches[4], - }, - }, - } -} - -func (r *Reviver) addTodo(newTodo *Todo) { - r.mu.Lock() - defer r.mu.Unlock() - - if todo := r.todos[newTodo.Issue]; todo == nil { - r.todos[newTodo.Issue] = newTodo - } else { - todo.Locations = append(todo.Locations, newTodo.Locations...) - } -} - -func (r *Reviver) addErr(err error) { - r.mu.Lock() - defer r.mu.Unlock() - r.errs = append(r.errs, err) -} - -func (r *Reviver) processTodo(todo *Todo) (bool, error) { - for _, bugger := range r.buggers { - ok, err := bugger.Activate(todo) - if err != nil { - return false, err - } - if ok { - return true, nil - } - } - return false, nil -} diff --git a/tools/issue_reviver/reviver/reviver_test.go b/tools/issue_reviver/reviver/reviver_test.go deleted file mode 100644 index a9fb1f9f1..000000000 --- a/tools/issue_reviver/reviver/reviver_test.go +++ /dev/null @@ -1,88 +0,0 @@ -// Copyright 2019 The gVisor Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package reviver - -import ( - "testing" -) - -func TestProcessLine(t *testing.T) { - for _, tc := range []struct { - line string - want *Todo - }{ - { - line: "// TODO(foobar.com/issue/123): comment, bla. blabla.", - want: &Todo{ - Issue: "foobar.com/issue/123", - Locations: []Location{ - {Comment: "comment, bla. blabla."}, - }, - }, - }, - { - line: "// FIXME(b/123): internal bug", - want: &Todo{ - Issue: "b/123", - Locations: []Location{ - {Comment: "internal bug"}, - }, - }, - }, - { - line: "TODO(issue): not todo", - }, - { - line: "FIXME(issue): not todo", - }, - { - line: "// TODO (issue): not todo", - }, - { - line: "// TODO(issue) not todo", - }, - { - line: "// todo(issue): not todo", - }, - { - line: "// TODO(issue):", - }, - } { - t.Logf("Testing: %s", tc.line) - r := Reviver{} - got := r.processLine(tc.line, "test", 0) - if got == nil { - if tc.want != nil { - t.Errorf("failed to process line, want: %+v", tc.want) - } - } else { - if tc.want == nil { - t.Errorf("expected error, got: %+v", got) - continue - } - if got.Issue != tc.want.Issue { - t.Errorf("wrong issue, got: %v, want: %v", got.Issue, tc.want.Issue) - } - if len(got.Locations) != len(tc.want.Locations) { - t.Errorf("wrong number of locations, got: %v, want: %v, locations: %+v", len(got.Locations), len(tc.want.Locations), got.Locations) - } - for i, wantLoc := range tc.want.Locations { - if got.Locations[i].Comment != wantLoc.Comment { - t.Errorf("wrong comment, got: %v, want: %v", got.Locations[i].Comment, wantLoc.Comment) - } - } - } - } -} -- cgit v1.2.3