Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return all open PRs instead of filtering by date #4

Merged
merged 2 commits into from
Dec 8, 2021

Conversation

bmalehorn
Copy link

@bmalehorn bmalehorn commented Dec 7, 2021

From the original commit:

This resource can inadvertently miss Pull Requests due to out-of-order
commits across PRs. If PR#2 is opened after PR#1, but the head commit
of PR#2 is older than the head commit of PR#1, the resource will not
include PR#2 in the list of new versions provided to Concourse.

This problem is not fixed upstream, but it is discussed in telia-oss#205. There are a few solutions provided:

  1. using digitalocean/github-pr-resource
  2. applying ctreatma@50ef79a.

Right now we're using solution 1, using opendoor/digitalocean-github-pr-resource in only the code pipeline. In all other pipelines we use opendoor/telia-oss-github-pr-resource, which is based on the master branch in this repo and contains caching improvements to reduce GitHub API token usage.

Instead of using two different version, I've applied ctreatma@50ef79a to our branch, fixed the merge conflicts and made tests pass. Once this is merged in, all pipelines can use a single version of this resource which has all the required features:

  1. based on the latest upstream
  2. api caching
  3. context filter
  4. date filter fix

This fixes the issue by not filtering on date at all, with deduplication happening at the DB layer in concourse. In theory this will increase the DB resource usage, but that's sitting at 5% today and has never been a bottleneck.

Hopefully the api caching part will reduce the GitHub API token usage of the code pipeline, which is almost at the limit of a single user already.

Test Plan

  • go test -race -v ./...

test manually

  • push this as a separate docker tag, bm-fix-telia-oss-26
  • fly update admiral-integration-tests pipeline, confirm it's still fetching properly, put it back to latest
  • fly update code pipeline, confirm it's still fetching properly, monitor github API token usage, put it back to latest

To deploy:

  • merge this PR
  • copy tag bm-fix-telia-oss-26 to latest

This resource can inadvertently miss Pull Requests due to out-of-order
commits across PRs.  If PR#2 is opened after PR#1, but the head commit
of PR#2 is older than the head commit of PR#1, the resource will not
include PR#2 in the list of new versions provided to Concourse.

Rather than attempt to find a different way of tracking which PRs are
"new" given an input version, we can remove the date-based filtering
and return all open PRs.  Concourse can deduplicate versions based on
metadata, which means that we will only trigger new jobs for versions
that Concourse hasn't seen before.

This makes it easier for teams to use this resource to track PRs in
Concourse, since they no longer have to ensure that a PR has a later
head commit than all currently-opened PRs in order to notify Concourse
that their PR exists.

(cherry picked from commit 50ef79a)
Copy link

@defjosiah defjosiah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, seems like a good fix

@bmalehorn
Copy link
Author

I'll try testing / deploying this tomorrow when concourse pipelines are unpaused.

@bmalehorn bmalehorn force-pushed the bm/fix-telia-oss-26 branch from 53fdfe2 to e7c80e8 Compare December 8, 2021 19:54
@bmalehorn
Copy link
Author

To confirm this bug is fixed, I:

  1. bm/test-go-build-8: created, committed, pushed
  2. bm/test-go-build-9: created, committed, pushed
  3. bm/test-go-build-9: opened PR https://github.com/opendoor-labs/code/pull/38065
  4. bm/test-go-build-8: opened PR https://github.com/opendoor-labs/code/pull/38066

This matches the reproduction described in telia-oss#26 (comment).
Despite being opened out of order, both were picked up by code/pr-branch resource:
image
Both PRs then triggered concourse-ci/bazel-build-test and councourse-ci/go-lint:
9 ran tests:
image
8 ran tests:
image

So, this resolves the issue we used digitalocean/github-pr-resource for.

On top of that, it reduces the API usage of the github token by about half, no longer hitting the limit. I tried turning this feature on & off, and you can see the difference here (datadog link):
image

I'm merging this PR and making it the latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants