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

check: paginate by push date, not git commit date #6

Merged
merged 2 commits into from
Dec 11, 2021

Conversation

bmalehorn
Copy link

@bmalehorn bmalehorn commented Dec 10, 2021

Context

github-pr-resource used to paginate by PushedDate. This is a timestamp GitHub stores on each commit at the time the user typed git push. By paginating by PushedDate, github-pr-resource would only trigger builds on PRs that have had a new commit pushed to them since the last time github-pr-resource ran. That makes sense.

In telia-oss#22 it was discovered that on pull requests created from forked branches have PushedDate: nil. To fix, the paginated on CommitDate, which is the time that the user types git commit.

However, this created telia-oss#26 - if someone types git commit, waits a few hours, then pushes, their commit won't be picked up because we've already handled newer commits.

I attempted to fix this in #4 by removing the filter entirely. This fixes the problem but increases the github token usage a lot.

Instead, fix this by applying mplzik@edca4f5 and combining it with the PR creation date. This paginates by max(lastCommit.PushedDate || lastCommit.CommitDate, pullRequest.CreatedAt). In code, all of our commits have a PushedDate so we're back to the original, fixed behavior. It also handles the case where you push a branch, then don't open a pull request for several minutes.

This should reduce the github usage a lot, since we'll go from loading the data on all 300 open pull requests to only loading 1-5 pull requests that have opened since the last call.

Origin

Me

Summary of Changes

  • hand-apply mplzik@edca4f5, which introduces an ChangedDate = PushedDate || CommitDate

Test Plan

  • task test
  • cat ~/tmp/test.json | go run cmd/check/main.go
{
  "source": {
    "repository": "opendoor-labs/code",
    "access_token": "ghp_<redacted>",
    "base_branch": "master"
  },
  "version": {
    "approved_review_count": "0",
    "changed": "2021-12-10T01:30:42Z",
    "commit": "0130ca9d1475adfc094a84b4b6c230122f97db4f",
    "committed": "2021-12-10T01:30:32Z",
    "pr": "38122",
    "state": "OPEN"
  }
}

I observed that this only returned the 4 new commits that have been updated since 2021-12-10T01:30:42Z.
To deploy:

  • make docker image, push to admiral pipeline
    image
  • push docker image to code pipeline
  • make 2 draft PRs to test it out

to deploy:

  • make latest docker image

Documentation

N/A

@delete-merged-branch delete-merged-branch bot deleted the branch master December 10, 2021 05:07
@bmalehorn bmalehorn changed the base branch from bm/revert-fix-telia-oss-26 to master December 10, 2021 05:09
@bmalehorn bmalehorn force-pushed the bm/pushed-date-pagination branch from ec88bd5 to dfbf290 Compare December 10, 2021 05:09
@bmalehorn
Copy link
Author

@dan-hipschman-od updated this to also take into account the pull request creation date.

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.

2 participants