-
Notifications
You must be signed in to change notification settings - Fork 8
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
Version selection refactor, logging, schema previews, and more #1
Conversation
8e1252e
to
aaa9833
Compare
aaa9833
to
073cbcd
Compare
5c7398e
to
95720c7
Compare
4696a27
to
e73c1fd
Compare
a67d164
to
5f049e3
Compare
Hi, can you please give us some more information when are you planning to merge this PR or the current status? Thank you. |
Hi @aliculPix4D, thanks for reaching out. I am getting close to having something that is worth merging. Just need to do a bit more testing and polish it up. Then I will squash, have some other folks test it, then merge to our fork. We will give it a little while running internally (maybe two weeks) before submitting it upstream in a pull request. |
Will this end up triggering builds of a PR every time there's any action on the PR (e.g. comments, reviews, title changes, etc.)? |
@bhcleek nah, I recently added a section to the README covering how the search and filtering works, you can see it here: |
9c874ff
to
e962567
Compare
* `pullrequest.HeadRefForcePushed` which will include PRs where a [HeadRefForcePushed](https://developer.github.com/v4/object/headrefforcepushedevent) occurred | ||
* `pullrequest.Reopened` which will include PRs where a [BaseRefChanged](https://developer.github.com/v4/object/reopenedevent) occurred | ||
* `pullrequest.BuildCI` which will include PRs with a new comment containing `[build ci|ci build]` | ||
* `pullrequest.NewCommits` which will include PRs with a new commit since the last `updated` timestamp of the last check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than include this list, can we just link to some godoc? This type of documentation is a prime example of "a lie waiting to happen" - it's too far from the code to expect anyone to remember to update it when they change the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, however, I wanted it documented on the README for now because all of this work is in a feature branch and I am not sure it will ever get merged back upstream. Once we merge it to the master branch, I will replace it to a link to GoDoc.
check.go
Outdated
pulls, err = manager.GetLatestOpenPullRequest() | ||
} else { | ||
pulls, err = manager.ListOpenPullRequests(request.Version.UpdatedDate) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know else statements are bad go 😜
This is another case where the var pulls []pullrequest.PullRequest
followed by assignment inside branching logic suggests that what you really want is a function call:
func findPulls(since time.Time) ([]pullrequest.PullRequest, error) {
if since.IsZero() {
return manager.GetLatestOpenPullRequest()
}
return manager.ListOpenPullRequests(since)
}
func Check(request CheckRequest, manager Github) (CheckResponse, error) {
var response CheckResponse
pulls, err := findPulls(request.Version.UpdatedDate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, when I wrote it and looked at it, I could hear you in the back of my head... Thanks for the sample fix. Will update it. :)
@christophermancini this PR is huge. Are there particular concerns that you'd like to me focus on? |
@bhcleek The logic for the Check operation.
|
98432ad
to
f0a1167
Compare
goal was to make version selection more robust, easier to test and reduce the chance legitimate versions are missed by concourse * add support for schema previews * add support to trigger pr via comment * convert files to use graphql api * logging * use glob path filtering * decouple internal PullRequest from GitHub PullRequestObject * add env var support to context param * add head short ref to metadata
dbe0c36
to
9f1c080
Compare
The initial scope of this PR was to not use
CommittedDate
for filtering PRs that will not be included as new versions. This has clearly been identified as a source for missing PRs in Issue #26. This modified approach looks at the PRUpdatedAt
field instead. This field is updated every time there is a new commit and shifts the filtering from the code to the GraphQL engine, improving performance even further.How it works, is that the version information pushed back to concourse contains the PR #, the head ref sha, and the PR UpdatedAt (
UpdatedDate
). Every resource check operation will take theupdated
value from the most recent version in Concourse to filter the list of PRs queried. We end up with only PRs that are open and have been updated since the last time we checked. We also use theupdated
value from Concourse to filter the TimelineItems connection, only receiving events & objects attached to the PR since we last checked. This allows us to expand our methods of filtering down the PR versions we return to Concourse.You can read more about the filtering options in the changes to the README of this PR.
Other noteworthy changes include:
[build ci | ci build]
/tmp/github-pr-resource.log
on the containers which can be accessed via hijacking the check, get, or put steps for a pipelinefilepath.Match
which doesn't adhere to glob standards and is not deterministic (e.g. acts differently on different operating systems)PullRequest
struct from the GitHubPullRequestObject
context
parameter on status putWe used a GraphQL query like the following to experiment and come up with this solution:
Sample variables: