Implement RtGithub Constructors For Custom Domain

[kkashi01]

Thanks for quick turn-around on issue below and updating README.

Relates to: #1516

Follow-up, does it make sense to update code to a constructor that takes Request and token?
Or a way to set Token and/or userid/pwd in RtGithub? Per your suggestion, if RtGithub is instantiated with custom-domain, we won't be able to set token (or userid/pwd). Is my understanding correct?

[0crat]

@amihaiemil/z please, pay attention to this issue

[0crat]

@kkashi01/z this project will fix the problem faster if you donate a few dollars to it; just click here and pay via Stripe, it's very fast, convenient and appreciated; thanks a lot!

[amihaiemil]

@kkashi01

if RtGithub is instantiated with custom-domain, we won't be able to set token (or userid/pwd). Is my understanding correct?

I think you can do that right when you are instantiating the Request. See how its done in the constructors that we have now. I believe all you have to do is the following (for token)

final Github github = new RtGithub (
    new ApacheRequest("https://your.githubdomain.com")
        .header(
            HttpHeaders.USER_AGENT,
            new FromProperties("jcabigithub.properties").format()
        )
        .header(HttpHeaders.ACCEPT, MediaType.APPLICATION_JSON)
        .header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON)
        .header(                                              //HERE, set the auth token
            HttpHeaders.AUTHORIZATION,
            String.format("token %s", token)
        )
        .through(AutoRedirectingWire.class)
);

or the following for password

final Github github = new RtGithub (
    new ApacheRequest("https://your.githubdomain.com")
        .header(
            HttpHeaders.USER_AGENT,
            new FromProperties("jcabigithub.properties").format()
        )
        .header(HttpHeaders.ACCEPT, MediaType.APPLICATION_JSON)
        .header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON)
        .header(                         //HERE, set auth with username/pwd
            HttpHeaders.AUTHORIZATION,
            String.format(
                "Basic %s",
                DatatypeConverter.printBase64Binary(
                    String.format("%s:%s", user, pwd)
                        .getBytes(StandardCharsets.UTF_8)
                )
            )
        )
        .through(AutoRedirectingWire.class)
);

Is it not possible to do this? Or do you think it's ugly?

[amihaiemil]

@kkashi01 On a second thought, I think we will implement a few extra constructors so you can specify a custom domain without bothering with all the configuration of a Request.

But in the meantime, the above snippets of code should work fine :)

[kkashi01]

@amihaiemil Thanks for the code-snippet. Constructor for custom domain would definitely help.
Meanwhile, using your code snippet, I'm definitely getting closer. However, getting a 406 [1] below. The issue for getting 406 code is because the url request should be like [2] not [1]. In [1], the path "/repo" is automatically added. For my case, it should be eliminated. Thoughts?

