search

Netflix / denominator

Portably control DNS clouds using java or bash
Apache License 2.0
580 stars 110 forks source link

Add some more exotic resource record types #256

Closed wnagele closed 10 years ago

cloudbees-pull-request-builder commented 11 years ago

denominator-pull-requests #347 SUCCESS This pull request looks good

codefromthecrypt commented 11 years ago

Presumably, you need these resources? :)

If so, you mind sweeping the providers to see which support them? Hint: classes named like RouteProvider list supported types.

wnagele commented 11 years ago

Some of them are used by other providers already - DS is supported by many of them.

I've added this change because I am working on a provider for our DiscoveryDNS (http://www.discoverydns.com) product which is about to be released soon and it supports all of these.

codefromthecrypt commented 11 years ago

Oh ok. Heres a couple things

  1. Switch guava back to testCompile as main code cant depend on it. Guava is a contensious dep when multiple versions exist.
  2. Add tests to model, if you havent already.
  3. In core, there are base test classes that create sample records of each supported type on real services. Update this to include your new types so that they are automatically tested.

Make sense?

wnagele commented 11 years ago

Can you add some more details for the third point? Not sure where to add what you are after here.

codefromthecrypt commented 11 years ago

sure.

you'll want to update the base set of test data here: https://github.com/Netflix/denominator/blob/master/core/src/test/java/denominator/BaseProviderLiveTest.java#L114

In order to make this testable, you'll need to update Mock to override basicRecordTypes and include your new types here: https://github.com/Netflix/denominator/blob/master/core/src/main/java/denominator/mock/MockProvider.java#L60

cloudbees-pull-request-builder commented 10 years ago

denominator-pull-requests #348 SUCCESS This pull request looks good

wnagele commented 10 years ago

I think these should now cover all of the things you wanted adjusted.

cloudbees-pull-request-builder commented 10 years ago

denominator-pull-requests #349 SUCCESS This pull request looks good

codefromthecrypt commented 10 years ago

ok. a few more things to ensure this code is exercised. If you don't mind, squash the commits on next round!

codefromthecrypt commented 10 years ago

Ps dont forget to update CHANGES.MD

wnagele commented 10 years ago

Latest commit should address all the feedback that I can fix.

cloudbees-pull-request-builder commented 10 years ago

denominator-pull-requests #350 SUCCESS This pull request looks good

wnagele commented 10 years ago

Anything else you want here before merging in?

wnagele commented 10 years ago

Getting a bit annoyed here. Please let me know if you don't intend to merge this - in that case I'd rather run it as a fork than keeping this pull request open.

codefromthecrypt commented 10 years ago

@wnagele personally, I have one question about the removed test, as there should not be any removed tests for a change. Otherwise, I'm fine if @jdamick is.

jdamick commented 10 years ago

Yeah, me too.

wnagele commented 10 years ago

Well spotted - this was by accident. I've re-added this. If you are happy I can squash and rebase for merge?

cloudbees-pull-request-builder commented 10 years ago

denominator-pull-requests #351 SUCCESS This pull request looks good

codefromthecrypt commented 10 years ago

Yeah can you squash? Weird that this PR built fine and the other didnt!!

cloudbees-pull-request-builder commented 10 years ago

denominator-pull-requests #352 SUCCESS This pull request looks good

wnagele commented 10 years ago

All done and ready to merge.

cloudbees-pull-request-builder commented 10 years ago

denominator-pull-requests #353 SUCCESS This pull request looks good

codefromthecrypt commented 10 years ago

Weird.. merge button isn't visible. @allenxwang can you merge this?

wnagele commented 10 years ago

Ping. @allenxwang @adriancole - anything I can/need to fix?

allenxwang commented 10 years ago

@wnagele @adriancole just merged the pull request.