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

Test fails: should create a project from github and run its first test #495

Closed
knownasilya opened this issue Jul 30, 2014 · 11 comments
Closed

Comments

@knownasilya
Copy link
Member

Adding the test repo gives an error code.

screen shot 2014-07-30 at 3 39 15 pm

@knownasilya
Copy link
Member Author

@knownasilya
Copy link
Member Author

The login works. But yeah, maybe something is down.. Checking status page now.

Edit: status seems fine.

@knownasilya
Copy link
Member Author

Merge #494 before tackling this, since that fixes other test issues.

@knownasilya
Copy link
Member Author

This is the response received from github:

{ 
  message: 'Validation Failed',
  documentation_url: 'https://developer.github.com/v3/repos/hooks/#create-a-hook',
  errors: [
     {
       resource: 'Hook',
       code: 'custom',
       message: 'The "push" event cannot have more than 20 hooks'
     }, {
       resource: 'Hook',
       code: 'custom',
       message: 'The "pull_request" event cannot have more than 20 hooks' 
     }, {
       resource: 'Hook',
       code: 'custom',
       message: 'The "issue_comment" event cannot have more than 20 hooks' 
     }
   ]
}

@knownasilya
Copy link
Member Author

Looks like I found the issue! Seems like we need to check if the hook exists before adding a duplicate. Tests should run now, until the next time we hit the limit lol.

screen shot 2014-08-01 at 8 57 02 am

@kfatehi
Copy link
Member

kfatehi commented Aug 1, 2014

Nice work @knownasilya ! Yeah definitely not a strider bug. I've run into this before as Github limits your # of total project hooks. Honestly I wouldn't do any #DELETE calls to their API even though you can --adding that logic will be cluttery and dangerous (what if it deletes the wrong webhook, what if it thinks it deleted it but something added another and you're still at the 20 limit) -- instead what we should do is forward the correct error to the user and let them deal with it. I'd only go so far as to give the user a link to his webhooks settings page. Thoughts?

@knownasilya
Copy link
Member Author

Well we could check if the hook exists, if the API allows that functionality? And just not add the hook if it's already there.

But I think your idea might be better.

@knownasilya knownasilya removed the bug label Aug 1, 2014
@kfatehi
Copy link
Member

kfatehi commented Aug 1, 2014

It does allow it, you would need to store the "id" when you make the POST call ( see response: https://developer.github.com/v3/repos/hooks/#response-2 ) but what I'm saying is that it's a better virtue to do few features well, with good errors, than to do many features poorly, with poor errors. in this particular case i would rather we propogate the error from github to the user directly rather than widen Strider's scope. Also consider if the webhooks are from 20 other services, we dont have an ID for any of them. then what?

@knownasilya
Copy link
Member Author

@keyvanfatehi exactly why I think your suggestion is better, since we have to implement it anyways 👍

Maybe create a new issue for this, not sure if it belongs in strider-github..

@kfatehi
Copy link
Member

kfatehi commented Aug 1, 2014

kk will do

@niallo
Copy link
Member

niallo commented Aug 1, 2014

Nice detective work!

On Friday, August 1, 2014, Keyvan Fatehi notifications@github.com wrote:

kk will do


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

Niall O'Higgins
W: http://niallohiggins.com
E: n@niallo.me
T: @niallohiggins

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants