Skip to content

sd etcd register/deregister implementation#299

Merged
peterbourgon merged 1 commit intogo-kit:masterfrom
xmattstrongx:master
Jul 1, 2016
Merged

sd etcd register/deregister implementation#299
peterbourgon merged 1 commit intogo-kit:masterfrom
xmattstrongx:master

Conversation

@xmattstrongx
Copy link
Copy Markdown
Contributor

@peterbourgon PR started for discussion and code review.

@xmattstrongx
Copy link
Copy Markdown
Contributor Author

I think I see my issue in client_test.go. When I am testing locally it is more like an integration test because I am hitting a locally running etcd session. I need to get my stubs/mocks in order so I can effectively unit test.

@xmattstrongx
Copy link
Copy Markdown
Contributor Author

I have implemented all I can think of. The test suite could definitely use some more love eventually.
Waiting for some PR comments :)

Comment thread sd/etcd/client.go Outdated
if s.Value == "" {
return ErrNoValue
}
if _, err := c.keysAPI.Create(context.Background(), s.Key, s.Value); err != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why context.Background() instead of c.ctx?

@peterbourgon
Copy link
Copy Markdown
Member

Sorry it took me so long to hop on this, thanks very much for the contribution! Lots of nits in there, but one big question about the presence of localhost:2379 in what looks like a unit test. Hit me back with questions!

Comment thread sd/etcd/subscriber_test.go Outdated

func (c *fakeClient) WatchPrefix(prefix string, responseChan chan *stdetcd.Response) {}

func (c *fakeClient) Register(*Service) error {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also removed pointer as parameter in this interface satisfying function declaration

@xmattstrongx
Copy link
Copy Markdown
Contributor Author

The build process seems to be having issues around go getting openzipkin and opentracing. Im running out of mana so Im gonna call it for the day. Im not sure Ill have the access needed to address these build issues.

@peterbourgon
Copy link
Copy Markdown
Member

re: the OpenTracing stuff, that's opentracing/basictracer-go#31

Etcd does not need to be running. Though it does register a new client, if you try to actually access any functions of that client it will return an etcd cluster is unavailable or misconfigured error.

To convey this intent, how about changing "localhost:2379" to "irrelevant:12345"?

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

Successfully merging this pull request may close these issues.

2 participants