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

PushedDate is null when the pull request is from a fork #22

Closed
itsdalmo opened this issue May 23, 2018 · 4 comments
Closed

PushedDate is null when the pull request is from a fork #22

itsdalmo opened this issue May 23, 2018 · 4 comments

Comments

@itsdalmo
Copy link
Contributor

This means that pull requests from a fork will never be discovered - need to figure out an alternative.

@itsdalmo
Copy link
Contributor Author

I looked at pull requests, and it seems like updatedAt is what gets us closest, but it will probably be bumped whenever there is a new review, comment, etc. in addition to new commits.

For commits we have the following dates (as an example):

Branch:

"pushedDate": "2018-05-16T12:21:09Z",
"committedDate": "2018-05-16T12:20:37Z",
"authoredDate": "2018-05-16T12:20:37Z",

Fork:

"pushedDate": null,
"committedDate": "2018-05-22T14:25:11Z",
"authoredDate": "2018-05-22T12:08:40Z",

Not sure what is implied with authoredDate, so commitedDate seems like the safest option. One obvious drawback of using commitedDate is that someone opening a pull request from a branch/fork with old commits in it will have to make new commits in order to trigger a build. Might not be an issue on small projects, but on larger projects with lots of open PR's one would need to rush from commit to push in order for it to be discovered...

On the other hand, if we skip filtering on date all together means that we will produce an array of new versions which is always equal to the number of open pull requests. And if we specify paths or ignore_paths, each check will require a number of V3 API calls equal to the number of open pull requests...

I think we need to filter on date somehow.

@itsdalmo
Copy link
Contributor Author

itsdalmo commented May 23, 2018

Pull requests support filtering the timeline with a since argument: timeline(since: <DateTime>), however, this simply filters by committedDate, so we might as well do this filtering on our end and not have to handle filtering out non-commit events.

@itsdalmo
Copy link
Contributor Author

Could consider using potentialMergeCommit and the committedDate on that commit instead. However, the committed date for the potential merge is not only updated when there is a push to the base branch or the PR itself - e.g. for an example PR I was looking at:

  • createdAt for the pull request: 2018-05-14
  • pushedDate for the last commit in the PR: 2018-05-16
  • publishedAt for last event in the PR (a comment): 2018-05-16 (1h after the commit)
  • updatedAt for the repository: 2018-05-17
  • committedDate for the potential merge commit: 2018-05-18

I.e., it's not immediately clear what prompted Github to create a new merge commit for this PR. And if we were to use that date instead of the committedDate from the last commit in the PR, we'd be producing new versions at random times when Github decided to update the merge...

itsdalmo pushed a commit that referenced this issue May 23, 2018
Use committed date instead of pushed date (part of #22)
@itsdalmo
Copy link
Contributor Author

Closing this as it seems more likely that committed will be removed instead of replaced.

mplzik referenced this issue in mplzik/github-pr-resource May 15, 2019
The fix for #22 makes github-pr-resource skip PRs if the the latest
commit in a PR pushed has a later date than one in PR being pushed.
This fix works around this by using `PushedDate` by default and
resorting to `CommitDate` iff `PushedDate` is not available.

This should bring the best of both worlds -- checks for all PRs on
master and a chance for non-master-based PRs to be checked.

Signed-off-by: Milan Plzik <[email protected]>
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

No branches or pull requests

1 participant