diff --git a/README.md b/README.md index e104a99..adc17fd 100644 --- a/README.md +++ b/README.md @@ -39,30 +39,46 @@ Make sure to check out [#migrating](#migrating) to learn more. | `disable_forks` | No | `true` | Disable triggering of the resource if the pull request's fork repository is different to the configured repository. | | `ignore_drafts` | No | `false` | Disable triggering of the resource if the pull request is in Draft status. | | `required_review_approvals` | No | `2` | Disable triggering of the resource if the pull request does not have at least `X` approved review(s). | -| `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. | +| `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. | | `base_branch` | No | `master` | Name of a branch. The pipeline will only trigger on pull requests against the specified branch. | | `labels` | No | `["bug", "enhancement"]` | The labels on the PR. The pipeline will only trigger on pull requests having at least one of the specified labels. | | `disable_git_lfs` | No | `true` | Disable Git LFS, skipping an attempt to convert pointers of files tracked into their corresponding objects when checked out into a working copy. | | `states` | No | `["OPEN", "MERGED"]` | The PR states to select (`OPEN`, `MERGED` or `CLOSED`). The pipeline will only trigger on pull requests matching one of the specified states. Default is ["OPEN"]. | Notes: - - If `v3_endpoint` is set, `v4_endpoint` must also be set (and the other way around). - - Look at the [Concourse Resources documentation](https://concourse-ci.org/resources.html#resource-webhook-token) + +- If `v3_endpoint` is set, `v4_endpoint` must also be set (and the other way around). +- Look at the [Concourse Resources documentation](https://concourse-ci.org/resources.html#resource-webhook-token) for webhook token configuration. - - When using `required_review_approvals`, you may also want to enable GitHub's branch protection rules to [dismiss stale pull request approvals when new commits are pushed](https://help.github.com/en/articles/enabling-required-reviews-for-pull-requests). +- When using `required_review_approvals`, you may also want to enable GitHub's branch protection rules to [dismiss stale pull request approvals when new commits are pushed](https://help.github.com/en/articles/enabling-required-reviews-for-pull-requests). ## Behaviour -#### `check` +### `check` -Produces new versions for all commits (after the last version) ordered by the committed date. +Produces a new version for the tip of every pull request pushed since the last version, ordered by when it was pushed. A version is represented as follows: - `pr`: The pull request number. - `commit`: The commit SHA. -- `committed`: Timestamp of when the commit was committed. Used to filter subsequent checks. +- `committed`: Timestamp of when the commit was committed. +- `changed`: Timestamp the version is ordered by, and used to filter subsequent checks: when the commit was pushed to + the pull request's branch, taken from the [repository activity API](https://docs.github.com/en/rest/repos/repos#list-repository-activities), + but never earlier than the pull request was opened, closed or merged. +- `state`: The pull request state. + +Ordering by the push rather than the committed date matters because Concourse only schedules `version: every` inputs +ranked above the newest version a job has already built: a rebased commit keeps an old committer date, so ordering by +it would rank the commit below versions that were already built and the push would never be scheduled. GitHub's +GraphQL API no longer populates `Commit.pushedDate`, hence the activity API. If several commits are pushed to a given PR at the same time, the last commit will be the new version. +Only open pull requests updated since the last version are looked up (one REST call each); a failed lookup fails the +check so that no push is skipped past. Pull requests from forks have no activity in the base repository, and a push +GitHub has not recorded cannot be found, so those are ordered by their committed date instead. + +A version that only names a pull request, as in `fly check-resource --from pr:123`, produces just that pull request's +current tip, ordered as the newest version regardless of its dates. **Note on webhooks:** This resource does not implement any caching, so it should work well with webhooks (should be subscribed to `push` and `pull_request` events). @@ -71,7 +87,7 @@ generate notifications over the webhook. So if you have a repository with little you'll need to discover those versions with `check_every: 1m` for instance. `check` in this resource is not a costly operation, so normally you should not have to worry about the rate limit. -#### `get` +### `get` | Parameter | Required | Example | Description | |----------------------|----------|----------|------------------------------------------------------------------------------------| @@ -87,13 +103,14 @@ into master. This ensures that we are both testing and setting status on the exa input. Because the base of the PR is not locked to a specific commit in versions emitted from `check`, a fresh `get` will always use the latest commit in master and *report the SHA of said commit in the metadata*. Both the requested version and the metadata emitted by `get` are available to your tasks as JSON: + - `.git/resource/version.json` - `.git/resource/metadata.json` - `.git/resource/changed_files` (if enabled by `list_changed_files`) The information in `metadata.json` is also available as individual files in the `.git/resource` directory, e.g. the `base_sha` -is available as `.git/resource/base_sha`. For a complete list of available (individual) metadata files, please check the code -[here](https://github.com/telia-oss/github-pr-resource/blob/master/in.go#L66). +is available as `.git/resource/base_sha`. For a complete list of available (individual) metadata files, please check +[the metadata written by `in.go`](https://github.com/telia-oss/github-pr-resource/blob/master/in.go#L66). When specifying `skip_download` the pull request volume mounted to subsequent tasks will be empty, which is a problem when you set e.g. the pending status before running the actual tests. The workaround for this is to use an alias for @@ -116,8 +133,7 @@ the base, Concourse will reuse the volume (i.e. not trigger a new `get`) if it s unexpected results (#5). As such, re-testing a PR against a newer version of the base is best done by *pushing an empty commit to the PR*. - -#### `put` +### `put` | Parameter | Required | Example | Description | |----------------------------|----------|--------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------| @@ -197,11 +213,15 @@ translates to 5000 requests, whereas for the V4 API (GraphQL) the calculation i https://developer.github.com/v4/guides/resource-limitations/#calculating-a-rate-limit-score-before-running-the-call Ref the above, here are some examples of running `check` against large repositories and the cost of doing so: + - [concourse/concourse](https://github.com/concourse/concourse): 51 open pull requests at the time of testing. Cost 2. - [torvalds/linux](https://github.com/torvalds/linux): 305 open pull requests. Cost 8. - [kubernetes/kubernetes](https://github.com/kubernetes/kubernetes): 1072 open pull requests. Cost: 22. +`check` additionally spends one V3 API call per pull request updated since the last version, to find when its tip was pushed. + For the other two operations the costing is a bit easier: + - `get`: Fixed cost of 1. Fetches the pull request at the given commit. - `put`: Uses the V3 API and has a min cost of 1, +1 for each of `status`, `comment` and `comment_file` etc. @@ -209,13 +229,15 @@ For the other two operations the costing is a bit easier: If you are coming from [jtarchie/github-pullrequest-resource][original-resource], its important to know that this resource is inspired by *but not a drop-in replacement for* the original. Here are some important differences: -#### New parameters: +### New parameters + - `source`: - `v4_endpoint` (see description above) - `put`: - `comment` (see description above) -#### Parameters that have been renamed: +### Parameters that have been renamed + - `source`: - `repo` -> `repository` - `ci_skip` -> `disable_ci_skip` (the logic has been inverted and its `true` by default) @@ -228,7 +250,8 @@ If you are coming from [jtarchie/github-pullrequest-resource][original-resource] - `put`: - `comment` -> `comment_file` (because we added `comment`) -#### Parameters that are no longer needed: +### Parameters that are no longer needed + - `src`: - `uri`: We fetch the URI directly from the Github API instead. - `private_key`: We clone over HTTPS using the access token for authentication. @@ -238,7 +261,8 @@ If you are coming from [jtarchie/github-pullrequest-resource][original-resource] - `get`: - `fetch_merge`: We are opinionated and always do a fetch_merge. -#### Parameters that did not make it: +### Parameters that did not make it + - `src`: - `authorship_restriction` - `label` @@ -291,7 +315,7 @@ query { } ``` +### Clone/Fetch -#### Clone/Fetch Clone/fetch is defaulted to a `git_depth` : `1` This will double up to a `MaxDepth` if we can't find a common ancestor diff --git a/check.go b/check.go index 13ea53e..ac9b95c 100644 --- a/check.go +++ b/check.go @@ -2,12 +2,15 @@ package resource import ( "fmt" - "github.com/shurcooL/githubv4" + "log" "path/filepath" "regexp" "sort" + "strconv" "strings" "time" + + "github.com/shurcooL/githubv4" ) // Check (business logic) @@ -25,6 +28,14 @@ func Check(request CheckRequest, manager Github) (CheckResponse, error) { return nil, fmt.Errorf("failed to get last commits: %w", err) } + // A version that only names a pull request (fly check-resource --from pr:N) asks for that + // pull request's tip alone; it is not a version to order against or to return. + if request.Version.IsPullRequestHint() { + pulls = pullRequestNumbered(pulls, request.Version.PR) + request.Version = Version{} + } + previousPR, _ := strconv.Atoi(request.Version.PR) + disableSkipCI := request.Source.DisableCISkip Loop: @@ -44,33 +55,17 @@ Loop: continue } - versionTime := p.Age() - if len(request.Source.StatusFilters) > 0 { - for _, statusFilter := range request.Source.StatusFilters { - // "zero time - epoch = 0" - versionTime = time.Time{} - isValid := false - for _, prStatus := range p.Tip.Status.Contexts { - if prStatus.Context == statusFilter.Context && strings.EqualFold(prStatus.State, statusFilter.State) { - // requires the given status exists and that it matches the desired state - isValid = true - // set the versionTime to the latest time of all - // the status checks - - if prStatus.CreatedAt.After(versionTime) { - versionTime = prStatus.CreatedAt.Time - } - break - } - } - if !isValid { - continue Loop - } - } + versionTime, statusOK := statusVersionTime(p, request.Source.StatusFilters) + if !statusOK { + continue } - // Filter out commits that are too old. - if !p.Age().After(request.Version.ChangedDate) { + // Skip pull requests with no activity since the last version, and the one that produced + // it, before spending an API call on them. + if !p.UpdatedAt.After(request.Version.ChangedDate) { + continue + } + if p.Number == previousPR && p.Tip.OID == request.Version.Commit && p.State == request.Version.State && len(request.Source.StatusFilters) == 0 { continue } @@ -108,6 +103,20 @@ Loop: continue } + pushed, err := pushedDate(manager, p) + if err != nil { + return nil, err + } + changed := p.ChangedDate(pushed) + + // Filter out commits that are too old. + if !changed.After(request.Version.ChangedDate) { + continue + } + if len(request.Source.StatusFilters) == 0 { + versionTime = changed + } + // Fetch files once if paths/ignore_paths are specified. var files []string @@ -163,6 +172,60 @@ Loop: return response, nil } +func pullRequestNumbered(pulls []*PullRequest, number string) []*PullRequest { + for _, p := range pulls { + if strconv.Itoa(p.Number) == number { + return []*PullRequest{p} + } + } + return nil +} + +// statusVersionTime is the time of the newest status check the filters require, and whether the +// pull request's tip has every one of them. +func statusVersionTime(p *PullRequest, filters []StatusFilter) (time.Time, bool) { + var versionTime time.Time + for _, statusFilter := range filters { + // "zero time - epoch = 0" + versionTime = time.Time{} + isValid := false + for _, prStatus := range p.Tip.Status.Contexts { + if prStatus.Context == statusFilter.Context && strings.EqualFold(prStatus.State, statusFilter.State) { + // requires the given status exists and that it matches the desired state + isValid = true + // set the versionTime to the latest time of all + // the status checks + + if prStatus.CreatedAt.After(versionTime) { + versionTime = prStatus.CreatedAt.Time + } + break + } + } + if !isValid { + return time.Time{}, false + } + } + return versionTime, true +} + +// pushedDate is when the pull request's tip was pushed, or zero when the order does not depend on +// it: a fork's branch has no activity in this repository, a merged pull request is ordered by its +// merge, and a push GitHub has not recorded is ordered by its committer date. +func pushedDate(manager Github, p *PullRequest) (time.Time, error) { + if p.IsCrossRepository || p.State == githubv4.PullRequestStateMerged { + return time.Time{}, nil + } + pushed, found, err := manager.PushedDate(p.HeadRefName, p.Tip.OID) + if err != nil { + return time.Time{}, fmt.Errorf("pr %d: failed to look up when %s was pushed: %w", p.Number, p.Tip.OID, err) + } + if !found { + log.Printf("pr %d: no push of %s recorded on %s, ordering it by its committer date", p.Number, p.Tip.OID, p.HeadRefName) + } + return pushed, nil +} + // ContainsSkipCI returns true if a string contains [ci skip] or [skip ci]. func ContainsSkipCI(s string) bool { re := regexp.MustCompile("(?i)\\[(ci skip|skip ci)\\]") diff --git a/check_test.go b/check_test.go index da8fa20..2b5b02a 100644 --- a/check_test.go +++ b/check_test.go @@ -1,9 +1,11 @@ package resource_test import ( + "errors" "testing" "time" + "github.com/google/go-github/v81/github" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" resource "github.com/telia-oss/github-pr-resource" @@ -11,6 +13,8 @@ import ( ) var ( + checkBase = time.Date(2026, 10, 7, 0, 0, 0, 0, time.UTC) + testPullRequests = []*resource.PullRequest{ createTestPR(1, "master", true, false, 0, nil, false, githubv4.PullRequestStateOpen, []resource.StatusContext{}), createTestPR(2, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen, []resource.StatusContext{}), @@ -39,15 +43,153 @@ var ( } ) +// testVersion is the version check produces for a test pull request pushed when it was committed. +func testVersion(p *resource.PullRequest) resource.Version { + return resource.NewVersion(p, p.Tip.CommittedDate.Time) +} + +func datedPR(number int, committed, updated time.Time) *resource.PullRequest { + p := createTestPR(number, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen, nil) + p.Tip.CommittedDate = githubv4.DateTime{Time: committed} + p.UpdatedAt = githubv4.DateTime{Time: updated} + p.ClosedAt = githubv4.DateTime{Time: updated} + p.MergedAt = githubv4.DateTime{Time: updated} + return p +} + +type pushLookup struct { + at time.Time + missing bool + err error +} + func TestCheck(t *testing.T) { + rebased := datedPR(21, checkBase.Add(-5*24*time.Hour), checkBase.Add(-time.Hour)) + previous := datedPR(22, checkBase.Add(-2*24*time.Hour), checkBase.Add(-2*24*time.Hour)) + previousVersion := testVersion(previous) + + forked := datedPR(23, checkBase.Add(-time.Hour), checkBase.Add(-time.Hour)) + forked.IsCrossRepository = true + + reopened := datedPR(24, checkBase.Add(-5*24*time.Hour), checkBase.Add(-time.Hour)) + reopened.CreatedAt = githubv4.DateTime{Time: checkBase.Add(-time.Hour)} + + merged := datedPR(25, checkBase.Add(-5*24*time.Hour), checkBase.Add(-time.Hour)) + merged.State = githubv4.PullRequestStateMerged + + draft := datedPR(26, checkBase.Add(-time.Hour), checkBase.Add(-time.Hour)) + draft.IsDraft = true + + unrecorded := datedPR(27, checkBase.Add(-time.Hour), checkBase.Add(-time.Hour)) + commented := datedPR(previous.Number, previous.Tip.CommittedDate.Time, checkBase.Add(-time.Hour)) + tests := []struct { description string source resource.Source version resource.Version files [][]string pullRequests []*resource.PullRequest + pushed map[string]pushLookup + pushLookups *int expected resource.CheckResponse + wantErr bool }{ + { + description: "check orders a rebased commit by when it was pushed rather than committed", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: previousVersion, + pullRequests: []*resource.PullRequest{rebased, previous}, + pushed: map[string]pushLookup{rebased.Tip.OID: {at: checkBase.Add(-time.Hour)}}, + expected: resource.CheckResponse{resource.NewVersion(rebased, checkBase.Add(-time.Hour))}, + }, + { + description: "check skips pull requests with no activity since the last version without looking up pushes", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: previousVersion, + pullRequests: []*resource.PullRequest{datedPR(21, checkBase.Add(-5*24*time.Hour), checkBase.Add(-3*24*time.Hour)), previous}, + pushLookups: github.Ptr(0), + expected: resource.CheckResponse{previousVersion}, + }, + { + description: "check does not produce a version for a pull request that was only commented on", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: previousVersion, + pullRequests: []*resource.PullRequest{rebased, previous}, + pushed: map[string]pushLookup{rebased.Tip.OID: {at: checkBase.Add(-5 * 24 * time.Hour)}}, + expected: resource.CheckResponse{previousVersion}, + }, + { + description: "check does not look up the pull request that produced the previous version", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: previousVersion, + pullRequests: []*resource.PullRequest{commented}, + pushLookups: github.Ptr(0), + expected: resource.CheckResponse{previousVersion}, + }, + { + description: "check orders a pull request whose push is not recorded by its committer date", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: previousVersion, + pullRequests: []*resource.PullRequest{unrecorded, previous}, + pushed: map[string]pushLookup{unrecorded.Tip.OID: {missing: true}}, + expected: resource.CheckResponse{testVersion(unrecorded)}, + }, + { + description: "check fails rather than skipping a pull request when the push lookup fails", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: previousVersion, + pullRequests: []*resource.PullRequest{rebased, previous}, + pushed: map[string]pushLookup{rebased.Tip.OID: {err: errors.New("rate limited")}}, + wantErr: true, + }, + { + description: "check orders a fork's pull request by committer date without looking up pushes", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: previousVersion, + pullRequests: []*resource.PullRequest{forked, previous}, + pushLookups: github.Ptr(0), + expected: resource.CheckResponse{resource.NewVersion(forked, checkBase.Add(-time.Hour))}, + }, + { + description: "check orders a pull request opened after its branch was pushed by its creation date", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: previousVersion, + pullRequests: []*resource.PullRequest{reopened, previous}, + pushed: map[string]pushLookup{reopened.Tip.OID: {at: checkBase.Add(-5 * 24 * time.Hour)}}, + expected: resource.CheckResponse{resource.NewVersion(reopened, checkBase.Add(-time.Hour))}, + }, + { + description: "check orders a merged pull request by when it was merged", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken", States: []githubv4.PullRequestState{githubv4.PullRequestStateMerged}}, + version: previousVersion, + pullRequests: []*resource.PullRequest{merged}, + pushLookups: github.Ptr(0), + expected: resource.CheckResponse{resource.NewVersion(merged, checkBase.Add(-time.Hour))}, + }, + { + description: "check with a pull request hint produces only that pull request's tip whatever its dates", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: resource.Version{PR: "21"}, + pullRequests: []*resource.PullRequest{rebased, previous, forked}, + pushed: map[string]pushLookup{rebased.Tip.OID: {at: checkBase.Add(-5 * 24 * time.Hour)}}, + pushLookups: github.Ptr(1), + expected: resource.CheckResponse{resource.NewVersion(rebased, checkBase.Add(-5*24*time.Hour))}, + }, + { + description: "check with a pull request hint falls back to the committer date when the push is unknown", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken"}, + version: resource.Version{PR: "21"}, + pullRequests: []*resource.PullRequest{rebased, previous}, + pushed: map[string]pushLookup{rebased.Tip.OID: {missing: true}}, + expected: resource.CheckResponse{testVersion(rebased)}, + }, + { + description: "check with a pull request hint for a filtered pull request produces nothing", + source: resource.Source{Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken", IgnoreDrafts: true}, + version: resource.Version{PR: "26"}, + pullRequests: []*resource.PullRequest{draft, previous}, + expected: resource.CheckResponse(nil), + }, { description: "check returns the latest version if there is no previous", source: resource.Source{ @@ -59,7 +201,7 @@ func TestCheck(t *testing.T) { pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[1], testPullRequests[1].Tip.PushedDate.Time), + testVersion(testPullRequests[1]), }, }, @@ -69,11 +211,11 @@ func TestCheck(t *testing.T) { Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken", }, - version: resource.NewVersion(testPullRequests[1], testPullRequests[1].Tip.PushedDate.Time), + version: testVersion(testPullRequests[1]), pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[1], testPullRequests[1].Tip.PushedDate.Time), + testVersion(testPullRequests[1]), }, }, @@ -83,12 +225,12 @@ func TestCheck(t *testing.T) { Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken", }, - version: resource.NewVersion(testPullRequests[3], testPullRequests[3].Tip.PushedDate.Time), + version: testVersion(testPullRequests[3]), pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[2], testPullRequests[2].Tip.PushedDate.Time), - resource.NewVersion(testPullRequests[1], testPullRequests[1].Tip.PushedDate.Time), + testVersion(testPullRequests[2]), + testVersion(testPullRequests[1]), }, }, @@ -99,7 +241,7 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", Paths: []string{"terraform/*/*.tf", "terraform/*/*/*.tf"}, }, - version: resource.NewVersion(testPullRequests[3], testPullRequests[3].Tip.PushedDate.Time), + version: testVersion(testPullRequests[3]), pullRequests: testPullRequests, files: [][]string{ {"README.md", "travis.yml"}, @@ -107,7 +249,7 @@ func TestCheck(t *testing.T) { {"terraform/modules/variables.tf", "travis.yml"}, }, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[2], testPullRequests[2].Tip.PushedDate.Time), + testVersion(testPullRequests[2]), }, }, @@ -118,7 +260,7 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", IgnorePaths: []string{"*.md", "*.yml"}, }, - version: resource.NewVersion(testPullRequests[3], testPullRequests[3].Tip.PushedDate.Time), + version: testVersion(testPullRequests[3]), pullRequests: testPullRequests, files: [][]string{ {"README.md", "travis.yml"}, @@ -126,7 +268,7 @@ func TestCheck(t *testing.T) { {"terraform/modules/variables.tf", "travis.yml"}, }, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[2], testPullRequests[2].Tip.PushedDate.Time), + testVersion(testPullRequests[2]), }, }, @@ -137,10 +279,10 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", DisableCISkip: true, }, - version: resource.NewVersion(testPullRequests[1], testPullRequests[1].Tip.PushedDate.Time), + version: testVersion(testPullRequests[1]), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[0], testPullRequests[0].Tip.PushedDate.Time), + testVersion(testPullRequests[0]), }, }, @@ -151,10 +293,10 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", IgnoreDrafts: true, }, - version: resource.NewVersion(testPullRequests[3], testPullRequests[3].Tip.PushedDate.Time), + version: testVersion(testPullRequests[3]), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[1], testPullRequests[1].Tip.PushedDate.Time), + testVersion(testPullRequests[1]), }, }, @@ -165,11 +307,11 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", IgnoreDrafts: false, }, - version: resource.NewVersion(testPullRequests[3], testPullRequests[3].Tip.PushedDate.Time), + version: testVersion(testPullRequests[3]), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[2], testPullRequests[2].Tip.PushedDate.Time), - resource.NewVersion(testPullRequests[1], testPullRequests[1].Tip.PushedDate.Time), + testVersion(testPullRequests[2]), + testVersion(testPullRequests[1]), }, }, @@ -180,12 +322,12 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", DisableForks: true, }, - version: resource.NewVersion(testPullRequests[5], testPullRequests[5].Tip.PushedDate.Time), + version: testVersion(testPullRequests[5]), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[3], testPullRequests[3].Tip.PushedDate.Time), - resource.NewVersion(testPullRequests[2], testPullRequests[2].Tip.PushedDate.Time), - resource.NewVersion(testPullRequests[1], testPullRequests[1].Tip.PushedDate.Time), + testVersion(testPullRequests[3]), + testVersion(testPullRequests[2]), + testVersion(testPullRequests[1]), }, }, @@ -200,7 +342,7 @@ func TestCheck(t *testing.T) { pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[6], testPullRequests[6].Tip.PushedDate.Time), + testVersion(testPullRequests[6]), }, }, @@ -211,10 +353,10 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", RequiredReviewApprovals: 1, }, - version: resource.NewVersion(testPullRequests[8], testPullRequests[8].Tip.PushedDate.Time), + version: testVersion(testPullRequests[8]), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[7], testPullRequests[7].Tip.PushedDate.Time), + testVersion(testPullRequests[7]), }, }, @@ -229,7 +371,7 @@ func TestCheck(t *testing.T) { pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[6], testPullRequests[6].Tip.PushedDate.Time), + testVersion(testPullRequests[6]), }, }, @@ -244,7 +386,7 @@ func TestCheck(t *testing.T) { pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[9], testPullRequests[9].Tip.PushedDate.Time), + testVersion(testPullRequests[9]), }, }, @@ -267,12 +409,12 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", States: []githubv4.PullRequestState{githubv4.PullRequestStateClosed, githubv4.PullRequestStateMerged}, }, - version: resource.NewVersion(testPullRequests[11], testPullRequests[11].Tip.PushedDate.Time), + version: testVersion(testPullRequests[11]), pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[10], testPullRequests[10].Tip.PushedDate.Time), - resource.NewVersion(testPullRequests[9], testPullRequests[9].Tip.PushedDate.Time), + testVersion(testPullRequests[10]), + testVersion(testPullRequests[9]), }, }, { @@ -288,7 +430,7 @@ func TestCheck(t *testing.T) { pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[12], testPullRequests[12].Tip.PushedDate.Time), + testVersion(testPullRequests[12]), }, }, { @@ -300,11 +442,11 @@ func TestCheck(t *testing.T) { {Context: "my-status-check-2", State: "success"}, }, }, - version: resource.NewVersion(testPullRequests[9], testPullRequests[9].Tip.PushedDate.Time), + version: testVersion(testPullRequests[9]), pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[9], testPullRequests[9].Tip.PushedDate.Time), + testVersion(testPullRequests[9]), }, }, { @@ -322,14 +464,14 @@ func TestCheck(t *testing.T) { files: [][]string{}, expected: resource.CheckResponse{ // todo: pull request index - resource.NewVersion(testPullRequests[13], testPullRequests[13].Tip.PushedDate.Time), + testVersion(testPullRequests[13]), }, }, } for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { - github := new(fakes.FakeGithub) + fake := new(fakes.FakeGithub) pullRequests := []*resource.PullRequest{} filterStates := []githubv4.PullRequestState{githubv4.PullRequestStateOpen} if len(tc.source.States) > 0 { @@ -343,19 +485,37 @@ func TestCheck(t *testing.T) { } } } - github.ListPullRequestsReturns(pullRequests, nil) + fake.ListPullRequestsReturns(pullRequests, nil) + fake.PushedDateStub = func(branch, commit string) (time.Time, bool, error) { + if lookup, ok := tc.pushed[commit]; ok { + return lookup.at, !lookup.missing, lookup.err + } + for _, p := range tc.pullRequests { + if p.Tip.OID == commit { + assert.Equal(t, p.HeadRefName, branch) + return p.Tip.CommittedDate.Time, true, nil + } + } + return time.Time{}, false, nil + } for i, file := range tc.files { - github.ListModifiedFilesReturnsOnCall(i, file, nil) + fake.ListModifiedFilesReturnsOnCall(i, file, nil) } input := resource.CheckRequest{Source: tc.source, Version: tc.version} - output, err := resource.Check(input, github) + output, err := resource.Check(input, fake) - if assert.NoError(t, err) { + if tc.wantErr { + assert.Error(t, err) + assert.Nil(t, output) + } else if assert.NoError(t, err) { assert.Equal(t, tc.expected, output) } - assert.Equal(t, 1, github.ListPullRequestsCallCount()) + assert.Equal(t, 1, fake.ListPullRequestsCallCount()) + if tc.pushLookups != nil { + assert.Equal(t, *tc.pushLookups, fake.PushedDateCallCount()) + } }) } } diff --git a/fakes/fake_github.go b/fakes/fake_github.go index a8cf263..e1a65f3 100644 --- a/fakes/fake_github.go +++ b/fakes/fake_github.go @@ -3,6 +3,7 @@ package fakes import ( "sync" + "time" "github.com/shurcooL/githubv4" resource "github.com/telia-oss/github-pr-resource" @@ -87,6 +88,22 @@ type FakeGithub struct { postCommentReturnsOnCall map[int]struct { result1 error } + PushedDateStub func(string, string) (time.Time, bool, error) + pushedDateMutex sync.RWMutex + pushedDateArgsForCall []struct { + arg1 string + arg2 string + } + pushedDateReturns struct { + result1 time.Time + result2 bool + result3 error + } + pushedDateReturnsOnCall map[int]struct { + result1 time.Time + result2 bool + result3 error + } UpdateCommitStatusStub func(string, string, string, string, string, string) error updateCommitStatusMutex sync.RWMutex updateCommitStatusArgsForCall []struct { @@ -494,6 +511,74 @@ func (fake *FakeGithub) PostCommentReturnsOnCall(i int, result1 error) { }{result1} } +func (fake *FakeGithub) PushedDate(arg1 string, arg2 string) (time.Time, bool, error) { + fake.pushedDateMutex.Lock() + ret, specificReturn := fake.pushedDateReturnsOnCall[len(fake.pushedDateArgsForCall)] + fake.pushedDateArgsForCall = append(fake.pushedDateArgsForCall, struct { + arg1 string + arg2 string + }{arg1, arg2}) + stub := fake.PushedDateStub + fakeReturns := fake.pushedDateReturns + fake.recordInvocation("PushedDate", []interface{}{arg1, arg2}) + fake.pushedDateMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1, ret.result2, ret.result3 + } + return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3 +} + +func (fake *FakeGithub) PushedDateCallCount() int { + fake.pushedDateMutex.RLock() + defer fake.pushedDateMutex.RUnlock() + return len(fake.pushedDateArgsForCall) +} + +func (fake *FakeGithub) PushedDateCalls(stub func(string, string) (time.Time, bool, error)) { + fake.pushedDateMutex.Lock() + defer fake.pushedDateMutex.Unlock() + fake.PushedDateStub = stub +} + +func (fake *FakeGithub) PushedDateArgsForCall(i int) (string, string) { + fake.pushedDateMutex.RLock() + defer fake.pushedDateMutex.RUnlock() + argsForCall := fake.pushedDateArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeGithub) PushedDateReturns(result1 time.Time, result2 bool, result3 error) { + fake.pushedDateMutex.Lock() + defer fake.pushedDateMutex.Unlock() + fake.PushedDateStub = nil + fake.pushedDateReturns = struct { + result1 time.Time + result2 bool + result3 error + }{result1, result2, result3} +} + +func (fake *FakeGithub) PushedDateReturnsOnCall(i int, result1 time.Time, result2 bool, result3 error) { + fake.pushedDateMutex.Lock() + defer fake.pushedDateMutex.Unlock() + fake.PushedDateStub = nil + if fake.pushedDateReturnsOnCall == nil { + fake.pushedDateReturnsOnCall = make(map[int]struct { + result1 time.Time + result2 bool + result3 error + }) + } + fake.pushedDateReturnsOnCall[i] = struct { + result1 time.Time + result2 bool + result3 error + }{result1, result2, result3} +} + func (fake *FakeGithub) UpdateCommitStatus(arg1 string, arg2 string, arg3 string, arg4 string, arg5 string, arg6 string) error { fake.updateCommitStatusMutex.Lock() ret, specificReturn := fake.updateCommitStatusReturnsOnCall[len(fake.updateCommitStatusArgsForCall)] diff --git a/github.go b/github.go index b7c0d60..2a9a48a 100644 --- a/github.go +++ b/github.go @@ -33,6 +33,9 @@ import ( // failures to write cache are silently ignored. const diskCacheFolder = "github-api-cache" +// pushes to look through on a branch for the one that produced the pull request's tip +const pushActivityPageSize = 10 + // Github for testing purposes. // //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o fakes/fake_github.go . Github @@ -44,6 +47,7 @@ type Github interface { GetChangedFiles(string, string) ([]ChangedFileObject, error) UpdateCommitStatus(string, string, string, string, string, string) error DeletePreviousComments(string, string) error + PushedDate(string, string) (time.Time, bool, error) } // GithubClient for handling requests to the Github V3 and V4 APIs. @@ -346,6 +350,29 @@ func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState) ([ return response, nil } +// PushedDate returns when the commit was pushed to the branch, from the repository activity +// (not supported by V4 API, which no longer populates Commit.pushedDate). +func (m *GithubClient) PushedDate(branch, commit string) (time.Time, bool, error) { + activities, _, err := m.V3.Repositories.ListRepositoryActivities( + context.TODO(), + m.Owner, + m.Repository, + &github.ListRepositoryActivityOptions{ + Ref: "refs/heads/" + branch, + PerPage: pushActivityPageSize, + }, + ) + if err != nil { + return time.Time{}, false, err + } + for _, activity := range activities { + if activity.After == commit && activity.Timestamp != nil { + return activity.Timestamp.Time, true, nil + } + } + return time.Time{}, false, nil +} + // ListModifiedFiles in a pull request (not supported by V4 API). func (m *GithubClient) ListModifiedFiles(prNumber int) ([]string, error) { var files []string diff --git a/in_test.go b/in_test.go index bcc7112..0b309ec 100644 --- a/in_test.go +++ b/in_test.go @@ -372,14 +372,15 @@ func createTestPR( IsCrossRepository: isCrossRepo, IsDraft: isDraft, State: state, - ClosedAt: githubv4.DateTime{Time: time.Now()}, - MergedAt: githubv4.DateTime{Time: time.Now()}, + UpdatedAt: githubv4.DateTime{Time: d}, + ClosedAt: githubv4.DateTime{Time: d}, + MergedAt: githubv4.DateTime{Time: d}, }, Tip: resource.CommitObject{ - ID: fmt.Sprintf("commit%s", n), - OID: fmt.Sprintf("oid%s", n), - PushedDate: &githubv4.DateTime{Time: d}, - Message: m, + ID: fmt.Sprintf("commit%s", n), + OID: fmt.Sprintf("oid%s", n), + CommittedDate: githubv4.DateTime{Time: d}, + Message: m, Author: struct { User struct{ Login string } Email string diff --git a/models.go b/models.go index aed558c..65f6a9e 100644 --- a/models.go +++ b/models.go @@ -109,6 +109,12 @@ func NewVersion(p *PullRequest, changedDate time.Time) Version { } } +// IsPullRequestHint reports whether the version only names a pull request to re-check, +// as passed by `fly check-resource --from pr:N`, rather than a version check has produced. +func (v Version) IsPullRequestHint() bool { + return v.PR != "" && v.ChangedDate.IsZero() +} + // PullRequest represents a pull request and includes the tip (commit). type PullRequest struct { PullRequestObject @@ -117,20 +123,27 @@ type PullRequest struct { Labels []LabelObject } -// Age: returns a date of the last update to the PR. -// If the job runs every minute, only PRs with an Age() in the last minute will run. -func (p *PullRequest) Age() time.Time { - age := p.Tip.PushedDate - if age == nil { - age = &p.Tip.CommittedDate +// ChangedDate orders the version: when the tip was pushed to the pull request, or committed when +// the push is unknown, never earlier than the pull request was opened, closed or merged. +func (p *PullRequest) ChangedDate(pushed time.Time) time.Time { + changed := p.Tip.CommittedDate.Time + if !pushed.IsZero() { + changed = pushed } - // handles the case where you're creating a fresh PR: - // there might be a few minutes between the push date and when you open the PR, - // so in that case take the CreatedAt date. - if age.Before(p.CreatedAt.Time) { - age = &p.CreatedAt + if p.CreatedAt.After(changed) { + changed = p.CreatedAt.Time + } + switch p.State { + case githubv4.PullRequestStateClosed: + if p.ClosedAt.After(changed) { + changed = p.ClosedAt.Time + } + case githubv4.PullRequestStateMerged: + if p.MergedAt.After(changed) { + changed = p.MergedAt.Time + } } - return age.Time + return changed } // PullRequestObject represents the GraphQL commit node. @@ -149,30 +162,17 @@ type PullRequestObject struct { IsDraft bool State githubv4.PullRequestState CreatedAt githubv4.DateTime + UpdatedAt githubv4.DateTime ClosedAt githubv4.DateTime MergedAt githubv4.DateTime } -// UpdatedDate returns the last time a PR was updated, either by commit -// or being closed/merged. -func (p *PullRequest) UpdatedDate() githubv4.DateTime { - date := p.Tip.CommittedDate - switch p.State { - case githubv4.PullRequestStateClosed: - date = p.ClosedAt - case githubv4.PullRequestStateMerged: - date = p.MergedAt - } - return date -} - // CommitObject represents the GraphQL commit node. // https://developer.github.com/v4/object/commit/ type CommitObject struct { ID string OID string CommittedDate githubv4.DateTime - PushedDate *githubv4.DateTime Message string Author struct { User struct {