fcoury / octopi

A Ruby interface to GitHub API v2
http://hasmany.info/2009/4/18/ruby-interface-to-github-api
MIT License
216 stars 47 forks source link

change MAX_RETRIES on a per-command basis #36

Closed xiongchiamiov closed 14 years ago

xiongchiamiov commented 14 years ago

MAX_RETRIES, defined in api.rb, restricts the number of times octopi will attempt to pull some information from the api before throwing an APIError. A bit new to Ruby, but I didn't see any way to easily change that, especially on a per-command basis. I'd most certainly like to, since I'm using this in an irc bot, and having to wait for 10 403s before giving an error message when I try and retrieve a non-existant issue is a bit annoying.

radar commented 14 years ago

How would you like this to be configurable? And is it really a 403 that is sent back when an issue doesn't exist, if so then that's quite silly. One would expect a 404.

xiongchiamiov commented 14 years ago

Ha, now that is the question. ;)

Here is an example of the current behavior, using whatever version of octopi gem pulled down yesterday:

irb(main):001:0> require 'octopi'
=> true
irb(main):002:0> repo = Octopi::Repository.find("xiongchiamiov", "erasmus")
=> #<Octopi::Repository:0x9926778 @api=#<Octopi::AnonymousApi:0x920e464 @format="yaml", @read_only=true>, @keys=[:description, :url, :homepage, :fork, :private, :name, :owner, :open_issues, :forks, :watchers], @description="simple, modular irc bot/framework in ruby; successor to mpu", @url="http://github.com/xiongchiamiov/erasmus", @homepage="", @fork=false, @private=false, @name="erasmus", @owner="xiongchiamiov", @open_issues=12, @forks=1, @watchers=2>
irb(main):003:0> repo.issue(100)
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403. Retrying request.
GitHub returned status 403, despite repeating the request 10 times. Giving up.
Octopi::APIError: Octopi::APIError
        from /usr/lib/ruby/gems/1.9.1/gems/octopi-0.1.0/lib/octopi.rb:146:in `rescue in get'
        from /usr/lib/ruby/gems/1.9.1/gems/octopi-0.1.0/lib/octopi.rb:134:in `get'
        from /usr/lib/ruby/gems/1.9.1/gems/octopi-0.1.0/lib/octopi.rb:121:in `find'
        from /usr/lib/ruby/gems/1.9.1/gems/octopi-0.1.0/lib/octopi/resource.rb:42:in `find'
        from /usr/lib/ruby/gems/1.9.1/gems/octopi-0.1.0/lib/octopi/issue.rb:58:in `find'
        from /usr/lib/ruby/gems/1.9.1/gems/octopi-0.1.0/lib/octopi/repository.rb:90:in `issue'
        from (irb):3
        from /usr/bin/irb:12:in `<main>'
irb(main):004:0> 

When I was browsing through the source yesterday, I was thinking that I'd find an option to change this in the options hash that gets passed around on commands, so perhaps that'd be a good place for it? I'm not entirely sure about that, though, since I seem to remember that options contains things that are passed on to the API (correct me if I'm wrong here), and it's a bit bad mixing things like that. It's still the best option I can think of, though.

avdi commented 14 years ago

GitHub returns a 403 for both non-existant repos and repos you don't have access to. From a security standpoint it makes sense to pick ether 404 or 403 for both scenarios, since otherwise you'd be giving people a clue as to whether a private repo exists or not. I'm not sure whether 404 or 403 makes more sense as the one to pick.

Either way, neither 403 nor 404 should be retried according to RFC2616, and I'm curious why 403s are retried by default in Octopi - some quirk of Github? I've been having the same problem as a previous commenter: our software has to wait for 10 retries to tell if a repo is invalid or innaccessable.

avdi commented 14 years ago

Correction: 403s should not be retried according to RFC2616; no judgement is ade aboue 404s.

radar commented 14 years ago

Thank you for your input avdi, in this commit: http://github.com/fcoury/octopi/commit/eba5abbf2d3d8de3be9da3cebe4e186b4dd50348 I ensure that there are no retries made when there is a 403. Is this to your liking?

avdi commented 14 years ago

Awesome, that's one item to cross off on my list of patches to submit. Thanks!

radar commented 14 years ago

Great to hear!