lbryio / lbry.go

MIT License
29 stars 20 forks source link

Add API Server to lbry.go #12

Closed tiger5226 closed 6 years ago

tiger5226 commented 6 years ago

moved api server from internal-apis to lbry.go so it can be reused.

lyoshenka commented 6 years ago

I like this overall - extracting this code makes sense. Some comments:

lyoshenka commented 6 years ago

In fact, a few things need to be fixed. We can’t use util.Debugging, we have to configure how errors get back to us (util.Send.ToSlack is too api-specific), we can’t hardcode lbry.io in the headers, etc

tiger5226 commented 6 years ago

your right, best to just delete the references, to see what breaks. Then repackage that.

tiger5226 commented 6 years ago

Made the following adjustments. -Removed dependency on internal apis -Moved over only required packages. -Adjusted slack.go to be generic instead of hardcoding channel name. -Moved over travis package from internal-apis -Added Repository struct for webhook and an IsMatch method. It is possible for any repository to send a webhook to the api and it will trigger a deploy. We should check against the owner, repo and branch.

RE util.Debugging, I moved that over as it stands for internal-apis. I can create an issue to solve for this and start the conversation on how we get this implemented better.

lyoshenka commented 6 years ago

There's also slack code in lbryio/boardbot repo. It should be merged with this as well.

Also, I don't like that slack errors are ignored. They should be returned, and then we can ignore them when we call the SendToSlack function if we want. SendToSlack def shouldn't be logging the error itself.

tiger5226 commented 6 years ago
tiger5226 commented 6 years ago

-Renamed package to api -removed util.Debugging from server.go -Added an ErrorHandling function that be used as interface for slack for internal-apis -Added Map for Header settings that can be set before the serving -Merged slack code from lbryio/boardbot -Cleaned up the slack.go code so it made more sense and flowed better -Removed gitignore entry for .idea, should be global -Removed debugging.go

tiger5226 commented 6 years ago

FYI - I am using the apiserver branch for chainquery until this is resolved. Please do not delete the branch. I will delete it once I correct the vendoring to use master.

tiger5226 commented 6 years ago

Can we merge this in now? ( DO NOT DELETE BRANCH ).

lyoshenka commented 6 years ago

Looks good to me, though to be sure, I'd like to see what internal-apis looks like using this code. But I think its fine to merge this. If there are any tweaks to make later, I bet they'll be minor.

tiger5226 commented 6 years ago

All clear I will create an internal-apis branch. This will also fix the travis issue with the webhook where it runs twice. I can make that change here.