From 1f697e3792098acf585c2c9d16190647b0cc8f5b Mon Sep 17 00:00:00 2001 From: Sam Wronski Date: Thu, 5 Nov 2020 11:21:15 -0800 Subject: [PATCH 1/6] Run multi-module check during presubmit --- Makefile | 13 +++++++++++++ cmd/prchecker/main.go | 44 +++++++++++++++++++++++++++---------------- 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/Makefile b/Makefile index cf299c7bc..e791f225e 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,7 @@ MYGOBIN := $(shell go env GOPATH)/bin SHELL := /bin/bash export PATH := $(MYGOBIN):$(PATH) +MODULES := "cmd/config" "api/" "kustomize/" "kyaml/" .PHONY: all all: verify-kustomize @@ -23,6 +24,7 @@ verify-kustomize: \ .PHONY: prow-presubmit-check prow-presubmit-check: \ lint-kustomize \ + test-multi-module \ test-unit-kustomize-all \ test-unit-cmd-all \ test-go-mod \ @@ -226,6 +228,17 @@ test-unit-cmd-all: test-go-mod: ./travis/check-go-mod.sh +# Environment variables are defined at +# https://github.com/kubernetes/test-infra/blob/master/prow/jobs.md#job-environment-variables +.PHONY: test-multi-module +test-multi-module: + go install github.com/google/go-github/github + go run ./cmd/prchecker \ + -owner=$(REPO_OWNER) \ + -repo=$(REPO_NAME) \ + -pr=$(PR_NUM) \ + $(MODULES) + .PHONY: test-examples-e2e-kustomize: $(MYGOBIN)/mdrip $(MYGOBIN)/kind ( \ diff --git a/cmd/prchecker/main.go b/cmd/prchecker/main.go index 0c13eeeda..982fe60d7 100644 --- a/cmd/prchecker/main.go +++ b/cmd/prchecker/main.go @@ -27,9 +27,8 @@ import ( // splitCommitsHelp is a help message displayed when a PR has commits which // span modules. -const splitCommitsHelp = "\nModifications to multiple restricted directories occurred.\n" + - "To resolve this please limit code changes in a commit to a single go module.\n\n" + - "You may learn more about how to split your existing commits at: https://git-scm.com/docs/git-rebase#_splitting_commits\n" +const splitCommitsHelp = "\nCommits that include multiple Go modules must be split into multiple commits that each touch no more than one module.\n" + + "Splitting instructions: https://git-scm.com/docs/git-rebase#_splitting_commits\n" // Ignore span pattern defines a regular expression pattern that, when matched, // causes this check to be ignored. @@ -62,16 +61,33 @@ type GitHubService interface { ListCommits(prId int, options *github.ListOptions) ([]*github.RepositoryCommit, *github.Response, error) } -func (repository GitHubRepository) GetPullRequest(prId int) (*github.PullRequest, *github.Response, error) { - return repository.client.PullRequests.Get(context.Background(), *repository.owner, *repository.repo, prId) +// GetPullRequest retrieves details about a pull request from GitHub API +func (repository GitHubRepository) GetPullRequest(prId int) ( + *github.PullRequest, *github.Response, error) { + return repository.client.PullRequests.Get( + context.Background(), + *repository.owner, + *repository.repo, + prId) } -func (repository GitHubRepository) GetCommit(commitSha string) (*github.RepositoryCommit, *github.Response, error) { - return repository.client.Repositories.GetCommit(context.Background(), *repository.owner, *repository.repo, commitSha) +// GetCommit retrieves commit details from GitHub API +func (repository GitHubRepository) GetCommit(commitSha string) ( + *github.RepositoryCommit, *github.Response, error) { + return repository.client.Repositories.GetCommit(context.Background(), + *repository.owner, + *repository.repo, + commitSha) } -func (repository GitHubRepository) ListCommits(prId int, options *github.ListOptions) ([]*github.RepositoryCommit, *github.Response, error) { - return repository.client.PullRequests.ListCommits(context.Background(), *repository.owner, *repository.repo, prId, options) +// ListCommits lists commits in a PR via GitHub API +func (repository GitHubRepository) ListCommits(prId int, options *github.ListOptions) ( + []*github.RepositoryCommit, *github.Response, error) { + return repository.client.PullRequests.ListCommits(context.Background(), + *repository.owner, + *repository.repo, + prId, + options) } func main() { @@ -145,7 +161,7 @@ func StringAllowsModuleSpan(body string) (bool, error) { return regexp.MatchString(ignoreSpanPattern, body) } -// IsModuleSpanAllowed tests a Pull Requests description for a regular +// ModuleSpanAllowed tests a Pull Requests description for a regular // expression. If the expression matches then spanning modules are allowed. func ModuleSpanAllowed(repository GitHubService, pullId int) (bool, error) { @@ -163,9 +179,6 @@ func ModuleSpanAllowed(repository GitHubService, pullId int) (bool, error) { // GetCommitChanges looks up a github commit by SHA and returns a Changeset // containing the modified files in the specified commit. func GetCommitChanges(repository GitHubService, commitSha string) (*Changeset, error) { - - // Note: There are multiple ways to pull a github commit object - // we want a RepositoryCommit. commit, _, err := repository.GetCommit(commitSha) if err != nil { @@ -210,7 +223,7 @@ func GetPullRequestCommits(repository GitHubService, pullrequest int) ([]*Change return changesetResults, nil } -// IsPullRequestSpanningPathList tests if a pull request spans multiple +// PullRequestSpanningPathList tests if a pull request spans multiple // directory paths func PullRequestSpanningPathList(repository GitHubService, pullrequest int, paths []string) (bool, []*Changeset, error) { // Create a buffer for commits @@ -237,7 +250,7 @@ func PullRequestSpanningPathList(repository GitHubService, pullrequest int, path return spanningChangesExist, changesets, nil } -// IsChangesetSpanningPathList tests if a changeset is spanning +// ChangesetSpanningPathList tests if a changeset is spanning // multiple directory paths. func ChangesetSpanningPathList(changeset *Changeset, paths []string) bool { matchedPath := "" @@ -254,6 +267,5 @@ func ChangesetSpanningPathList(changeset *Changeset, paths []string) bool { } } - // If more than 1 matched paths then the changeset spans paths return false } From 712eb6d27655f1e09059ca0dfb99f9ed3ab43fd1 Mon Sep 17 00:00:00 2001 From: Sam Wronski Date: Thu, 5 Nov 2020 12:23:45 -0800 Subject: [PATCH 2/6] Refactor changeset spanning function naming --- cmd/prchecker/main.go | 6 +++--- cmd/prchecker/main_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/prchecker/main.go b/cmd/prchecker/main.go index 982fe60d7..4459e0d61 100644 --- a/cmd/prchecker/main.go +++ b/cmd/prchecker/main.go @@ -235,7 +235,7 @@ func PullRequestSpanningPathList(repository GitHubService, pullrequest int, path spanningChangesExist := false for _, changeset := range changesets { - if ChangesetSpanningPathList(changeset, paths) { + if changeset.isSpanningPaths(paths) { // When detecting the first spanning changeset print a prefix message if !spanningChangesExist { fmt.Printf("Spanning changesets detected in the following commits:\n\n") @@ -250,9 +250,9 @@ func PullRequestSpanningPathList(repository GitHubService, pullrequest int, path return spanningChangesExist, changesets, nil } -// ChangesetSpanningPathList tests if a changeset is spanning +// isSpanningPaths tests if a changeset is spanning // multiple directory paths. -func ChangesetSpanningPathList(changeset *Changeset, paths []string) bool { +func (changeset *Changeset) isSpanningPaths(paths []string) bool { matchedPath := "" for _, file := range changeset.files { diff --git a/cmd/prchecker/main_test.go b/cmd/prchecker/main_test.go index edb6f9dbe..88bea9c31 100644 --- a/cmd/prchecker/main_test.go +++ b/cmd/prchecker/main_test.go @@ -260,7 +260,7 @@ func TestIsChangesetSpanning(t *testing.T) { changeset := &Changeset{files: tt.changeset} - result := ChangesetSpanningPathList(changeset, tt.files) + result := changeset.isSpanningPaths(tt.files) if result != tt.expected { t.Errorf("got %t, want %t", result, tt.expected) From 9d2f257acf1f7b5dcc088e21892fae2fcb61ea8b Mon Sep 17 00:00:00 2001 From: Sam Wronski Date: Thu, 5 Nov 2020 15:28:37 -0800 Subject: [PATCH 3/6] Install dependencies from go module --- Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Makefile b/Makefile index e791f225e..a33334e03 100644 --- a/Makefile +++ b/Makefile @@ -231,8 +231,7 @@ test-go-mod: # Environment variables are defined at # https://github.com/kubernetes/test-infra/blob/master/prow/jobs.md#job-environment-variables .PHONY: test-multi-module -test-multi-module: - go install github.com/google/go-github/github +test-multi-module: $(MYGOBIN)/prchecker go run ./cmd/prchecker \ -owner=$(REPO_OWNER) \ -repo=$(REPO_NAME) \ From 03b847a7490ca5757e3786a14b460f59f11b4e2f Mon Sep 17 00:00:00 2001 From: Sam Wronski Date: Thu, 5 Nov 2020 15:44:40 -0800 Subject: [PATCH 4/6] Use `PULL_NUMBER` env from prow --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index a33334e03..4369eb5c4 100644 --- a/Makefile +++ b/Makefile @@ -235,7 +235,7 @@ test-multi-module: $(MYGOBIN)/prchecker go run ./cmd/prchecker \ -owner=$(REPO_OWNER) \ -repo=$(REPO_NAME) \ - -pr=$(PR_NUM) \ + -pr=$(PULL_NUMBER) \ $(MODULES) .PHONY: From fe84d119d6e42e98355b250e2af47e820c17100c Mon Sep 17 00:00:00 2001 From: Sam Wronski Date: Thu, 5 Nov 2020 16:03:36 -0800 Subject: [PATCH 5/6] Use gomodule when running prchecker --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 4369eb5c4..64f7dc749 100644 --- a/Makefile +++ b/Makefile @@ -232,7 +232,7 @@ test-go-mod: # https://github.com/kubernetes/test-infra/blob/master/prow/jobs.md#job-environment-variables .PHONY: test-multi-module test-multi-module: $(MYGOBIN)/prchecker - go run ./cmd/prchecker \ + cd ./cmd/prchecker; go run . \ -owner=$(REPO_OWNER) \ -repo=$(REPO_NAME) \ -pr=$(PULL_NUMBER) \ From ec2a6e4e4ba4a5d782b00edde75c0b8ccf3532cf Mon Sep 17 00:00:00 2001 From: Jeff Regan Date: Fri, 6 Nov 2020 13:15:43 -0800 Subject: [PATCH 6/6] Update Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 64f7dc749..63b289beb 100644 --- a/Makefile +++ b/Makefile @@ -232,7 +232,7 @@ test-go-mod: # https://github.com/kubernetes/test-infra/blob/master/prow/jobs.md#job-environment-variables .PHONY: test-multi-module test-multi-module: $(MYGOBIN)/prchecker - cd ./cmd/prchecker; go run . \ + $(MYGOBIN)/prchecker \ -owner=$(REPO_OWNER) \ -repo=$(REPO_NAME) \ -pr=$(PULL_NUMBER) \