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

WIP: Support GitLab API #37

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

WIP: Support GitLab API #37

wants to merge 23 commits into from

Conversation

stanhu
Copy link

@stanhu stanhu commented Dec 2, 2015

This is the beginnings of supporting the GitLab API. The first test was to implement Ghee.repos. Here is a start. Notice that the endpoints are quite different:

GitHub: https://developer.github.com/v3/repos/#response

GitLab: https://github.com/gitlabhq/gitlabhq/blob/master/doc/api/projects.md#list-projects

See #26

@rauhryan
Copy link
Member

rauhryan commented Dec 2, 2015

@stanhu I have some further feedback, I wrote it out this morning but somehow didn't post the comment.

My initial reaction is that it's going to be really hard for me to parse through the noise of the namespace change with the new classes being added for gitlab.

I propose that we split those commits out into a new PR and merge them into master once all the spec are passing again.

Not exactly sure if that means we need to close this PR or *gasp* force push your branch (please don't do that yet, it was a joke)

@stanhu
Copy link
Author

stanhu commented Dec 2, 2015

@rauhryan I think that's a good idea. We should probably first decide whether these namespace changes are appropriate (e.g. Ghee::API::GitHub vs. Ghee::GitHub::API?). I did the former since I'm focused on getting the API translation working, but I can imagine we may need to put in specific connection clients for each.

@rauhryan
Copy link
Member

rauhryan commented Dec 2, 2015

Good question, we're going to noodle on it a little bit.

We'll post a picture of our whiteboard soon.

On Wed, Dec 2, 2015 at 11:59 AM, Stan Hu [email protected] wrote:

@rauhryan https://github.com/rauhryan I think that's a good idea. We
should probably first decide whether these namespace changes are
appropriate (e.g. Ghee::API::GitHub vs. Ghee::GitHub::API?). I did the
former since I'm focused on getting the API translation working, but I can
imagine we may need to put in specific connection clients for each.


Reply to this email directly or view it on GitHub
#37 (comment).

@ahimta
Copy link

ahimta commented Dec 2, 2015

I think that, initially, Ghee::API should stay as is.
For example: you would add the GitLab implementation in Ghee::Gitlab.
The implementation will need to have the same API as the current GitHub implementation.

The changes should almost be transparent, except for a small detail where the user
instantiates the client as a Gitlab client, and even then the Gitlab client should behave as close
as possible to the GitHub client (this part needs some serious analysis).

This is of course the first stage of working on Ghee.
After that you'll probably work on huboard-web and make some changes to Ghee to make sure it works, since Ghee Gitlab implementation is bound to have some issues with huboard-web

This is the second stage, and you will be working on both projects at the same time.

After you make sure everything works, then you can make other changes and refactors (e.g: move Ghee::API to Ghee::Github::API)

@rauhryan
Copy link
Member

rauhryan commented Dec 2, 2015

Sooo, in an ideal world...

huboard-web changes very little

my vision would require two changes to huboard-web

  1. Gemfile
    gem 'ghee', require: nil
  2. ./config/application.rb
if ENV["HUBOARD_BACKEND"] == "github"
    require 'ghee/github'
else
    require 'ghee/gitlab'
end

In a perfect world, nothing would change other than config values in HuBoard, all the abstractions would live in Ghee

@gsmethells
Copy link

👍 @stanhu thanks for working on this one.

@rauhryan
Copy link
Member

rauhryan commented Dec 3, 2015

We should probably first decide whether these namespace changes are appropriate (e.g. Ghee::API::GitHub vs. Ghee::GitHub::API?).

We had some discussions and we think the best way to break it down is by having a common interface that the outside world interacts with ie;

Ghee::API

Under the hood, we can try to infer which API you are talking to by the url (only possible for the respective public API of github.com, gitlab.com, bitbucket.com and beyond), but we will also need the ability to explictily set the implementation through config or via require 'ghee/{github,gitlab}'

The implementations should be pulled up a namespace ie;

Ghee::API
Ghee::GitHub::API
Ghee::GitLab::API

But both implementations should respect the same public API as Ghee::API

@ahimta
Copy link

ahimta commented Dec 3, 2015

@rauhryan, I agree with you on the way to divide up namespaces.

As for distinguishing which type of API is currently being used, I think the right way to do it is to
add a method to the clients to tell the client type
For example: Ghee::Client#{is_gitlab,is_github}? or Ghee::Client#client_type.

Ghee::Client, of course will be either Ghee::Github::Client or Ghee::Gitlab::Client.
And both these clients should have the same API return very similar values for each
corresponding method.

So this way, the two clients are completely independent and private to Ghee and users only interact
with Ghee::Client.

I think this is, possibly, one very good initial solution, but there's definitely room for environment.

@ahimta
Copy link

ahimta commented Dec 3, 2015

In addition, the Github and Gitlab client should be completely independent, and not have any
dependencies to each other, since if you think about the abstractions, it just doesn't make any
sense for any of them to depend on the other.

And honestly, if you want to do so, there's definitely a much better way to do it.

@stanhu
Copy link
Author

stanhu commented Dec 3, 2015

Yes, I like the idea of providing some parameter to a client rather than depending on the application including the right ghee/gitXXX module.

@ahimta
Copy link

ahimta commented Dec 3, 2015

I agree. If Ghee supports both Gitlab and Github, it doesn't make sense to make users of the library
use two requires when it can be done with one.

I don't know, @rauhryan maybe you have a good reason for making this choice.

@rauhryan
Copy link
Member

rauhryan commented Dec 3, 2015

Yes, I like the idea of providing some parameter to a client rather than depending on the application including the right ghee/gitXXX module.

My thought process is, by designing the system to be capable of defaulting the implementation by using require 'ghee/gitXXX' it would have net benefits in the design of the code

For example, by default, ghee works just like any library.

You gem install ghee, require 'ghee', client = Ghee::GitHub.create_client token and you're off to the races

But as far as HuBoard is concerned, the experience should be client = Ghee.new hash and the implementation is driven from configation or by the explicit require 'ghee/xxx'

The idea is that existing users and codebases that currently use Ghee should experience a very low impact from this new work and that the breaking changes to existing applications can be as simple as require 'ghee/github'

@stanhu
Copy link
Author

stanhu commented Dec 4, 2015

Out of curiosity, how many projects other than Huboard depend on ghee?

I can see the argument of making changes that minimize the impact of other applications. I think that's fine for now. I do think @ahimta's ideas are good and are probably worth considering for the next major release.

@ahimta
Copy link

ahimta commented Dec 4, 2015

I think, the most important thing now is to implement Gitlab integration.
After that you can work out other details.

Then at least we actually have something tangible to talk about :)

