Support for DiscoveryDNS

[wnagele]

This adds support for the DiscoveryDNS product.

Commit 838bcc5 makes two methods for handling RData available in a newly created module 'common'. I have created this module as it was indicated that Guava is not supposed to be in the 'model' and 'core' modules to avoid dependency issues.

[cloudbees-pull-request-builder]

denominator-pull-requests #354 FAILURE Looks like there's a problem with this pull request

[wnagele]

Fails because of the missing Feign release.

[cloudbees-pull-request-builder]

denominator-pull-requests #355 FAILURE Looks like there's a problem with this pull request

[codefromthecrypt]

Providers should not depend on guava, so I personally am not keen on this change. Can you implement what you need without guava?

[codefromthecrypt]

Util should have all you need to complete the change you wanted without adding a guava dep or complicating the build by making a common module. https://github.com/Netflix/denominator/blob/master/model/src/main/java/denominator/common/Util.java

[codefromthecrypt]

Formatting is distractingly different. Can you adjust to 2 spaces etc?

[codefromthecrypt]

Scratch above comment! I suppose I am the one distracting.. didnt realize this project is 4 space.

[codefromthecrypt]

Looks like mock tests are needed, especially since I doubt other committers have an account to test live.

[wnagele]

These two methods I moved to the 'common' module are also using plenty of Guava code. That and all of the DiscoveryDNS code could obviously be refactored to not use Guava but it'll be quite some effort. I am still not quite sure that I completely get the "fear" of Guava and if it justifies all the effort spent in these projects re-inventing the wheel. Could you elaborate?

The mock tests I can put in, no problems.

[codefromthecrypt]

Sure. You can look at issue history for context on guava, but here is a recap.

We spent considerable time removing guava from providers because denominator is used in plaform code. Guava versions 11+ are used in platform code, so it is very hard to guarantee there will not be a conflict. Guava is used in the cli since it is used as a standalone tool, not a library, hence no chance of conflict.

Since discovery dns isnt used by netflix, you can choose to add the dependency, and if someone else merges this, fine by me. I highly doubt there is a pattern you aee using that is so wildly different than the other providers that you cannot get by without guava, but again your call.

Definitely, I wont merge this with a common module extracted as we already have a lean model module which has common classes.

[codefromthecrypt]

@allenxwang @jdamick if either of you want to play "good cop", here's a good thread to do so in :)

[allenxwang]

I agree with @adriancole that we should strive to put common pieces in one module "core". Having another "common" module is confusing. It will be great if we can manage to move forward with this change without dependency change.

[jdamick]

+1 agreed, seems overly abstracted..

[wnagele]

I'll look into this - will require a bit of work so please be patient. :)

[cloudbees-pull-request-builder]

denominator-pull-requests #356 FAILURE Looks like there's a problem with this pull request

[cloudbees-pull-request-builder]

denominator-pull-requests #357 FAILURE Looks like there's a problem with this pull request

[cloudbees-pull-request-builder]

denominator-pull-requests #358 FAILURE Looks like there's a problem with this pull request

[wnagele]

Allright. I've now removed the 'common' module and instead refactored all of the relevant code without Guava. I have also added Mock coverage for DiscoveryDNS so it can be tested without API access.

I still have the problem that Feign hasn't been released with their bug 85 fixed. 6.1.1 was released recently but didn't include that fix (why I am not sure).

Cloudbees can't build this as it seems to have trouble merging my squashed pull request. Needs a manual full build run without merge. Locally it builds and tests all clean.

[wnagele]

Ping. :)

[jdamick]

Looks like something is up with the pull, it needs to merge cleanly.. Think you need to rebase from master..

[wnagele]

Please read my previous comments. Feign has been released as 6.1.1 but for some reason the bugfix 85 wasn't included. This is why it fails the build - once that is sorted I can fix the merge. Can you guys arrange for a new release of Feign?

[codefromthecrypt]

Ping @allenxwang for a new version of feign

[allenxwang]

Is the fix merged to 6.x release branch?

[wnagele]

It's not at the moment. @adriancole do you want me to create a pull request for that?

[codefromthecrypt]

Yes! Please raise a pr on 6.x cherry picking the commits needed

[wnagele]

Raised: https://github.com/Netflix/feign/pull/103

[cloudbees-pull-request-builder]

denominator-pull-requests #359 FAILURE Looks like there's a problem with this pull request

[cloudbees-pull-request-builder]

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

[wnagele]

Now that the bug was fixed in Feign and released with 6.1.2 I have upgraded Feign in here and adjusted the few places where it needed.

Also brought the commits forward so they cleanly apply against master.

[wnagele]

Ping. Any issue with merging this now?

[wnagele]

@adriancole Happy to merge this?

[codefromthecrypt]

can you un-tab this? Let's keep code style consistent.

[cloudbees-pull-request-builder]

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

[wnagele]

@adriancole done. sorry, should have noticed myself that the code style was messed up.

[codefromthecrypt]

new method in Util and ResourceRecordSetCommands could use a test.

[codefromthecrypt]

Looks like we are at the final lap. Let's properly isolate the provider from how it gets its private key. I highly suggest using PEM as this naturally with fit in the CLI yaml file. To that end I put links on how to do this, as jclouds has this approach in place and tested for reasons including isolating providers from scary things like filesystem i/o and making config isolated to a single file as opposed to a file system.

[wnagele]

@adriancole Your point about the filesystem dependency is totally valid. Whats your thought on dragging in that code from JClouds? I know you guys want to avoid external dependencies where possible - C&P all of the relevant code seems like a mess though.

[wnagele]

@adriancole looking more into the JClouds implementation - it's riddled with Google Guava for it's implementation. getting this in without Guava is gonna be a nightmare. what about using Bouncycastle - it has a PEM reader that'll do the trick.

[cloudbees-pull-request-builder]

denominator-pull-requests #362 FAILURE Looks like there's a problem with this pull request

[cloudbees-pull-request-builder]

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

[wnagele]

@adriancole worked in all of your feedback and rebased against the current master for a clean merge. let me know if there is anything else you would like done here.

[codefromthecrypt]

LG with nits

[codefromthecrypt]

I think the gson package is a bit overdoing it, since there's nothing in there that has anything to do with gson, except one unnecessary annotation. I'd flatten it, mark classes that don't need to be public as package private (read effective java for rationale), then good to merge!

[cloudbees-pull-request-builder]

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

[cloudbees-pull-request-builder]

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

[cloudbees-pull-request-builder]

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

[wnagele]

@adriancole all done as far as i can see.

[codefromthecrypt]

Hmm I seem to be without karma again. @allenxwang you cool to merge? I am.

Great job.

[wnagele]

Cool. Also seems to be a new record on the number of comments for a pull request in this project. :P

[codefromthecrypt]

:1234: