search

hudl / fargo

Golang client for Netflix Eureka
MIT License
133 stars 53 forks source link

Added ReregisterInstance to allow registering over an existing registrat... #4

Closed cquinn closed 10 years ago

cquinn commented 10 years ago

...ion

Added DeregisterInstance to all deregistration Added UpdateInstanceStatus to allow an instance to be taken out of service Other rpc and struct changes to match.

Let me know if you think you'd like these changes back and I'll write the tests to go with them. (I'll probably write the test anyway :)

ryansb commented 10 years ago

I'd definitely like to merge these back in, it'd be great if you could write tests for your additions.

Thanks!

cquinn commented 10 years ago

I have written tests, but am having trouble getting the vagrant env with eureka working. The gradle build failed, so I just pulled a binary from the jenkins job linked from the eureka github. But it's giving me 404's on all requests, so that's not good. I'll keep working on that, but let me know if you have any hints..

cquinn commented 10 years ago

Figured out the 404 problem: the vagrant nodes didn't get their eurekas. Now it seems that the eurekas are running, but don't see each other, and then a bunch of the tests fail because of that.

Could you add a little more info on the steps to setup and run the tests? I'm guessing that I need to have the eurekas know about their node*.localdomain names, so I'll try that next...

(I had been doing development against a cluster we have internally, and hadn't tried setting up a vagrant harness like this, so please excuse my dumb questions.)

ryansb commented 10 years ago

I'll add more instructions to the tests. So far they've only really been run on my local setup, so I appreciate the feedback.

ryansb commented 10 years ago

@cquinn I've added testing instructions at https://github.com/hudl/fargo#testing and I've updated the vagrant provisioning script to use the eureka build from Netflix's cloudbees page.

Let me know if the tests work on your vagrant installation.

cquinn commented 10 years ago

Thanks Ryan, that helped a lot, and my Vagrant Eurekas are running well now.

The tests all work except the TestMetadataReading which fails with:

  * /Users/cquinn/ws/go/src/github.com/hudl/fargo/tests/net_test.go
  Line 162:
  Expected: 'AValue'
  Actual:   ''
  (Should be equal)

(my line numbers are different)

It looks like the call to GetString() after the AddMetadataString call ends up doing a parse() which wipes out the parsed map since Raw was never set. Does this test work for you?

ryansb commented 10 years ago

It does work for me, you may have to run in twice (separated by 20 seconds or so) for it to pass. I believe the syncing between eureka servers occasionally causes that test to fail.

cquinn commented 10 years ago

Ah, it now works after a second run. I still don't see how the instance metadata is read back: probably via readAppInto() but I don't see where that is enabled for the tests. Not a big deal.

I'll clean up my files and prepare another PR later today.

cquinn commented 10 years ago

OK, all done, ready for merge I hope.

ryansb commented 10 years ago

Looks great, thanks!