Skip to content

Commit 9f1c080

Browse files
author
Christopher Mancini
committed
refactor version selection + more
goal was to make version selection more robust, easier to test and reduce the chance legitimate versions are missed by concourse * add support for schema previews * add support to trigger pr via comment * convert files to use graphql api * logging * use glob path filtering * decouple internal PullRequest from GitHub PullRequestObject * add env var support to context param * add head short ref to metadata
1 parent 94d3f6b commit 9f1c080

31 files changed

+2236
-1015
lines changed

‎Dockerfile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ ADD . /go/src/github.com/telia-oss/github-pr-resource
33
WORKDIR /go/src/github.com/telia-oss/github-pr-resource
44
RUN go get -u -v github.com/go-task/task/cmd/task && task build
55

6-
FROM alpine:3.8 as resource
6+
FROM alpine:3.10 as resource
77
COPY --from=builder /go/src/github.com/telia-oss/github-pr-resource/build /opt/resource
88
RUN apk add --update --no-cache \
99
git \

‎README.md

Lines changed: 52 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,20 @@ Make sure to check out [#migrating](#migrating) to learn more.
1919

2020
## Source Configuration
2121

22-
| Parameter | Required | Example | Description |
23-
|-------------------------|----------|----------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
24-
| `repository` | Yes | `itsdalmo/test-repository` | The repository to target. |
25-
| `access_token` | Yes | | A Github Access Token with repository access (required for setting status on commits). N.B. If you want github-pr-resource to work with a private repository. Set `repo:full` permissions on the access token you create on GitHub. If it is a public repository, `repo:status` is enough. |
26-
| `v3_endpoint` | No | `https://api.github.com` | Endpoint to use for the V3 Github API (Restful). |
27-
| `v4_endpoint` | No | `https://api.github.com/graphql` | Endpoint to use for the V4 Github API (Graphql). |
28-
| `paths` | No | `terraform/*/*.tf` | Only produce new versions if the PR includes changes to files that match one or more glob patterns or prefixes. |
29-
| `ignore_paths` | No | `.ci/` | Inverse of the above. Pattern syntax is documented in [filepath.Match](https://golang.org/pkg/path/filepath/#Match), or a path prefix can be specified (e.g. `.ci/` will match everything in the `.ci` directory). |
30-
| `disable_ci_skip` | No | `true` | Disable ability to skip builds with `[ci skip]` and `[skip ci]` in commit message or pull request title. |
31-
| `skip_ssl_verification` | No | `true` | Disable SSL/TLS certificate validation on git and API clients. Use with care! |
32-
| `disable_forks` | No | `true` | Disable triggering of the resource if the pull request's fork repository is different to the configured repository. |
33-
| `git_crypt_key` | No | `AEdJVENSWVBUS0VZAAAAA...` | Base64 encoded git-crypt key. Setting this will unlock / decrypt the repository with git-crypt. To get the key simply execute `git-crypt export-key -- - | base64` in an encrypted repository. |
34-
| `base_branch` | No | `master` | Name of a branch. The pipeline will only trigger on pull requests against the specified branch. |
22+
| Parameter | Required | Example | Description |
23+
|-------------------------|----------|----------------------------------|--------------|
24+
| `repository` | Yes | `itsdalmo/test-repository` | The repository to target |
25+
| `access_token` | Yes | | A Github Access Token with repository access (required for setting status on commits). N.B. If you want github-pr-resource to work with a private repository. Set `repo:full` permissions on the access token you create on GitHub. If it is a public repository, `repo:status` is enough |
26+
| `v3_endpoint` | NO | `https://api.github.com` | Endpoint to use for the V3 Github API (Restful) |
27+
| `v4_endpoint` | NO | `https://api.github.com/graphql` | Endpoint to use for the V4 Github API (Graphql) |
28+
| `paths` | No | `terraform/*/*.tf` | Only produce new versions if the PR includes changes to files that match one or more glob patterns or prefixes |
29+
| `ignore_paths` | No | `.ci/` | Inverse of the above. Pattern syntax is documented in [filepath.Match](https://golang.org/pkg/path/filepath/#Match), or a path prefix can be specified (e.g. `.ci/` will match everything in the `.ci` directory) |
30+
| `disable_ci_skip` | No | `true` | Disable ability to skip builds with `[ci skip]` and `[skip ci]` in commit message or pull request title |
31+
| `skip_ssl_verification` | No | `true` | Disable SSL/TLS certificate validation on git and API clients. Use with care! |
32+
| `disable_forks` | No | `true` | Disable triggering of the resource if the pull request's fork repository is different to the configured repository |
33+
| `git_crypt_key` | No | `AEdJVENSWVBUS0VZAAAAA...` | Base64 encoded git-crypt key. Setting this will unlock / decrypt the repository with git-crypt. To get the key simply execute `git-crypt export-key -- - | base64` in an encrypted repository. |
34+
| `base_branch` | No | `master` | Name of a branch. The pipeline will only trigger on pull requests against the specified branch |
35+
| `preview_schema` | No | `true` | if enabled, an `Accept: application/vnd.github.starfire-preview+json` header will be appended to each request to enable preview schema's that are hidden behind a feature flag on GitHub |
3536

3637
Notes:
3738
- If `v3_endpoint` is set, `v4_endpoint` must also be set (and the other way around).
@@ -45,13 +46,46 @@ Notes:
4546
Produces new versions for all commits (after the last version) ordered by the committed date.
4647
A version is represented as follows:
4748

48-
- `pr`: The pull request number.
49-
- `commit`: The commit SHA.
50-
- `committed`: Timestamp of when the commit was committed. Used to filter subsequent checks.
49+
- `pr`: The pull request number
50+
- `commit`: The commit SHA
51+
- `updated`: Timestamp of when the pull request was last updated at the time of the check
5152

52-
If several commits are pushed to a given PR at the same time, the last commit will be the new version.
53+
If several commits are pushed to a given PR at the same time, the PR with the latest updated at will be the newest version.
54+
55+
#### search
56+
57+
The GraphQL search for pull requests uses the `Search` endpoint and follows the following pattern:
58+
59+
`is:pr is:open repo:%s/%s updated:>%s sort:updated`
60+
61+
Which means that we want to search for only OPEN PULL REQUESTS that have been UPDATED since the latest `updated` timestamp of the last check. To test this query, you can simply use the search box in the navigation of github.com.
62+
63+
Then, we use the [PullRequestTimelineItemsConnection](https://developer.github.com/v4/object/pullrequesttimelineitemsconnection/) to fetch all commits / events on the PRs timeline since the latest `updated` timestamp of the last check. This allows us to iterate over the pull requests and filter them as is covered in the next section.
64+
65+
#### filters
66+
67+
There are many ways by which this resource filters pull requests and each filter is a function in the `filter.go` file within the `pullrequest` package. This makes it very easy to test the functionality of each filter. There are two types:
68+
69+
* negative filters which when return true, the pull request should be excluded (skipped) from versions
70+
* positive filters which when return true, the pull request should be included from versions
71+
72+
Current negative filters:
73+
74+
* `pullrequest.SkipCI` which will exclude PRs containing `[skip ci|ci skip]` in the PR Title / Message
75+
* `pullrequest.BaseBranch` which will exclude PRs where the base branch (e.g. `master`) does not match the source configuration
76+
* `pullrequest.Fork` which will exclude PRs from forks when `disable_forks` is configured true
77+
78+
Current positive filters:
79+
* `pullrequest.Created` which will include PRs with `Created == Updated` OR `Created > HeadRef.Commited | Authored | Pushed`
80+
* `pullrequest.BaseRefChanged` which will include PRs where a [BaseRefChanged](https://developer.github.com/v4/object/baserefchangedevent/) occurred
81+
* `pullrequest.BaseRefForcePushed` which will include PRs where a [BaseRefForcePushed](https://developer.github.com/v4/object/baserefforcepushedevent) occurred
82+
* `pullrequest.HeadRefForcePushed` which will include PRs where a [HeadRefForcePushed](https://developer.github.com/v4/object/headrefforcepushedevent) occurred
83+
* `pullrequest.Reopened` which will include PRs where a [BaseRefChanged](https://developer.github.com/v4/object/reopenedevent) occurred
84+
* `pullrequest.BuildCI` which will include PRs with a new comment containing `[build ci|ci build]`
85+
* `pullrequest.NewCommits` which will include PRs with a new commit since the last `updated` timestamp of the last check
5386

5487
**Note on webhooks:**
88+
5589
This resource does not implement any caching, so it should work well with webhooks (should be subscribed to `push` and `pull_request` events).
5690
One thing to keep in mind however, is that pull requests that are opened from a fork and commits to said fork will not
5791
generate notifications over the webhook. So if you have a repository with little traffic and expect pull requests from forks,
@@ -77,7 +111,7 @@ requested version and the metadata emitted by `get` are available to your tasks
77111
- `.git/resource/changed_files` (if enabled by `list_changed_files`)
78112

79113
The information in `metadata.json` is also available as individual files in the `.git/resource` directory, e.g. the `base_sha`
80-
is available as `.git/resource/base_sha`. For a complete list of available (individual) metadata files, please check the code
114+
is available as `.git/resource/base_sha`. For a complete list of available (individual) metadata files, please check the code
81115
[here](https://github.com/telia-oss/github-pr-resource/blob/master/in.go#L66).
82116

83117
When specifying `skip_download` the pull request volume mounted to subsequent tasks will be empty, which is a problem

‎Taskfile.yml

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,12 @@ tasks:
2727
desc: Run test suite
2828
deps: [generate]
2929
cmds:
30-
- gofmt -s -l -w .
31-
- go vet -v ./...
32-
- go test -race -v ./...
30+
- |
31+
{{ if ne (env "SKIP_TEST") "true" }}
32+
gofmt -s -l -w .
33+
go vet -v ./...
34+
go test -race -v -cover ./...
35+
{{ end }}
3336
3437
e2e:
3538
desc: Run E2E test suite

‎check.go

Lines changed: 65 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -2,154 +2,113 @@ package resource
22

33
import (
44
"fmt"
5-
"path/filepath"
6-
"regexp"
5+
"log"
76
"sort"
8-
"strings"
7+
"time"
8+
9+
"github.com/telia-oss/github-pr-resource/pullrequest"
910
)
1011

12+
func findPulls(since time.Time, gh Github) ([]pullrequest.PullRequest, error) {
13+
if since.IsZero() {
14+
return gh.GetLatestOpenPullRequest()
15+
}
16+
return gh.ListOpenPullRequests(since)
17+
}
18+
1119
// Check (business logic)
1220
func Check(request CheckRequest, manager Github) (CheckResponse, error) {
1321
var response CheckResponse
1422

15-
pulls, err := manager.ListOpenPullRequests()
23+
pulls, err := findPulls(request.Version.UpdatedDate, manager)
1624
if err != nil {
1725
return nil, fmt.Errorf("failed to get last commits: %s", err)
1826
}
1927

20-
disableSkipCI := request.Source.DisableCISkip
28+
paths := request.Source.Paths
29+
iPaths := request.Source.IgnorePaths
2130

22-
Loop:
23-
for _, p := range pulls {
24-
// [ci skip]/[skip ci] in Pull request title
25-
if !disableSkipCI && ContainsSkipCI(p.Title) {
26-
continue
27-
}
28-
// [ci skip]/[skip ci] in Commit message
29-
if !disableSkipCI && ContainsSkipCI(p.Tip.Message) {
30-
continue
31-
}
32-
// Filter pull request if the BaseBranch does not match the one specified in source
33-
if request.Source.BaseBranch != "" && p.PullRequestObject.BaseRefName != request.Source.BaseBranch {
34-
continue
35-
}
36-
// Filter out commits that are too old.
37-
if !p.Tip.CommittedDate.Time.After(request.Version.CommittedDate) {
38-
continue
39-
}
31+
log.Println("total pulls found:", len(pulls))
4032

41-
if request.Source.DisableForks && p.IsCrossRepository {
33+
for _, p := range pulls {
34+
log.Printf("evaluate pull: %+v\n", p)
35+
if !newVersion(request, p) {
36+
log.Println("no new version found")
4237
continue
4338
}
4439

45-
// Fetch files once if paths/ignore_paths are specified.
46-
var files []string
47-
48-
if len(request.Source.Paths) > 0 || len(request.Source.IgnorePaths) > 0 {
49-
files, err = manager.ListModifiedFiles(p.Number)
40+
if len(paths)+len(iPaths) > 0 {
41+
log.Println("pattern/s configured")
42+
p.Files, err = pullRequestFiles(p.Number, manager)
5043
if err != nil {
51-
return nil, fmt.Errorf("failed to list modified files: %s", err)
44+
return nil, err
5245
}
53-
}
5446

55-
// Skip version if no files match the specified paths.
56-
if len(request.Source.Paths) > 0 {
57-
var wanted []string
58-
for _, pattern := range request.Source.Paths {
59-
w, err := FilterPath(files, pattern)
60-
if err != nil {
61-
return nil, fmt.Errorf("path match failed: %s", err)
62-
}
63-
wanted = append(wanted, w...)
64-
}
65-
if len(wanted) == 0 {
66-
continue Loop
47+
log.Println("paths configured:", paths)
48+
log.Println("ignore paths configured:", iPaths)
49+
log.Println("changed files found:", p.Files)
50+
51+
switch {
52+
case pullrequest.Patterns(paths)(p) && !pullrequest.Files(paths, false)(p):
53+
log.Println("paths excluded pull")
54+
continue
55+
case !pullrequest.Patterns(paths)(p) && pullrequest.Patterns(iPaths)(p) && pullrequest.Files(iPaths, true)(p):
56+
log.Println("ignore paths excluded pull")
57+
continue
6758
}
6859
}
6960

70-
// Skip version if all files are ignored.
71-
if len(request.Source.IgnorePaths) > 0 {
72-
wanted := files
73-
for _, pattern := range request.Source.IgnorePaths {
74-
wanted, err = FilterIgnorePath(wanted, pattern)
75-
if err != nil {
76-
return nil, fmt.Errorf("ignore path match failed: %s", err)
77-
}
78-
}
79-
if len(wanted) == 0 {
80-
continue Loop
81-
}
82-
}
8361
response = append(response, NewVersion(p))
8462
}
8563

8664
// Sort the commits by date
8765
sort.Sort(response)
8866

8967
// If there are no new but an old version = return the old
90-
if len(response) == 0 && request.Version.PR != "" {
68+
if len(response) == 0 && request.Version.PR != 0 {
69+
log.Println("no new versions, use old")
9170
response = append(response, request.Version)
9271
}
72+
9373
// If there are new versions and no previous = return just the latest
94-
if len(response) != 0 && request.Version.PR == "" {
74+
if len(response) != 0 && request.Version.PR == 0 {
9575
response = CheckResponse{response[len(response)-1]}
9676
}
97-
return response, nil
98-
}
9977

100-
// ContainsSkipCI returns true if a string contains [ci skip] or [skip ci].
101-
func ContainsSkipCI(s string) bool {
102-
re := regexp.MustCompile("(?i)\\[(ci skip|skip ci)\\]")
103-
return re.MatchString(s)
104-
}
105-
106-
// FilterIgnorePath ...
107-
func FilterIgnorePath(files []string, pattern string) ([]string, error) {
108-
var out []string
109-
for _, file := range files {
110-
match, err := filepath.Match(pattern, file)
111-
if err != nil {
112-
return nil, err
113-
}
114-
if !match && !IsInsidePath(pattern, file) {
115-
out = append(out, file)
116-
}
117-
}
118-
return out, nil
119-
}
78+
log.Println("version count in response:", len(response))
79+
log.Println("versions:", response)
12080

121-
// FilterPath ...
122-
func FilterPath(files []string, pattern string) ([]string, error) {
123-
var out []string
124-
for _, file := range files {
125-
match, err := filepath.Match(pattern, file)
126-
if err != nil {
127-
return nil, err
128-
}
129-
if match || IsInsidePath(pattern, file) {
130-
out = append(out, file)
131-
}
132-
}
133-
return out, nil
81+
return response, nil
13482
}
13583

136-
// IsInsidePath checks whether the child path is inside the parent path.
137-
//
138-
// /foo/bar is inside /foo, but /foobar is not inside /foo.
139-
// /foo is inside /foo, but /foo is not inside /foo/
140-
func IsInsidePath(parent, child string) bool {
141-
if parent == child {
84+
func newVersion(r CheckRequest, p pullrequest.PullRequest) bool {
85+
switch {
86+
// negative filters
87+
case pullrequest.SkipCI(r.Source.DisableCISkip)(p),
88+
pullrequest.BaseBranch(r.Source.BaseBranch)(p),
89+
pullrequest.Fork(r.Source.DisableForks)(p):
90+
return false
91+
// positive filters
92+
case pullrequest.Created()(p),
93+
pullrequest.BaseRefChanged()(p),
94+
pullrequest.BaseRefForcePushed()(p),
95+
pullrequest.HeadRefForcePushed()(p),
96+
pullrequest.Reopened()(p),
97+
pullrequest.BuildCI()(p),
98+
pullrequest.NewCommits(r.Version.UpdatedDate)(p):
14299
return true
143100
}
144101

145-
// we add a trailing slash so that we only get prefix matches on a
146-
// directory separator
147-
parentWithTrailingSlash := parent
148-
if !strings.HasSuffix(parentWithTrailingSlash, string(filepath.Separator)) {
149-
parentWithTrailingSlash += string(filepath.Separator)
102+
return false
103+
}
104+
105+
func pullRequestFiles(n int, manager Github) ([]string, error) {
106+
files, err := manager.GetChangedFiles(n)
107+
if err != nil {
108+
return nil, fmt.Errorf("failed to list modified files: %s", err)
150109
}
151110

152-
return strings.HasPrefix(child, parentWithTrailingSlash)
111+
return files, nil
153112
}
154113

155114
// CheckRequest ...
@@ -166,7 +125,7 @@ func (r CheckResponse) Len() int {
166125
}
167126

168127
func (r CheckResponse) Less(i, j int) bool {
169-
return r[j].CommittedDate.After(r[i].CommittedDate)
128+
return r[j].UpdatedDate.After(r[i].UpdatedDate)
170129
}
171130

172131
func (r CheckResponse) Swap(i, j int) {

0 commit comments

Comments
 (0)