diff --git a/Makefile b/Makefile index 7886badbb..0511c3074 100644 --- a/Makefile +++ b/Makefile @@ -40,7 +40,6 @@ verify-kustomize: \ prow-presubmit-check: \ install-tools \ lint-kustomize \ - test-multi-module \ test-unit-kustomize-all \ test-unit-cmd-all \ test-go-mod \ @@ -83,11 +82,6 @@ $(MYGOBIN)/pluginator: cd cmd/pluginator; \ go install . -# Build from local source. -$(MYGOBIN)/prchecker: - cd cmd/prchecker; \ - go install . - # Build from local source. $(MYGOBIN)/kustomize: build-kustomize-api cd kustomize; \ @@ -102,7 +96,6 @@ install-tools: \ $(MYGOBIN)/k8scopy \ $(MYGOBIN)/mdrip \ $(MYGOBIN)/pluginator \ - $(MYGOBIN)/prchecker \ $(MYGOBIN)/stringer ### Begin kustomize plugin rules. @@ -252,19 +245,6 @@ test-unit-cmd-all: test-go-mod: ./hack/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: $(MYGOBIN)/prchecker - ( \ - export MYGOBIN=$(MYGOBIN); \ - export REPO_OWNER=$(REPO_OWNER); \ - export REPO_NAME=$(REPO_NAME); \ - export PULL_NUMBER=$(PULL_NUMBER); \ - export MODULES=$(MODULES); \ - ./hack/check-multi-module.sh; \ - ) - .PHONY: test-examples-e2e-kustomize: $(MYGOBIN)/mdrip $(MYGOBIN)/kind ( \ @@ -358,7 +338,6 @@ clean: clean-kustomize-external-go-plugin rm -f $(MYGOBIN)/golangci-lint-kustomize rm -f $(MYGOBIN)/kustomize rm -f $(MYGOBIN)/mdrip - rm -f $(MYGOBIN)/prchecker rm -f $(MYGOBIN)/stringer # Handle pluginator manually. diff --git a/cmd/prchecker/go.mod b/cmd/prchecker/go.mod deleted file mode 100644 index 38e6da9e5..000000000 --- a/cmd/prchecker/go.mod +++ /dev/null @@ -1,8 +0,0 @@ -module sigs.k8s.io/kustomize/cmd/prchecker - -go 1.16 - -require ( - github.com/google/go-github v17.0.0+incompatible - github.com/google/go-querystring v1.0.0 // indirect -) diff --git a/cmd/prchecker/go.sum b/cmd/prchecker/go.sum deleted file mode 100644 index c90713b76..000000000 --- a/cmd/prchecker/go.sum +++ /dev/null @@ -1,4 +0,0 @@ -github.com/google/go-github v17.0.0+incompatible h1:N0LgJ1j65A7kfXrZnUDaYCs/Sf4rEjNlfyDHW9dolSY= -github.com/google/go-github v17.0.0+incompatible/go.mod h1:zLgOLi98H3fifZn+44m+umXrS52loVEgC2AApnigrVQ= -github.com/google/go-querystring v1.0.0 h1:Xkwi/a1rcvNg1PPYe5vI8GbeBY/jrVuDX5ASuANWTrk= -github.com/google/go-querystring v1.0.0/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck= diff --git a/cmd/prchecker/main.go b/cmd/prchecker/main.go deleted file mode 100644 index 4459e0d61..000000000 --- a/cmd/prchecker/main.go +++ /dev/null @@ -1,271 +0,0 @@ -// prchecker examines pull requests -// -// - When a PR includes files from multiple modules that we'd rather not -// modify at the same time (in an effort to have more self-contained -// release notes), the script will exit with a non-zero exit code. -// -// Usage: -// -// go run . \ -// -owner=kubernetes-sigs \ -// -repo=kustomize \ -// -pr=2997 \ -// cmd/config api kustomize kyaml - -package main - -import ( - "context" - "flag" - "fmt" - "os" - "regexp" - "strings" - - "github.com/google/go-github/github" -) - -// splitCommitsHelp is a help message displayed when a PR has commits which -// span modules. -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. -// Pattern expects "ALLOW_MODULE_SPAN" in CAPS on a line by itself. -// Spaces may be included before or after but no other characters with one -// exception. ">" may be used to quote the exclusion. -// Pattern may be provided at any point in the pull request description. -// -// Ex: "ALLOW_MODULE_SPAN", "> ALLOW_MODULE_SPAN", " ALLOW_MODULE_SPAN " -const ignoreSpanPattern = "(?m)^>?\\s*ALLOW_MODULE_SPAN\\s*$" - -// Changeset represents a set of file modifications associated with a commit -type Changeset struct { - files []string - id string -} - -// GitHubRepository represents a pairing of the owner and repository to make -// passing around references to specific projects simpler -type GitHubRepository struct { - client *github.Client - owner *string - repo *string -} - -// GitHubService is the collection of GitHub API interactions this service consumes -type GitHubService interface { - GetPullRequest(prId int) (*github.PullRequest, *github.Response, error) - GetCommit(commitSha string) (*github.RepositoryCommit, *github.Response, error) - ListCommits(prId int, options *github.ListOptions) ([]*github.RepositoryCommit, *github.Response, error) -} - -// 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) -} - -// 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) -} - -// 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() { - owner := flag.String("owner", "", "the github repository owner name") - repo := flag.String("repo", "", "the github repository name") - pullrequest := flag.Int("pr", -1, "the pull request number") - flag.Parse() - // Treat all following arguments as restricted directories - restrictedPaths := flag.Args() - - // Short circuit check if restricted paths is less than 2 - // Conflicts won't exist in this scenario so we don't need to call the API - if len(restrictedPaths) <= 1 { - fmt.Println("Check not run. Add at least two restricted paths and run again.") - os.Exit(0) - } - - client := github.NewClient(nil) - - githubRepo := &GitHubRepository{ - client: client, - owner: owner, - repo: repo, - } - - // Check if module span is allowed before scanning on commits - isSpanAllowed, err := ModuleSpanAllowed(githubRepo, *pullrequest) - - if err != nil { - fmt.Printf("unable to retrieve pull request details: %v", err.Error()) - os.Exit(2) - } - - if isSpanAllowed { - fmt.Println("Check not run. Module spanning was allowed in this Pull Request.") - os.Exit(0) - } - - isSpanningPull, _, err := PullRequestSpanningPathList(githubRepo, *pullrequest, restrictedPaths) - - if err != nil { - fmt.Printf("unable to retrieve pull request details: %v", err) - os.Exit(2) - } - - // Exit with error if two or more restricted directories where modified - if isSpanningPull { - // Provide a suggestion for potential solution if the check fails. - fmt.Println(splitCommitsHelp) - os.Exit(1) - } -} - -// ConstructChangeset creates a changeset from a GitHub Commit object -func ConstructChangeset(commit *github.RepositoryCommit) *Changeset { - id := commit.SHA - fileset := []string{} - - for _, file := range commit.Files { - fileset = append(fileset, *file.Filename) - } - - return &Changeset{ - files: fileset, - id: *id, - } -} - -// StringAllowsModuleSpan tests if a string matches the allow span regex -func StringAllowsModuleSpan(body string) (bool, error) { - return regexp.MatchString(ignoreSpanPattern, body) -} - -// 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) { - - // Note: There are multiple ways to pull a github commit object - // we want a RepositoryCommit. - pullRequest, _, err := repository.GetPullRequest(pullId) - - if err != nil { - return false, err - } - - return StringAllowsModuleSpan(*pullRequest.Body) -} - -// 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) { - commit, _, err := repository.GetCommit(commitSha) - - if err != nil { - return nil, err - } - - return ConstructChangeset(commit), nil -} - -// GetPullRequestCommits constructs a list of all commits and the associated -// files changes in the given pull request -func GetPullRequestCommits(repository GitHubService, pullrequest int) ([]*Changeset, error) { - - // foundFiles across all pages from github api - var collectedCommits []*github.RepositoryCommit - // Github only returns a limited set of commits per request and PR's may - // exceed this so loop until all pages have been enumerated. - options := &github.ListOptions{Page: 1} - for options.Page != 0 { - commits, response, err := repository.ListCommits(pullrequest, options) - - // If an error has occurred while querying api exit early, report error - if err != nil { - return nil, err - } - collectedCommits = append(collectedCommits, commits...) - // setup next page to continue loop - options = &github.ListOptions{Page: response.NextPage} - } - - var changesetResults []*Changeset - for _, commit := range collectedCommits { - // The repository commits from list commits are not hydrated - // We will need to retrieve the complete object: - changeset, err := GetCommitChanges(repository, *commit.SHA) - if err != nil { - return nil, err - } - - changesetResults = append(changesetResults, changeset) - } - return changesetResults, nil -} - -// 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 - changesets, err := GetPullRequestCommits(repository, pullrequest) - - if err != nil { - return false, nil, err - } - - spanningChangesExist := false - for _, changeset := range changesets { - 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") - } - - fmt.Printf("\t* %s\n", changeset.id) - // In order provide a full list of outstanding commits, do not shortcircuit this check - spanningChangesExist = true - } - } - - return spanningChangesExist, changesets, nil -} - -// isSpanningPaths tests if a changeset is spanning -// multiple directory paths. -func (changeset *Changeset) isSpanningPaths(paths []string) bool { - matchedPath := "" - - for _, file := range changeset.files { - for _, path := range paths { - if strings.HasPrefix(file, path) { - // If a different path has already matched then the changeset spans multiple restricted paths - if matchedPath != "" && matchedPath != path { - return true - } - matchedPath = path - } - } - } - - return false -} diff --git a/cmd/prchecker/main_test.go b/cmd/prchecker/main_test.go deleted file mode 100644 index 88bea9c31..000000000 --- a/cmd/prchecker/main_test.go +++ /dev/null @@ -1,303 +0,0 @@ -package main - -import ( - "testing" - - "github.com/google/go-github/github" -) - -func TestStringAllowsModuleSpan(t *testing.T) { - var tests = []struct { - name string - body string - matchExpected bool - }{ - { - "exception not included", - "foo", - false, - }, - { - "exception mentioned in sentence does not exempt span check", - "don't ALLOW_MODULE_SPAN", - false, - }, - { - "PR body is just exception", - "ALLOW_MODULE_SPAN", - true, - }, - { - "support markdown quoting exception", - "> ALLOW_MODULE_SPAN", - true, - }, - { - "support whitespace padding", - "\t ALLOW_MODULE_SPAN\t ", - true, - }, - { - "module span exemption allowed at start of string", - "ALLOW_MODULE_SPAN\nat start of file", - true, - }, - { - "module span exemption allowed at end of string", - "at end of file\nALLOW_MODULE_SPAN", - true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := StringAllowsModuleSpan(tt.body) - - if err != nil { - t.Errorf(err.Error()) - } - - if result != tt.matchExpected { - t.Errorf("got %t, want %t", result, tt.matchExpected) - } - }) - } -} - -func TestIsModuleSpanAllowed(t *testing.T) { - var tests = []struct { - name string - body string - matchExpected bool - }{ - { - "module spanning not allowed", - "don't ALLOW_MODULE_SPAN", - false, - }, - { - "module spanning allowed", - "ALLOW_MODULE_SPAN", - true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - - client := FakeGitHubService{ - pullRequest: &github.PullRequest{ - Body: &tt.body, - }, - } - - result, _ := ModuleSpanAllowed(&client, 1) - - if result != tt.matchExpected { - t.Errorf("got %t, want %t", result, tt.matchExpected) - } - }) - } -} - -func TestGetCommitChanges(t *testing.T) { - client := FakeGitHubService{ - commit: &github.RepositoryCommit{ - SHA: github.String("abc123"), - Files: []github.CommitFile{{Filename: github.String("foo")}, {Filename: github.String("bar")}}, - }, - } - - result, _ := GetCommitChanges(&client, "abc123") - - if result.id != "abc123" { - t.Errorf("got %v, want %v", result.id, "abc123") - } - - if len(result.files) != 2 { - t.Errorf("got %v, want %v", len(result.files), "2") - } - - expectedFiles := []string{"foo", "bar"} - if !StringSliceAreEqual(result.files, expectedFiles) { - t.Errorf("got %v, want %v", result.files, expectedFiles) - } -} - -func TestGetPullRequestCommits(t *testing.T) { - client := FakeGitHubService{ - commit: &github.RepositoryCommit{ - SHA: github.String("abc123"), - Files: []github.CommitFile{{Filename: github.String("foo")}, {Filename: github.String("bar")}}, - }, - commitList: []*github.RepositoryCommit{ - { - SHA: github.String("abc123"), - }, - { - SHA: github.String("abc123"), - }, - }, - } - - result, _ := GetPullRequestCommits(&client, 42) - - if len(result) != 2 { - t.Errorf("got %v, want %v", len(result), 2) - } - - expectedFiles := []string{"foo", "bar"} - if !StringSliceAreEqual(result[0].files, expectedFiles) { - t.Errorf("[%d] got %v, want %v", 0, result[0].files, expectedFiles) - } - if !StringSliceAreEqual(result[1].files, expectedFiles) { - t.Errorf("[%d] got %v, want %v", 1, result[1].files, expectedFiles) - } -} - -func TestContstructingChangeset(t *testing.T) { - var tests = []struct { - name string - sha string - files []github.CommitFile - expected Changeset - }{ - { - "construct from single file", - "abc123", - []github.CommitFile{{Filename: github.String("foo")}}, - Changeset{id: "abc123", files: []string{"foo"}}, - }, - { - "construct from multiple files", - "1234", - []github.CommitFile{{Filename: github.String("foo")}, {Filename: github.String("bar")}}, - Changeset{id: "1234", files: []string{"foo", "bar"}}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - commit := &github.RepositoryCommit{SHA: github.String(tt.sha), Files: tt.files} - result := ConstructChangeset(commit) - - if tt.expected.id != result.id { - t.Errorf("got %v, want %v", result.id, tt.expected.id) - } - - if !StringSliceAreEqual(tt.expected.files, result.files) { - t.Errorf("got %v, want %v", result.files, tt.expected.files) - } - }) - } -} - -func TestIsChangesetSpanning(t *testing.T) { - var tests = []struct { - name string - changeset []string - files []string - expected bool - }{ - { - "matching sets of 1 element do not span", - []string{"1"}, - []string{"1"}, - false, - }, - { - "subdirectories do not match top level directories", - []string{"a/1"}, - []string{"1"}, - false, - }, - { - "distinct sets do not span", - []string{"1", "2"}, - []string{"a", "b"}, - false, - }, - { - "single matching path does not span", - []string{"1", "a"}, - []string{"1", "2"}, - false, - }, - { - "path and subdirectory of same restriction do not span", - []string{"1", "1/a"}, - []string{"1", "2", "3"}, - false, - }, - { - "matching sets span", - []string{"1", "2"}, - []string{"1", "2"}, - true, - }, - { - "superset of restricted paths spans", - []string{"1", "2", "3"}, - []string{"1", "2"}, - true, - }, - { - "subset of restricted paths spans", - []string{"1", "3"}, - []string{"1", "2", "3"}, - true, - }, - { - "subdirectories of restricted paths span", - []string{"1/a", "3/b"}, - []string{"1", "2", "3"}, - true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - - changeset := &Changeset{files: tt.changeset} - - result := changeset.isSpanningPaths(tt.files) - - if result != tt.expected { - t.Errorf("got %t, want %t", result, tt.expected) - } - }) - } -} - -type FakeGitHubService struct { - pullRequest *github.PullRequest - commit *github.RepositoryCommit - commitList []*github.RepositoryCommit -} - -func (repository FakeGitHubService) GetPullRequest(prId int) (*github.PullRequest, *github.Response, error) { - return repository.pullRequest, nil, nil -} - -func (repository FakeGitHubService) GetCommit(commitSha string) (*github.RepositoryCommit, *github.Response, error) { - return repository.commit, nil, nil - -} - -func (repository FakeGitHubService) ListCommits(prId int, options *github.ListOptions) ([]*github.RepositoryCommit, *github.Response, error) { - return repository.commitList, &github.Response{NextPage: 0}, nil -} - -func StringSliceAreEqual(left []string, right []string) bool { - if len(left) != len(right) { - return false - } - - for i, elem := range left { - if elem != right[i] { - return false - } - } - - return true -} diff --git a/hack/check-multi-module.sh b/hack/check-multi-module.sh deleted file mode 100755 index 8e75e0793..000000000 --- a/hack/check-multi-module.sh +++ /dev/null @@ -1,23 +0,0 @@ -#!/usr/bin/env bash - -set -e - -if [[ "$PULL_NUMBER" -ne "" ]]; then - cmd="$MYGOBIN/prchecker - -owner=$REPO_OWNER - -repo=$REPO_NAME - -pr=$PULL_NUMBER - $MODULES" - - - echo $MYGOBIN - echo $REPO_OWNER - echo $REPO_NAME - echo $PULL_NUMBER - echo $MODULES - eval $cmd -else - echo "Multi-module check skipped. No PULL_NUMBER provided. - -To run this check locally set PULL_NUMBER to the PR ID from GitHub." -fi