search

Netflix / denominator

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

Adding Verisign MDNS provider to Denominator #268

Closed sanjaympawar1 closed 9 years ago

sanjaympawar1 commented 9 years ago

Hi,

Please review this pull request as per our earlier discussion (mail) regarding Verisign MDNS plugin for Netflix Denominator, I tried to create feature branch however but settle down for a fork as I do not have permission for it. We are supporting basic Resource Record operations (list, add, delete) and zone list in this phase of development.

We have tested zone list and add, create, delete operation of Resource Records supported in this release. (Unfortunately Github does not allow to add test execution files as attachment) Please let us know your questions and concerns.

-Thanks

cloudbees-pull-request-builder commented 9 years ago

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

codefromthecrypt commented 9 years ago

Firstly, forking is the correct way to develop, although you should be using a feature branch on your fork, not master. That's because you'll notice revisions are needed, and you don't want to taint master when you squash them into a single commit for merge.

Second, there are a lot of missed opportunities here, developing this provider from scratch as opposed to using ultradns as an example. In both cases these are soap apis, so have very much in common. When you use existing code as an example, it lowers the burden on you and also on the reviewer as we don't have to revisit topics discussed in the past.

With regards to the implementation, I'm glad there are mock tests as this helps for those who don't have an account. However, there must be live tests present so that at least you can test that this works against a real server (without making ad-hoc classes). Please refer to other providers where LiveTests are present and operate against the base classes.

Based on this review pass, I'd suggest there's a significant amount of cleanup needed before merge. you may be best creating a new branch and starting the review again after addressing the feedback.

Best luck and thanks for the start.

codefromthecrypt commented 9 years ago

superceded by #270 Let's make sure the comments here are addressed in that one!