Skip to content

Use v4 API for paginating releases in check#102

Merged
aoldershaw merged 14 commits intoconcourse:masterfrom
kirillbilchenko:graph_ql
Nov 16, 2020
Merged

Use v4 API for paginating releases in check#102
aoldershaw merged 14 commits intoconcourse:masterfrom
kirillbilchenko:graph_ql

Conversation

@kirillbilchenko
Copy link
Copy Markdown
Contributor

@kirillbilchenko kirillbilchenko commented Oct 26, 2020

Update dependencies fix tests and added graphql query for fetching list of releases

Release Note

  • When an access_token is provided, check will use the v4 API for paginating releases. This results in much faster/more stable checks on repositories that have many release assets

Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Copy link
Copy Markdown
Contributor

@YoussB YoussB left a comment

Choose a reason for hiding this comment

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

Hey @kirillbilchenko, first question, is there a reason we still have the vendor folder, this might be related to the previous pr but it seems that we can git rid of the vendor folder altogether and the code will still work normally.

@kirillbilchenko
Copy link
Copy Markdown
Contributor Author

Hey @kirillbilchenko, first question, is there a reason we still have the vendor folder, this might be related to the previous pr but it seems that we can git rid of the vendor folder altogether and the code will still work normally.

Yes, you are right we can remove it, let me do this, I only did update of dependencies and moved to go mod, but have not consider the cleanup.

Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Copy link
Copy Markdown
Contributor

@YoussB YoussB left a comment

Choose a reason for hiding this comment

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

Hey @kirillbilchenko,
as much as I am with using the latest and greatest Github graphql, I am not sure about the change here, it seems that the code used to be a lot easier to read without using the graphql.
I have a couple of questions here:

  • is there a noticeable enhancement with using graphql over the normal api? I have used graphql once and this was specifically to make a complicated query that would have taken many calls to the normal api to achieve, but the case here seems to me a lot easier.
  • if there is a win with using graphql over the normal api. do you think we can wrap the calls in a plugin instead of having the queries in the code, which would make the code a lot similar to the way it used to be but using graphql.

let me know what you think?

Comment thread README.md
@kirillbilchenko
Copy link
Copy Markdown
Contributor Author

@YoussB thanks for review, more details about graphQL and usage I think you can find in this pr, it was a case for big repos when usual api returning huge payloads #96. This is a first case, another case if you are using token you have two separate limits for graph ql queries and for api v3 and they are calculated separately, so it will definitely will be win for checks where you have lot's of repos. In this pr I decided to go with the same contracts in return for list of releases. to not rewrite all dependant use cases. But I think this can be extended in future and maybe improved.

@YoussB
Copy link
Copy Markdown
Contributor

YoussB commented Nov 4, 2020

@kirillbilchenko, thanks for the clarification. I didn't think about the payload. Can we at least wrap the graphql calls in a separate helper?

@kirillbilchenko
Copy link
Copy Markdown
Contributor Author

@kirillbilchenko, thanks for the clarification. I didn't think about the payload. Can we at least wrap the graphql calls in a separate helper?

Not sure if this make real sense, cuz now all calls are in one class and file and it's easier to understand what's happening there. But again we can wrap but I don't see any real benefit from this for now.

Signed-off-by: kirillbilchenko <kirya7@gmail.com>
@kirillbilchenko
Copy link
Copy Markdown
Contributor Author

@YoussB I moved this list resources to separate file called github_graphql

@YoussB YoussB removed their assignment Nov 6, 2020
Copy link
Copy Markdown
Contributor

@aoldershaw aoldershaw left a comment

Choose a reason for hiding this comment

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

Looks pretty good, but I noticed it seems to break things when no access token is set

Comment thread github_graphql.go
Comment thread github.go
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
@kirillbilchenko
Copy link
Copy Markdown
Contributor Author

@aoldershaw I updated the pr for cases without token I keep v3 usage

Comment thread github.go Outdated
Comment thread github.go
Comment thread github.go Outdated
Comment thread github_graphql.go Outdated
Comment thread github_test.go
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Copy link
Copy Markdown
Contributor

@aoldershaw aoldershaw left a comment

Choose a reason for hiding this comment

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

Nice, good catch on the range reference.

I noticed one small thing that's still not quite working - the ID field always fails to parse as an int. I left a suggestion on how to "fix" the issue, but the fix is pretty hacky. Curious to hear your thoughts

Comment thread github_graphql.go Outdated
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Comment thread github_graphql.go Outdated
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Copy link
Copy Markdown
Contributor

@aoldershaw aoldershaw left a comment

Choose a reason for hiding this comment

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

Think just some more test coverage and we're good to go!

Comment thread github_test.go Outdated
Comment thread github_test.go
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
Copy link
Copy Markdown
Contributor

@aoldershaw aoldershaw left a comment

Choose a reason for hiding this comment

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

Thanks again!

@aoldershaw aoldershaw merged commit c82c11c into concourse:master Nov 16, 2020
@aoldershaw aoldershaw changed the title Graph ql Use v4 API for paginating releases in check Nov 16, 2020
@kirillbilchenko kirillbilchenko deleted the graph_ql branch November 16, 2020 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants