From f5e585f6f424ef47eb2890b7d4ceeed37eadfa91 Mon Sep 17 00:00:00 2001 From: kurtmc <2661907+kurtmc@users.noreply.github.com> Date: Wed, 7 Oct 2026 03:13:20 +0000 Subject: [PATCH] Order versions by push date instead of committer date GitHub's GraphQL API no longer populates Commit.pushedDate (deprecated, "Removal on 2023-07-01 UTC"), so Age() has been falling back to the committer date. Concourse only schedules a `version: every` input ranked above the newest version the job has already built, and a rebased commit keeps an old committer date, so pushes of rebased stacks were ranked below the cursor and never built. Take the push time from the repository activity API instead, looking up only pull requests updated since the last version. A push the repository has not recorded yet is left for the next check so a version's date never changes; forks fall back to the committer date. A version that only names a pull request (fly check-resource --from pr:N) now produces just that pull request's tip rather than every open pull request. The README's pre-existing markdownlint violations (heading levels, a pipe inside a table cell, a bare "here" link) are fixed so the edit can pass the lint hook. Co-Authored-By: clank-halter[bot] <2696940+clank-halter[bot]@users.noreply.github.com> Clank-Job-Id: 62103f95-6af0-4f16-9d96-36e5cac5a5c4 --- README.md | 58 +++++++---- check.go | 115 ++++++++++++++++----- check_test.go | 238 ++++++++++++++++++++++++++++++++++++------- fakes/fake_github.go | 85 ++++++++++++++++ github.go | 27 +++++ in_test.go | 13 +-- models.go | 52 +++++----- 7 files changed, 474 insertions(+), 114 deletions(-) diff --git a/README.md b/README.md index e104a99a..adc17fd9 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 13ea53e7..ac9b95c5 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 da8fa20d..2b5b02a6 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 a8cf2632..e1a65f3d 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 b7c0d601..2a9a48af 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 bcc71124..0b309ece 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 aed558c1..65f6a9e1 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 {