search

test-kitchen / kitchen-vra

A Test Kitchen driver for VMware vRealize Automation
Apache License 2.0
13 stars 22 forks source link

optional use_dns #3

Closed stevehedrick closed 9 years ago

stevehedrick commented 9 years ago

As per the discussion with @adamleff in #2 here's a PR with the use_dns functionality. This solves the use case I had, where my corporate vRA does not manage the IP addresses of vm's, but the server.name value was being pushed to DNS.

use_dns has been added with a default value of false. When false kitchen-vra will stop if vRA does not supply an ip address. When true kitchen-vra will insert the server.name value as the state[:hostname] only if an ip address is not returned by vRA. If an ip address is returned, and use_dns is true the ip address will still be passed to state[:hostname].

spec/vra_spec.rb has been updated with new tests which (I believe) should cover these changes. I've also updated the description on the existing tests around ip to clarify that they are with use_dns set to the default of false.

adamleff commented 9 years ago

Nice! Thanks for submitting this!

On the whole, it looks great. I have a few small suggestions, but I probably won't get to it until Tuesday due to vacation, but we can definitely get this in for you.

Thanks again! I'll get back to you next week!

adamleff commented 9 years ago

@stevehedrick this is awesome! Please let me know if you have any questions or concerns about my comments. I'm more than happy to help out any way I can.

Thanks again for your contribution. Looking forward to helping get this merged in!

stevehedrick commented 9 years ago

I'll get these changes made and should have it back to you today. Thanks @adamleff.

stevehedrick commented 9 years ago

I prefer option 1 (not creating the extra variable). I had originally done it the way I had to mimic what was already being done. It seems silly to create a variable only to test it when we can just use the original. As an added bonus it removes a rubocop warning (class length >125 lines). I know we could have just updated the rubocop file, but it makes me happy we don't have to.

stevehedrick commented 9 years ago

@adamleff Would you prefer the commits squashed?

adamleff commented 9 years ago

No need to squash as long as there aren't any merge commits. Thanks for being receptive to the feedback!

stevehedrick commented 9 years ago

All the requested changes have been implemented. And really, can't argue with good feedback. :) I've also realized that the tests I had for use_dns didn't check for the exception if vRA didn't return the server name, that's been fixed. And I've added a line to the readme documenting the use_dns parameter.

Have a look, let me know if there's any other feedback to make this better.

stevehedrick commented 9 years ago

Yes! (grumble grumble could have sworn I removed those... darn it.)

Sorry about that.

adamleff commented 9 years ago

Well done, @stevehedrick! Thanks again for the contribution - I'm going to merge this now and cut a release.

stevehedrick commented 9 years ago

Awesome! Thank you @adamleff for the help. And glad to be able to give something back to you guys.

adamleff commented 9 years ago

@stevehedrick kitchen-vra-v1.1.0 has been pushed up to RubyGems! Hat tip for you for the feature suggestion and bringing the PR. Please let us know if you run into any issues, and thanks for using the gem!