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

Danger will eventually need a GitHub API client #17

Closed
orta opened this issue Sep 2, 2017 · 23 comments
Closed

Danger will eventually need a GitHub API client #17

orta opened this issue Sep 2, 2017 · 23 comments
Labels
enhancement help wanted You can do this, Orta is unlikely to do this for you

Comments

@orta
Copy link
Member

orta commented Sep 2, 2017

Existing libs:

Both, very reasonably don't support SwiftPM and both have dependencies that would need to be resolved too.

If you ask me, this is something that can easily be code-gen'd - the JS version we use in Danger JS is basically using this routes.json to generate all the different versions: https://github.com/mikedeboer/node-github/blob/master/lib/routes.json

So it may be easier to build that as a new SwiftM package than to port another and all its deps.

@kdawgwilk
Copy link
Contributor

I actually have a working GitHub client somewhere on my machine because I came across both of these as well and needed it for a bot I was writing in Swift. I will try and dig it up

@orta
Copy link
Member Author

orta commented Oct 18, 2017

In the next Danger JS alpha the DSL will include a bunch of GitHub related metadata: https://github.com/danger/danger-js/pull/395/files#diff-7c330aa12e1d8f926998264b38ca39feR23 ( see the DangerDSLJSONType in source/dsl/DangerDSL.ts if the URL 404s. This should be enough metadata to bootstrap any API client.

@orta
Copy link
Member Author

orta commented Oct 18, 2017

Also, @kdawgwilk has an SPM fork of octokit.swift - https://github.com/kdawgwilk/octokit.swift which could get turned into something for production

@orta orta added enhancement help wanted You can do this, Orta is unlikely to do this for you labels Nov 24, 2017
@eneko
Copy link
Member

eneko commented Nov 28, 2017

I've been looking at Tentacle a little bit this last week. It is Swift 4 and SPM-ready.

I like how easy it looks, though haven't used it. However, I don't like that it depends on ReactiveSwift. It feels like a really big dependency to bring, and large dependencies are no bueno :(

@orta
Copy link
Member Author

orta commented Nov 28, 2017

Yeah, agreed, I wouldn't accept a lib with a transitive on Reactive Swift as a dep into Danger.

@SD10
Copy link
Member

SD10 commented Dec 12, 2017

@orta If you get a minute can you elaborate on the code generation? I'd like to look more into this topic.

If you ask me, this is something that can easily be code-gen'd - the JS version we use in Danger JS is basically using this routes.json to generate all the different versions: https://github.com/mikedeboer/node-github/blob/master/lib/routes.json

I wouldn't mind writing us our own API client free of dependencies. It seems like there are a few libraries that attempt to do this and then die off. Maybe if I throw it in an organization with Aeryn the community will help maintain it 😅

@eneko
Copy link
Member

eneko commented Dec 19, 2017

I'm looking at the GraphQL V4 API and it seems relatively easy to implement without any framework dependencies, as long as a token is provided and results are returned as dictionaries. Implementing Swift 4 Codable would be neat, but not required for our needs, I would say.

@orta
Copy link
Member Author

orta commented Dec 19, 2017

@eneko's point is pretty good for the V4 API - that could definitely be a trivial way to add an API client - just set it up to handle a string and pass back dictionaries - this gets quite tricky because everything could be optional in GraphQL ( there's some work around typing this here and you could probably do something with apollo swift too )

Could also look at general GraphQL libs and just ship one of those, someone'd probably have to play with it. I think it's a real good idea though 👍


WRT code gen from the v3 API - the module I use in node is generated from that one JSON file, and so are all the types. Happens in here.

https://github.com/octokit/node-github/blob/f5de18d3c700d6cd7132dc16d645c49dbeecc89c/lib/routes.json#L2-L11

screen shot 2017-12-19 at 6 49 29 pm

Adding, or using this as a submodule to generate Swift code could handle most of the general faffing in setting up this API.

The only thing it doesn't handle today is response types. Which is feasible but a tad tricky.

@eneko
Copy link
Member

eneko commented Dec 20, 2017

I got this working for another project, really basic right now, but if you guys would like to use it, we can start developing it further.

https://github.com/eneko/GitHub

Let me know your thoughts.

@eneko
Copy link
Member

eneko commented Dec 20, 2017

@orta do you have a list of API operations that Danger would use? I can figure out the queries needed for each operation.

Example operations needed:

  • Approve PR
  • Fail PR
  • etc.

@orta
Copy link
Member Author

orta commented Dec 20, 2017