[1] 406 Not Acceptable [http://github.example.com/repos/MyRepoName/propertySynchronizer/pulls?status=all]

[2] [http://github.example.com/MyRepoName/propertySynchronizer/pulls?status=all]

[amihaiemil]

@kkashi01 I'm afraid changing URI paths is not possible. Github's URI paths are at the core of this library's architecture.

Encapsulation and the objects' precedence/fluency all follow Github's URIs. This cannot be changed without breaking encapsulation or changing the architecture fundamentally.

If you have your own Github instance deployed, my advice is to not change URIs. Changing the domain is definetely not a problem, but do not change or mask URIs.

[kkashi01]

@amihaiemil thanks. Unfortunately I don't have control over our github and [2] above is just how the URL is set up. I do find your library very elegant and useful. Need to see how to handle this.

[amihaiemil]

@kkashi01 I think you could "highjack" the URI building logic by providing your own Request to the constructor.

Request is an interface which you could decorate with some implementations that would "ignore" or modify certain paths when they are being added throughout the library.

But this would be very painful and error-prone. First of all, you would have to study all the classes and methods of this library to see what paths are being added and where.

It would be dirty and painful, but it could work to some extent...

[amihaiemil]

@kkashi01 I've implemented the constructors, see PR #1518

It will be released as soon as possible, with version 1.1 of this library. However, we're having some issues with Rultor at the moment (rultor is the chatbot which automates our release process), and we're waiting for those to be fixed...

[kkashi01]

@amihaiemil Thanks for the update. Will wait for v1.1 and give it a try.
Looking through the code, I see that "/repos" is being hard coded in many classes (e.g. RtPull, RtIssue, ...).

Do you think it makes sense to update Repo class to have "/repos" as default attribute and allow setting it through a constructor method? That way, all above classes ( like above) can just call a helper method on Repo to get that value (the default or the the custom one; e.g. blank):
e.g. update highlighted below to something like .path(repo.repos)

BTW. You mentioned below. Do you mean

Request is an interface which you could decorate with some implementations that would "ignore" or modify certain paths when they are being added throughout the library. Did you mean RequestURI ?

[amihaiemil]

@kkashi01

Do you think it makes sense to update Repo class to have "/repos" as default attribute and allow setting it through a constructor method?

Yes, this would be an option, I'll look into it tomorrow :)

That way, all above classes ( like above) can just call a helper method on Repo to get that value

This will surely not happen. Helper methods are against our principles and besides that, encapsulation would be broken. Repo would become some sort of utility class.

But you are right, most classes should set their root paths (e.g. RtRepos -> /repos; RtIssues -> /issues etc) in their ctor rather than hardcoding them throughout methods.

Did you mean RequestURI ?

Yes, that's actually the interface that would need decoration, but you would have to start from the top Request anyway, because that's what the constructor of RtGithub expects.

[amihaiemil]

@kkashi01 I assume the end result would be something like this:

Github github = new RtGithub();
Repos repos = github.repos();    // What we have now; adds the default /repos path
Repos myRepos = github.repos("/myReposPath");    // Adds /myReposPath instead of the default /repos

[kkashi01]

@amihaiemil getting much closer 👍 THANKS

[amihaiemil]

@kkashi01 I looked into it.

Having the paths injected through the constructor is a good idea, but only for us, internally -- for instance, if Github's paths will change in the future, then it will be easier for us to do the change as well.

But, to be honest with you, I don't like the idea of letting the user specify paths when calling methods such as repos(). If we allow this, then the library becomes a "camel-bird": it will be both a formal wrapper for Github's API and some sort of "generic" REST client where users can specify different paths and URIs: it could actually be used as a REST client to any api which just so happens to have the same JSON output as Github.

However, I remembered we have the Github.entry() method which returns the internal configured Request: this request will have the proper domain and headers already configured and you can use it to make requests that are not suppored by the wrapper yourself (in your case, your requests will have different paths).

I hope this is ok with you. You could also fork the repo and make the changes yourself.

[kkashi01]

@amihaiemil

Having the paths injected through the constructor is a good idea, but only for us, internally -- for instance, if Github's paths will change in the future, then it will be easier for us to do the change as well.

Agreed and this will definitely be a good change:

But, to be honest with you, I don't like the idea of letting the user specify paths when calling methods such as repos()

I totally understand your point. However, this library will possibly have limitation that it won't work for enterprise Custom Domains unless a workaround is provided

If we allow this, then the library becomes a "camel-bird": it will be both a formal wrapper for Github's API and some sort of "generic" REST client where users can specify different paths and URIs: it could actually be used as a REST client to any api which just so happens to have the same JSON output as Github.

I don't think this will be true because all your classes are modeled around Github

However, I remembered we have the Github.entry() method which returns the internal configured Request: this request will have the proper domain and headers already configured and you can use it to make requests that are not suppored by the wrapper yourself (in your case, your requests will have different paths).

if Github.entry works for updating the Request, then I think this should be good and solves mine (and alike custom domain) issues

I hope this is ok with you. You could also fork the repo and make the changes yourself.

Sure. If you are refactoring to remove the hard-coding, I would wait till you are finished and then go from there and fork / update / put in PR if needed.

Thanks again for quick turn-around and library

-Hossein Amerkashi.

[amihaiemil]

@kkashi01 thanks. I will continue with the refactoring in #1520.

I'm closing this ticket since the main issue here was solved, I've implemented constructors for custom domains.

Will be released as soon as rultor is fixed ...

[0crat]

Job gh:jcabi/jcabi-github#1517 is not assigned, can't get performer

[0crat]

This job is not in scope