Githawk: Make base url injectable

[rnystrom]

Don’t hardcode github.com everywhere. Maybe we set this in NSUserDefaults?

This will help unlock enterprise support.

See #408

Bug Report Dump (Auto-generated)

Version 1.15.0 (SEE BUILD PHASE)
Device: iPhone X (iOS 11.1.2)
TestFlight: false

[Sherlouk]

I'm also going to link this to #159 - and I think we have a couple similar issues where we assume the path of something.

159 specifically is caused by this line of code: https://github.com/rnystrom/GitHawk/blob/master/Classes/View%20Controllers/CreateProfileViewController.swift#L13

There are we assume a profile is baseurl.com/:login where in the case of an app it's baseurl.com/apps/:login. This is one of the situations (among others) where we need to be using the responses we get back from GitHub for the URLs. Again for this issue specifically, in the actor GQL entity you can get a url which is the user's profile. This works for apps, as well as users.

This is necessary for enterprise support because some paths can be configured to be slightly different!

[Sherlouk]

In response to your base comment about where to store this information, I think the best place would actually be as part of the account manager?

We already support multiple accounts, each account should know what client it's from -- Obviously for now everything is on github.com, but eventually it could be anything git.githawk.com. We'll probably need a small migration piece to default all existing apps to github. (If this is generic enough it'll set us up for other platforms in the future, if we ever decide to)

[rnystrom]

That's a great idea. That way you could be logged into your enterprise _and_ github.com account at the same time! That's really cool.

So ya, we should tie a base url to each user session/account.

[Sherlouk]

enum Client {
    case github
    case githubEnterprise(String)

    var baseUrl: String {
        switch self {
        case .github: return "https://github.com/"
        case .githubEnterprise(let url): return url
        }
    }
}

Follow up from the example in 408, just an idea

[Sherlouk]

@BasThomas Are you working on this? or is this one free to pick up?

@rnystrom too

[BasThomas]

No(t yet), go ahead!

_{Sent with GitHawk}

[Sherlouk]

Thinking that GithubUserSession is the best place, but this uses NSCoding - not sure what the best way is to encode/decode the suggested enum above as we can't convert to/from a rawValue!

I'm suspecting we'd need a nested type here where it's more akin to this:

class Client: NSCoding {
  enum ClientType: String {
    case github
    case githubEnterprise

    // feature toggles on a client by client basis
  }

  var type: ClientType
  var baseUrl: String

  // GitHub
  init() {
    self.type = .github
    self.baseUrl = "https://github.com/"
  }

  // GitHub Enterprise
  init(type: ClientType, baseUrl: String) {
    self.type = type
    self.baseUrl = baseUrl
  }

  // NSCoding can easily encode/decode
}

Then GithubUserSession should be able to handle that type?

An alternative is we just store the baseUrl as a string directly, and we make the naive comparison that it's GitHub if the url == "https://github.com" to do feature toggles? But this locks us down if we ever want to do other providers!

@rnystrom @BasThomas Opinions?

[BasThomas]

Yeah, I'd say make it so we can just inject a client. Although I don't see it making too much sense to support sites like BitBucket or GitLab anytime soon (I think the feature sets are too far apart, but who knows), I think making it so you can just use a provider makes most sense.

Re code sample: for GitHub Enterprise you're also passing in the type; would that make sense? It should always be .githubEnterprise, no?

[Sherlouk]

Well yea we could make the assumption that if you're passing a baseUrl in then it's an enterprise instance! But if we're otherwise happy with that example then that's the plan we can go with

[weyert]

What about the case were the tickets are hosting somewhere different from were the code is hosted? For example, the code is hosted on Github private repo's and the tickets are handled through JIRA.

[BasThomas]

I think that's definitely out of scope for GitHawk atm 😛. Would probably even work better as a separate app I think.

[BasThomas]

@Sherlouk about that GitHub Enterprise init: I was thrown of by the comment // GitHub Enterprise; I think we shouldn't default the ClientType, as this could in the future be another client. :)

[Sherlouk]

@BasThomas This is what I ended up doing:
https://github.com/rnystrom/GitHawk/pull/1124/files#diff-5a3f4c1fa934dc525557fb13e824ff55R32

Tell me if you think this is incorrect, and how you think I should change it! 😄

[BasThomas]

Yeah that works, just think that we should maybe not even pass the default value. Obviously a nit as we can just change this once another Client is added, which probably won't be anytime soon.