@eneko - the API is not for Danger itself (That's handled in JS) - it's for people using Danger to easily do things with. So that could be checking org membership, etc. Just making it easy to pass GraphQL query through is a great win.

@SD10
Copy link
Member

SD10 commented Dec 21, 2017

@eneko I can hack on this with you if you're open to making it a community project in the future. I started this last night as well because I need it for some other automation plugins I want to build.

@eneko
Copy link
Member

eneko commented Dec 21, 2017

Cool, I see yeah, adding a method to pass any arbitrary query would be super easy. The rest would be just adding all the optional fields to the response models, which although tedious, should be really easy to do.

@SD10, yeah, I’m cool making this a community project, for sure :)

@SD10
Copy link
Member

SD10 commented Dec 21, 2017

@eneko I'll take a look at it this weekend 👍 If you want open any issues you want me to look at so I don't step on your toes

@SD10
Copy link
Member

SD10 commented Dec 21, 2017

42 minutes later
My opinion was changed by @Sherlouk from his experience working on GitHawk and using GitHub's V4 API with apollo. We may want to consider apollo-ios as Orta suggested. We could take it in as a reasonable dependency and build a GitHub API client on top of it.

@Sherlouk
Copy link

Sherlouk commented Dec 21, 2017

Stepping in with little context but purely from having contributed to GitHawk for a while and had the fun and sufferance of working with the GitHub APIs

One of the first questions is really V3 or V4, if you're just wanting a sorta "read only" version then I think V4 is definitely the way especially with the benefits of being able to choose exactly what you do/don't get but when it comes to mutations/modifications there's still quite a lot missing from the V4.

I could find certain Danger plugins suffering from that, so you may have to have some support for V3 regardless! For example this auto-label plugin for Danger Ruby! This disparity is definitely something to have noted, as it will become a limiting factor if you choose V4!

Apollo has been pretty easy to use, and we've had a call with Martijn and seems to be taking it good places! The ability to create GQL files, and have the models etc all generated for you is pretty slick and I think with a thin wrapper to add convenience it'd be pretty easy to use

What'd be super nice is if, as a plugin creator, I could create my own GQL files and then use that to do custom mutations/queries (which Apollo will already supply most of the ground work for you guys!)

@yhkaplan
Copy link
Contributor

yhkaplan commented Mar 24, 2018

I've written out some of the pros and cons below from what I've been able to research about each of the options that exist. Feel free to point out anything I might be missing.

My personal inclination is to use Apollo/APIv4 for as much stuff as possible, but also rely on Octokit or a fork of Octokit for whatever features APIv4 doesn't support currently, slowly phasing out Octokit as GitHub implements more features for APIv4.

New Apollo-iOS-based framework

Pros

Cons

  • No SPM compatibility (it apparently is compatible)
  • Optionals everywhere
  • Somewhat limited functionality
  • Testing? (I guess you test the autogenerated models)

Octokit

Pros

  • Lots of features
  • No big dependencies

Cons

Tentacle

Pros

  • Lots of features
  • SPM support

Cons

  • ReactiveSwift is a big dependency

@orta
Copy link
Member Author

orta commented Mar 31, 2018

I think there's a possibility for using apollo-ios but does it require pre-compiled queries in some sense to generate the types? I guess I should research that

@yhkaplan
Copy link
Contributor

yhkaplan commented Apr 1, 2018

@orta I stand corrected~

Apollo appears to take .graphql files (a simple form of model code) and generate Swift model classes from them conforming to the GraphQLQuery protocol. This page explains more of the details: https://www.apollographql.com/docs/ios/fetching-queries.html

Apollo iOS takes a schema and a set of .graphql files and uses these to generate code you can use to execute queries and access typed results.

@eneko
Copy link
Member

eneko commented Apr 24, 2018

Not sure what is the current status, but I'm going to try updating my GitHub GraphQL client framework with Quicktype to generate client models: https://github.com/quicktype/quicktype

@f-meloni
Copy link
Member

f-meloni commented Nov 5, 2018

@orta
Copy link
Member Author

orta commented Nov 5, 2018

Perfect, lets get that in as github.api - the auth token and any headers/ base urls are included in the JSON passed to Danger swift, https://github.com/danger/danger-js/blob/master/source/danger-incoming-process-schema.json#L531-L548 👍

@f-meloni
Copy link
Member

This should be closed now by #95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted You can do this, Orta is unlikely to do this for you
Projects
None yet
Development

No branches or pull requests

7 participants