One important thing in software development , I think,
is to delay as many design decisions as possible.

@rauhryan
Copy link
Member

rauhryan commented Dec 8, 2015

One important thing in software development , I think,
is to delay as many design decisions as possible.

I completely agree with you @ahimta, I'm on board with pushing forward with the current direction, as long as we avoid any decisions that are difficult to reverse or rollback, we should have another discussion

@stanhu
Copy link
Author

stanhu commented Dec 11, 2015

Made some good progress here. I've run into a a number of missing GitLab API features that Huboard uses, so I'll look into adding them to GitLab.

@rauhryan
Copy link
Member

Thank you @stanhu I will try and crave out some time to review this weekend.

@stanhu
Copy link
Author

stanhu commented Dec 12, 2015

Submitted a change to GitLab in preparation of supporting Huboard. I'll edit this note as I go along:

@discorick
Copy link
Member

@stanhu looks like things are coming along great! We might also want to consider adding a Unit Test suite for translators at some point, will make tracking down any potential bugs in in translation much easier for future maintainers.

Let @rauhryan or me know if you need anything!

edit: Oh and 👍 on the GitLab Merge Requests!!

@stanhu
Copy link
Author

stanhu commented Dec 13, 2015

@discorick Thanks! Yes, definitely I'll add tests later. I'm trying to flush out all the requirements and changes needed in GitLab as soon as possible.

I've hacked huboard-web to get it to the point where I can see the first page:

image

It looks like warden and GitHub are tightly integrated into the login flow, so I'll have to figure out how to separate that out.

In huboard, do you know why Rails.logger doesn't seem to display any of my custom messages while running huboard? I've tried disabling puma to no avail. I've had to resort to opening custom log files to debug things.

@stanhu
Copy link
Author

stanhu commented Dec 14, 2015

@discorick and @rauhryan: To support GitLab and GitHub in huboard-web, can we just use a generic OAuth2 provider, such as warden-oauth2, instead of GitHub-specific one? As far as I can tell, you're not using any of the specific GitHub calls within warden-github and rely on ghee for that.

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.

5 participants