Fix hard-coded URL in Configuration.cs

[johnbfair]

https://github.com/in2bits/IronGitHub/blob/master/IronGitHub/Configuration.cs#L8

This is a problem for anyone trying to use this via GitHub for Enterprise. This should be moved to the config file.

[johnduhart]

It's possible to change with

new GitHubApi(new GitHubContext(new Configuration("http://ghe.example.com")))

But that sucks. It should be a argument in GitHubApi's constructor.

This should be moved to the config file.

Huh?

[johnbfair]

It should be able to be injected via a config file. Now sure why that's confusing.

[johnduhart]

I think we should leave that up to implementers of the library instead of enforcing our own method for configuration.

[johnbfair]

I disagree, but if that's the consensus I'll go with it. It still has to come out to the GitHubApi layer and not be nested 3 layers deep as it is now.

[timerickson]

There currently is no config file, and I'd prefer to keep it that way. It doesn't have to be nested three layers deep. It could be something along the lines of introducing

public enum GitHubDomains
{
    Default,
    Enterprise
}

//changes to Configuration
public class Configuration
{
    public const string DefaultDomain = "api.github.com";
    public const string EnterpriseDomain = "ghe.github.com"; //or whatever it is

    public Configuration(GitHubDomains domain) : this(DefaultDomain)
    {
        switch (domain)
        {
            case Default: Domain = DefaultDomain; break;
            case Enterprise: Domain = EnterpriseDomain: break;
            default: throw new ArgumentOutOfRangeException("domain");
        }
    }
}

public class GitHubEnterpriseApi : GitHubApi
{
    public GitHubEnterpriseApi() : base(GitHubDomains.Enterprise)
    {
    {
}

you see where I'm going here?

This is fine so long as there's not much more (potentially variable) configuration we expect. I like to stay away from config files and keep all config in code and by convention so much as possible.

Comments?...

[johnbfair]

We can't keep the enterprise URL hard coded in the Configuration class. It's completely unpredictable and is set by each individual enterprise however they deem it should resolve. For us it's "http://git/" and that's it. It has to be a config variable (not necessarily from the config file inside of this library) that's passed into GitHubApi because otherwise we have the problem that John lists above which is

new GitHubApi(new GitHubContext(new Configuration("http://git/"))) 

[timerickson]

are there any other differences, such as to the api interface itself? If not, I would prefer to simply pass the domain, and maybe an optional path, into the GitHubApi constructor:

public GitHubApi(string domain)
{
    Context.Configuration.Domain = domain;
}

Better?

[johnbfair]

Yes I think that's what John was suggesting earlier. We might as well just pass the domain in like your code suggests. That way I can pass that value in from a config file on my side (which is what I was talking about earlier, but didn't have time to get into :frowning: ).

Auth might be a little different, but I'm not entirely sure because I haven't dug far enough into how it might differ.