Add CircleCi service #522

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
@pbiggar

pbiggar commented Feb 9, 2013

Adds CircleCi service, with tests, in accordance with CONTRIBUTING.md.

@reconbot

This comment has been minimized.

Show comment Hide comment
@reconbot

reconbot Feb 27, 2013

+1

+1

lib/services/circleci.rb
+
+ http.headers['content-type'] = 'application/x-www-form-urlencoded'
+ http.params.merge!(:payload => JSON.generate(payload) , :event_type => JSON.generate({ :event_type => self.event }))
+ http_post circleci_url, payload.to_json

This comment has been minimized.

Show comment Hide comment
@technoweenie

technoweenie Mar 2, 2013

Contributor

Why are you posting JSON when the content type is application/x-www-form-urlencoded

@technoweenie

technoweenie Mar 2, 2013

Contributor

Why are you posting JSON when the content type is application/x-www-form-urlencoded

@technoweenie

This comment has been minimized.

Show comment Hide comment
@technoweenie

technoweenie Mar 2, 2013

Contributor

This looks fine, but why is it receiving all hooks? Does circleci run builds when someone follows your repository?

I'd prefer you only listen to the services that you need to function. Otherwise, update your documentation with what events that your service will receive.

Contributor

technoweenie commented Mar 2, 2013

This looks fine, but why is it receiving all hooks? Does circleci run builds when someone follows your repository?

I'd prefer you only listen to the services that you need to function. Otherwise, update your documentation with what events that your service will receive.

@pbiggar

This comment has been minimized.

Show comment Hide comment
@pbiggar

pbiggar Mar 3, 2013

We're posting a form taking JSON strings, because that's what our backend accepts.

The main reason we're adding this service is that we want to embrace a lot more of github's features. We're starting to use some more events in the short term, but anticipate using a large majority over the next few months. When someone follows a repo, we want to mark them as following so that we can suggest they follow that repo from within circleci.

We have a use for nearly all features right now: commit_comment, create, delete, follow, fork, fork_apply, issue_comment, member, public, pull_request, push, team_add, watch, pull_request_review_comment and status. We want to do some experiments with how to support gist and gollum. We also want to be able to support new events without making a pull request, as we will almost certainly have a use for them.

pbiggar commented Mar 3, 2013

We're posting a form taking JSON strings, because that's what our backend accepts.

The main reason we're adding this service is that we want to embrace a lot more of github's features. We're starting to use some more events in the short term, but anticipate using a large majority over the next few months. When someone follows a repo, we want to mark them as following so that we can suggest they follow that repo from within circleci.

We have a use for nearly all features right now: commit_comment, create, delete, follow, fork, fork_apply, issue_comment, member, public, pull_request, push, team_add, watch, pull_request_review_comment and status. We want to do some experiments with how to support gist and gollum. We also want to be able to support new events without making a pull request, as we will almost certainly have a use for them.

@pbiggar

This comment has been minimized.

Show comment Hide comment
@pbiggar

pbiggar Mar 23, 2013

@technoweenie: how does this look now? OK to merge?

pbiggar commented Mar 23, 2013

@technoweenie: how does this look now? OK to merge?

@technoweenie

This comment has been minimized.

Show comment Hide comment
@technoweenie

technoweenie Apr 22, 2013

Contributor

As I mentioned, you need to update the documentation stating that you are listening to all of the events.

Also, you're still posting JSON as they body, and as the Get query parameters. You should do something like this:

http_post circleci_url,
  :payload => generate_json(payload) , :event_type =>  generate_json({ :event_type => self.event })

You'll have to parse the body in the tests to confirm you're passing it through properly.

Contributor

technoweenie commented Apr 22, 2013

As I mentioned, you need to update the documentation stating that you are listening to all of the events.

Also, you're still posting JSON as they body, and as the Get query parameters. You should do something like this:

http_post circleci_url,
  :payload => generate_json(payload) , :event_type =>  generate_json({ :event_type => self.event })

You'll have to parse the body in the tests to confirm you're passing it through properly.

@reillyse

This comment has been minimized.

Show comment Hide comment
@reillyse

reillyse Apr 24, 2013

Contributor

@technoweenie how is that ? any better ?

Contributor

reillyse commented Apr 24, 2013

@technoweenie how is that ? any better ?

@technoweenie

This comment has been minimized.

Show comment Hide comment
@technoweenie

technoweenie Apr 24, 2013

Contributor

I think sending JSON as form values is ridiculous. My only point is that you were specifying the form type, but sending JSON in the body and sending the JSON as URI query params.

Just send JSON if you want my opinion :)

Contributor

technoweenie commented Apr 24, 2013

I think sending JSON as form values is ridiculous. My only point is that you were specifying the form type, but sending JSON in the body and sending the JSON as URI query params.

Just send JSON if you want my opinion :)

@technoweenie

This comment has been minimized.

Show comment Hide comment
@technoweenie

technoweenie Apr 24, 2013

Contributor

Hey, I read your question wrong. I took it as "how is that any better" :) This looks good.

Contributor

technoweenie commented Apr 24, 2013

Hey, I read your question wrong. I took it as "how is that any better" :) This looks good.

@technoweenie

This comment has been minimized.

Show comment Hide comment
@technoweenie

technoweenie Apr 24, 2013

Contributor

This branch is based off an ancient master branch on your fork. I had to cherry pick your commits. You can see them here:

ca814d7...ee47e58

We'll get this pushed out later today. Thanks!

Contributor

technoweenie commented Apr 24, 2013

This branch is based off an ancient master branch on your fork. I had to cherry pick your commits. You can see them here:

ca814d7...ee47e58

We'll get this pushed out later today. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment