From e88df4be26a9b745b4a7eae44a66bd2d753fca94 Mon Sep 17 00:00:00 2001 From: Dennis Wong Date: Thu, 9 Apr 2020 12:08:21 -0400 Subject: [PATCH 1/3] Fix for new resource version created when one file matches ignore and the other does not When paths and ignore_paths are both defined, and the pull request contains one file in the ignored path and another outside of paths and outside ignore_paths, a new resource version is created because the number of files matching ignore_paths does not match the number of files changed. This behaviour is incorrect because there may be changed files that do not match paths and do not match ignore_paths. --- check.go | 54 +++++++++-- pullrequest/filter.go | 38 ++++---- pullrequest/functional_test.go | 166 +++++++++++++++++++-------------- 3 files changed, 160 insertions(+), 98 deletions(-) diff --git a/check.go b/check.go index 2942369..bd8c730 100644 --- a/check.go +++ b/check.go @@ -48,15 +48,55 @@ func Check(request CheckRequest, manager Github) (CheckResponse, error) { log.Println("ignore paths configured:", iPaths) log.Println("changed files found:", p.Files) - switch { // if `paths` is configured && NONE of the changed files match `paths` pattern/s - case pullrequest.Patterns(paths)(p) && !pullrequest.Files(paths, false)(p): - log.Println("paths excluded pull") - continue + if pullrequest.Patterns(paths)(p) { + matches, err := pullrequest.Files(paths)(p.Files) + if err != nil { + log.Println("error identifying matching paths") + continue + } + + if len(matches) == 0 { + log.Println("paths excluded pull") + continue + } + } + // if `ignore_paths` is configured && ALL of the changed files match `ignore_paths` pattern/s - case pullrequest.Patterns(iPaths)(p) && pullrequest.Files(iPaths, true)(p): - log.Println("ignore paths excluded pull") - continue + if pullrequest.Patterns(iPaths)(p) { + matches, err := pullrequest.Files(iPaths)(p.Files) + if err != nil { + log.Println("error identifying matching ignore_paths") + continue + } + + if len(matches) == len(p.Files) { + log.Println("ignore paths excluded pull") + continue + } + } + + // Both `paths` and `ignore_paths` are defined, it is possible for the pull request + // to contain files outside of `paths` + if pullrequest.Patterns(paths)(p) && pullrequest.Patterns(iPaths)(p) { + log.Println("paths and ignore_paths both defined") + matches, err := pullrequest.Files(paths)(p.Files) + if err != nil { + log.Println("error identifying matching paths when both paths and ignore_paths are defined") + continue + } + + if len(matches) > 0 { + matches, err = pullrequest.Files(iPaths)(matches) + if err != nil { + log.Println("error identifying matching ignore_paths when both paths and ignore_paths are defined") + continue + } + } + + if len(matches) == 0 { + continue + } } } diff --git a/pullrequest/filter.go b/pullrequest/filter.go index 2f38026..4cbb2cc 100644 --- a/pullrequest/filter.go +++ b/pullrequest/filter.go @@ -3,6 +3,7 @@ package pullrequest import ( "log" "regexp" + "strings" "time" glob "github.com/sabhiram/go-gitignore" @@ -191,37 +192,30 @@ func Patterns(patterns []string) Filter { } } -// Files matches the PRs changed files against a set of glob patterns: -// (invert == false) returns true if patterns empty OR any of the changed files match the patterns -// (invert == true) returns true if all of the changed files match the patterns to ignore -func Files(patterns []string, invert bool) Filter { - return func(p PullRequest) bool { - matched := make([]int8, 0) +// Files returns the files that match against a set of glob patterns: +func Files(patterns []string) func([]string) ([]string, error) { + return func(files []string) ([]string, error) { + matches := make([]string, 0) + gc, err := glob.CompileIgnoreLines(patterns...) if err != nil { - return false + return matches, err } - for _, f := range p.Files { + for _, f := range files { log.Println("comparing patterns to changed file:", f) - if gc.MatchesPath("/" + f) { - if !invert { - log.Println("paths: true -", "matched:", f) - return true - } - - matched = append(matched, 1) - log.Println("matched:", f) + fn := f + if !strings.HasPrefix(f, "/") { + fn = "/" + f } - } - log.Println("invert:", invert, "- matched:", len(matched), "- files:", len(p.Files)) - if invert && len(matched) == len(p.Files) { - log.Println("ignore paths: true") - return true + if gc.MatchesPath(fn) { + matches = append(matches, f) + } } - return false + log.Printf("found %d matching files", len(matches)) + return matches, nil } } diff --git a/pullrequest/functional_test.go b/pullrequest/functional_test.go index cd31eca..c3458a1 100644 --- a/pullrequest/functional_test.go +++ b/pullrequest/functional_test.go @@ -692,125 +692,153 @@ func TestFiles(t *testing.T) { tests := []struct { description string patterns []string - invert bool - pull pullrequest.PullRequest - expect bool + files []string + expect []string }{ { description: "match txt files @ root level", patterns: []string{"*.txt"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "file1.txt", - }, + files: []string{ + "file1.txt", + }, + expect: []string{ + "file1.txt", }, - expect: true, }, { description: "match txt files at any level", patterns: []string{"**/*.txt"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "test/file2.txt", - "test/testing/file2.txt", - "test/testing/tested/file2.txt", - }, + files: []string{ + "test/file2.txt", + "test/testing/file2.txt", + "test/testing/tested/file2.txt", + }, + expect: []string{ + "test/file2.txt", + "test/testing/file2.txt", + "test/testing/tested/file2.txt", }, - expect: true, }, { description: "match any file in test dir", patterns: []string{"test/*"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "file1.txt", - "test/file2.txt", - }, + files: []string{ + "file1.txt", + "test/file2.txt", + }, + expect: []string{ + "test/file2.txt", }, - expect: true, }, { description: "match any file recursively in test dir", patterns: []string{"test/**"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "file1.txt", - "test/testing/file2.txt", - "test/testing/tested/file2.txt", - }, + files: []string{ + "file1.txt", + "test/testing/file2.txt", + "test/testing/tested/file2.txt", + }, + expect: []string{ + "test/testing/file2.txt", + "test/testing/tested/file2.txt", + }, + }, + // { + // description: "no match in test subdirectory", + // patterns: []string{"test/**"}, + // files: []string{ + // "file1.txt", + // "testing/test/file2.txt", + // "testing/test/tested/file2.txt", + // }, + // expect: []string{}, + // }, + { + description: "No match in test root directory", + patterns: []string{"/test/**"}, + files: []string{ + "file1.txt", + "testing/test/file2.txt", + "testing/test/tested/file2.txt", + }, + expect: []string{}, + }, + { + description: "Match in test root", + patterns: []string{"/test/*"}, + files: []string{ + "file1.txt", + "testing/test/file2.txt", + "testing/test/tested/file2.txt", + "/test/file3.txt", + }, + expect: []string{ + "/test/file3.txt", }, - expect: true, }, { description: "match multiple files", patterns: []string{"ci/dockerfiles/**/*", "ci/dockerfiles/*", "ci/tasks/build-image.yml", "ci/pipelines/images.yml"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "ci/Makefile", - "ci/pipelines/images.yml", - "terraform/Makefile", - }, + files: []string{ + "ci/Makefile", + "ci/pipelines/images.yml", + "terraform/Makefile", + }, + expect: []string{ + "ci/pipelines/images.yml", }, - expect: true, }, { description: "no match /**/*", patterns: []string{"/ci/**/*"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "src/teams/myteam/README.md", - "src/teams/myteam/ci/pipeline.yml", - }, + files: []string{ + "src/teams/myteam/README.md", + "src/teams/myteam/ci/pipeline.yml", }, - expect: false, + expect: []string{}, }, { description: "match /**/*", patterns: []string{"/ci/**/*"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "ci/README.md", - "ci/pipeline.yml", - }, + files: []string{ + "ci/README.md", + "ci/pipeline.yml", + }, + expect: []string{ + "ci/README.md", + "ci/pipeline.yml", }, - expect: true, }, { description: "match **/*", patterns: []string{"src/teams/myteam/**/*"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "src/teams/myteam/README.md", - "src/teams/myteam/ci/pipeline.yml", - }, + files: []string{ + "src/teams/myteam/README.md", + "src/teams/myteam/ci/pipeline.yml", + }, + expect: []string{ + "src/teams/myteam/README.md", + "src/teams/myteam/ci/pipeline.yml", }, - expect: true, }, { description: "match multiple files 2", patterns: []string{"src/teams/myteam/**/*", "src/teams/myteam/*"}, - invert: false, - pull: pullrequest.PullRequest{ - Files: []string{ - "src/teams/myteam/README.md", - "src/teams/myteam/ci/pipeline.yml", - }, + files: []string{ + "src/teams/myteam/README.md", + "src/teams/myteam/ci/pipeline.yml", + }, + expect: []string{ + "src/teams/myteam/README.md", + "src/teams/myteam/ci/pipeline.yml", }, - expect: true, }, } for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { - out := pullrequest.Files(tc.patterns, tc.invert)(tc.pull) + out, err := pullrequest.Files(tc.patterns)(tc.files) + assert.Nil(t, err) assert.Equal(t, tc.expect, out) }) } From d168dd39288805e1e2c2b1fbeb13dbd9c7b988e6 Mon Sep 17 00:00:00 2001 From: Dennis Wong Date: Thu, 9 Apr 2020 14:17:51 -0400 Subject: [PATCH 2/3] Remove files that match the ignore_paths filter --- check.go | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/check.go b/check.go index bd8c730..00ad9d1 100644 --- a/check.go +++ b/check.go @@ -87,11 +87,13 @@ func Check(request CheckRequest, manager Github) (CheckResponse, error) { } if len(matches) > 0 { - matches, err = pullrequest.Files(iPaths)(matches) + ignoreMatches, err := pullrequest.Files(iPaths)(matches) if err != nil { log.Println("error identifying matching ignore_paths when both paths and ignore_paths are defined") continue } + + matches = diff(matches, ignoreMatches) } if len(matches) == 0 { @@ -123,6 +125,30 @@ func Check(request CheckRequest, manager Github) (CheckResponse, error) { return response, nil } +// diff returns elements in match that are not found in ignoreMatch +func diff(match []string, ignoreMatch []string) []string { + found := make(map[string]bool) + + for _, f := range match { + found[f] = true + } + + for _, f := range ignoreMatch { + if _, ok := found[f]; ok { + found[f] = false + } + } + + var diff []string + for file, ret := range found { + if ret { + diff = append(diff, file) + } + } + + return diff +} + func newVersion(r CheckRequest, p pullrequest.PullRequest) bool { switch { // negative filters From cce6c13dc0325fa8d19ec655b43a0f50b9d69a61 Mon Sep 17 00:00:00 2001 From: Dennis Wong Date: Thu, 9 Apr 2020 15:28:12 -0400 Subject: [PATCH 3/3] Add test for paths that end with trailing slash --- pullrequest/functional_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pullrequest/functional_test.go b/pullrequest/functional_test.go index c3458a1..74d76d5 100644 --- a/pullrequest/functional_test.go +++ b/pullrequest/functional_test.go @@ -833,6 +833,19 @@ func TestFiles(t *testing.T) { "src/teams/myteam/ci/pipeline.yml", }, }, + { + description: "match paths ending in file separator", + patterns: []string{"src/teams/myteam/Dockerfile", "src/teams/myteam/ci/", "src/teams/myteam/app/"}, + files: []string{ + "src/teams/myteam/ci/README.md", + "src/teams/myteam/ci/main.go", + "src/teams/yourteam/temp.go", + }, + expect: []string{ + "src/teams/myteam/ci/README.md", + "src/teams/myteam/ci/main.go", + }, + }, } for _, tc := range tests {