support listeners for events #72

Merged
merged 4 commits into from Apr 12, 2014

Conversation

6 participants
@jhulten
Contributor

jhulten commented Feb 21, 2014

Change adds two methods to client:

  • AddEventListener adds a channel to receive *APIEvents
    If this is the first listener, event monitoring is started.
  • RemoveEventListener causes a channel to no longer receive *APIEvents
    If this is the last listener, event monitoring is stopped.
Jeffrey Hulten
support listeners for events
Change adds two methods to client:

- AddEventListener adds a channel to receive *APIEvents
  If this is the first listener, event monitoring is started.
- RemoveEventListener causes a channel to no longer receive *APIEvents
  If this is the last listener, event monitoring is stopped.
@jhulten

This comment has been minimized.

Show comment
Hide comment
@jhulten

jhulten Feb 21, 2014

Contributor

Addresses #66

/cc @fsouza

Contributor

jhulten commented Feb 21, 2014

Addresses #66

/cc @fsouza

@cheneydeng

This comment has been minimized.

Show comment
Hide comment
@cheneydeng

cheneydeng Feb 24, 2014

Contributor

@jhulten hey man, are you still working on this feature?The CI still failed.
this is the feature which i need, please fix this :-) thank you very much,if you have no time recently,i think you can cancel this pr and i'm glad to contribute on it.

Contributor

cheneydeng commented Feb 24, 2014

@jhulten hey man, are you still working on this feature?The CI still failed.
this is the feature which i need, please fix this :-) thank you very much,if you have no time recently,i think you can cancel this pr and i'm glad to contribute on it.

@jhulten

This comment has been minimized.

Show comment
Hide comment
@jhulten

jhulten Feb 24, 2014

Contributor

I will work on it today.

Contributor

jhulten commented Feb 24, 2014

I will work on it today.

@jhulten

This comment has been minimized.

Show comment
Hide comment
@jhulten

jhulten Feb 24, 2014

Contributor

There we go. I think I got it all. Quite the learning experience.

/cc @fsouza Ready to merge.

Contributor

jhulten commented Feb 24, 2014

There we go. I think I got it all. Quite the learning experience.

/cc @fsouza Ready to merge.

@cheneydeng

This comment has been minimized.

Show comment
Hide comment
@cheneydeng

cheneydeng Feb 28, 2014

Contributor

@fsouza
hey man ,how is the progress about this usefull feature? Please give a review on it,thanks :-)

Contributor

cheneydeng commented Feb 28, 2014

@fsouza
hey man ,how is the progress about this usefull feature? Please give a review on it,thanks :-)

@jhulten

This comment has been minimized.

Show comment
Hide comment
@jhulten

jhulten Mar 3, 2014

Contributor

Ping @fsouza... Is something blocking accepting this PR?

Contributor

jhulten commented Mar 3, 2014

Ping @fsouza... Is something blocking accepting this PR?

@andrewsmedina

This comment has been minimized.

Show comment
Hide comment
@andrewsmedina

andrewsmedina Mar 5, 2014

Collaborator

LGTM

Collaborator

andrewsmedina commented Mar 5, 2014

LGTM

@benmccann benmccann referenced this pull request in flynn-archive/flynn-host Mar 8, 2014

Closed

Use upstream go-dockerclient #4

@benmccann

This comment has been minimized.

Show comment
Hide comment
@benmccann

benmccann Mar 10, 2014

Contributor

It will be great to have have event support. Flynn forked go-dockerclient and added an event stream:
flynn-archive/go-dockerclient@eb04377

I'd like to get Flynn using the upstream go/dockerclient again, so hopefully that would be possible after getting event support here.

Contributor

benmccann commented Mar 10, 2014

It will be great to have have event support. Flynn forked go-dockerclient and added an event stream:
flynn-archive/go-dockerclient@eb04377

I'd like to get Flynn using the upstream go/dockerclient again, so hopefully that would be possible after getting event support here.

@fsouza

This comment has been minimized.

Show comment
Hide comment
@fsouza

fsouza Mar 10, 2014

Owner

Thanks @benmccann.

O liked flynn implementation. What about using it, @jhulten? We would need some tests, of course, but it looks good and simpler.

What do you think?

Owner

fsouza commented Mar 10, 2014

Thanks @benmccann.

O liked flynn implementation. What about using it, @jhulten? We would need some tests, of course, but it looks good and simpler.

What do you think?

@andrewsmedina

This comment has been minimized.

Show comment
Hide comment
@andrewsmedina

andrewsmedina Mar 11, 2014

Collaborator

I liked the flynn implementation but it needs some tests.

Collaborator

andrewsmedina commented Mar 11, 2014

I liked the flynn implementation but it needs some tests.

@jhulten

This comment has been minimized.

Show comment
Hide comment
@jhulten

jhulten Mar 11, 2014

Contributor

Either way.

Contributor

jhulten commented Mar 11, 2014

Either way.

@fsouza

This comment has been minimized.

Show comment
Hide comment
@fsouza

fsouza Mar 11, 2014

Owner

@jhulten do you want to do this?

Owner

fsouza commented Mar 11, 2014

@jhulten do you want to do this?

@jhulten

This comment has been minimized.

Show comment
Hide comment
@jhulten

jhulten Mar 13, 2014

Contributor

I can take a look at submitting a new PR with tests.

The only thing I see I cover in my code that is not covered here is reconnect logic on failure. @benmccann do you have thoughts on this?

Contributor

jhulten commented Mar 13, 2014

I can take a look at submitting a new PR with tests.

The only thing I see I cover in my code that is not covered here is reconnect logic on failure. @benmccann do you have thoughts on this?

@benmccann

This comment has been minimized.

Show comment
Hide comment
@benmccann

benmccann Mar 13, 2014

Contributor

No thoughts from me. I just noticed code in both repos that looked similar, but I didn't write any of it

Contributor

benmccann commented Mar 13, 2014

No thoughts from me. I just noticed code in both repos that looked similar, but I didn't write any of it

@cheneydeng

This comment has been minimized.

Show comment
Hide comment
@cheneydeng

cheneydeng Apr 11, 2014

Contributor

@fsouza @jhulten
what's wrong with this pr?I'm waiting for using that in my project,will this be merged or closed?

Contributor

cheneydeng commented Apr 11, 2014

@fsouza @jhulten
what's wrong with this pr?I'm waiting for using that in my project,will this be merged or closed?

@srid

This comment has been minimized.

Show comment
Hide comment
@srid

srid Apr 11, 2014

Contributor

On Thu, Apr 10, 2014 at 8:23 PM, cheneydeng notifications@github.com wrote:

I'm waiting for using that in my project

i'm in the same situation. would love to see it in master sooner than later.

Contributor

srid commented Apr 11, 2014

On Thu, Apr 10, 2014 at 8:23 PM, cheneydeng notifications@github.com wrote:

I'm waiting for using that in my project

i'm in the same situation. would love to see it in master sooner than later.

@fsouza fsouza merged commit 7de6875 into fsouza:master Apr 12, 2014

1 check passed

default The Travis CI build passed
Details
@fsouza

This comment has been minimized.

Show comment
Hide comment
@fsouza

fsouza Apr 12, 2014

Owner

It was missing docs and the proper code style, but I've fixed these issues and merged it.

Owner

fsouza commented Apr 12, 2014

It was missing docs and the proper code style, but I've fixed these issues and merged it.

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