-
Notifications
You must be signed in to change notification settings - Fork 385
Customize the DNS service (fixes #545) #555
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @slauger thanks for submitting this PR, it looks good so far, just a couple comments :
When we introduce new fields in values/templates we try to create coverage in the bats test to be sure there are no regressions, and validate the new fields work as intended.
Could you add a new test to cover these changes in test/unit/dns-service.bats
?
You can run this via bats test/unit/dns-service.bats
Also it is currently failing the following bats test:
✗ dns/Service: specified clusterIP (in test file test/unit/dns-service.bats, line 84)
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Sorry I didn't see @kschoche's comment.
Thanks for the PR! Can you add tests for these two settings (see https://github.com/hashicorp/consul-helm/blob/master/CONTRIBUTING.md#writing-unit-tests).
Hi, sorry, for the late response. I'm already familiar with the usage of bats and CircleCI. Tests are added. Let me know what you think. I also introduced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
This PR adds some options to enable the customization the DNS service. See #545 for more information.