Update @mmrobins #247 ("Add a uri_scheme option") by merging with master #485
Conversation
This allows github enterprise installations that don't use https to use the hub gem. I think all installations should use https, but I don't control my company's install, and their argument for not using https is that the install is only available over VPN, so traffic is already encrypted, and using https means they'd need to buy another https cert. So until I can convince them otherwise, I'd still like to be able to make pull-requests from the command line.
…14-02-11 Conflicts: lib/hub/github_api.rb test/hub_test.rb
|
👍 Works great for me! |
|
@Yeraze: Cool, thanks!!! |
|
Works for me, thanks for bringing things up to date and adding on. |
| private | ||
|
|
||
| def scheme | ||
| config_file = ENV['HUB_CONFIG'] || '~/.config/hub' |
There was a problem hiding this comment.
This duplicates the logic of reading config files that is in GitHubAPI.
Could there be a way to re-use the config object? Maybe accessing api_client.config or something like that?
There was a problem hiding this comment.
I think I tried that a year ago, but api_client is a private method in commands.rb, so couldn't really get to it in a non hacky way. https://github.com/github/hub/blob/master/lib/hub/commands.rb#L827. In fact, I think I basically copy pasted from there. If you wanted that api_client method could be moved into its own object so it was useable in other places that seems reasonable to me.
There was a problem hiding this comment.
commands.rb methods have access to api_client. How else would they do API requests?
There was a problem hiding this comment.
Yeah, commands.rb has a private method called api_client so context.rb wouldn't be able to get to it. I might make it public or move it somewhere else (github_api.rb?)
|
Thanks for resurrecting this! |
This lets one do things like: hub clone myinternal/myrepo from a GitHub Enterprise host without setting GITHUB_HOST
so it can be used from context.rb and commands.rb; reduces duplication
GitHubAPI.api_client
Makes the stubbing in the tests work properly; fixes tests
|
For some reason, there are tests failing only in Ruby 1.9.2 (not even in 1.9.3): https://travis-ci.org/github/hub/builds/18901580 Anyone know why? |
|
Any idea why tests are failing only on Ruby 1.9.2 (and not 1.9.3)? I am not a Ruby expert. |
|
You end up with a double slash in some URLs that you construct, and 1.9.2 HTTP request mechanism takes it literally and produces invalid requests. This fixes it: diff --git a/lib/hub/github_api.rb b/lib/hub/github_api.rb
index 5a3f548..a0cece2 100644
--- a/lib/hub/github_api.rb
+++ b/lib/hub/github_api.rb
@@ -171,7 +171,7 @@ module Hub
end
def get_host_url host, format_str, *args
- ("%s://%s/" + format_str) %
+ File.join("%s://%s", format_str) %
[config.uri_scheme(host), api_host(host), *args]
endHowever I agree that this is impossible to test from cukes because in test mode, all requests are over HTTP always. It's a good thing that you raised this point. I'm going to try to devise a solution for this. |
This is a patch from @mislav and should fix a Ruby 1.9.2 test failure.
|
Cool, thanks for the patch, @mislav. This is passing tests now: https://travis-ci.org/github/hub/builds/19013735 Though I do want to take a look at your alternative, PR #488 -- I may not get a chance to look at for a few days, as I will be away traveling, but I'm eager to see what you came up with there. |
|
Thanks for working on this. This is now possible in master via the |
#247 from @mmrobins adds nice features for those of us with GitHub Enterprise and a server with only HTTP; no HTTPS.
Unfortunately, #247 was never merged and the code on master evolved and #247 doesn't merge cleanly anymore.
I don't want his work to go to waste though, so I went ahead and merged his changes with master to come up with this PR.
Maybe this can be merged?