Fix issue https://github.com/cloudfoundry/bosh/issues/2236 #2246
Conversation
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/171513194 The labels on this github issue will be updated when the story is started. |
|
Hi @gu-bin There are also unit tests for the ERB templating. Please update the tests and ensure |
|
Might also want to add the domain to the local_dns feature in the /info response for consistency https://github.com/cloudfoundry/bosh/blob/master/src/bosh-director/lib/bosh/director/api/controllers/info_controller.rb#L26-L32 |
|
@gu-bin we'll be able to merge this once the CLA passes, there are tests, and the recommended addition. Thanks. |
|
@xtreme-conor-nosal @h4xnoodle I've addressed all the comments. Please review again. Thanks. |
|
Anything else do I need to do to get this PR merged? |
| params['dns']['domain_name'] = domain_name | ||
| end | ||
| if_p('dns.domain_name') do |domain_name| | ||
| params['dns'] = { |
There was a problem hiding this comment.
won't this assignment drop any other dns fields that were set? The powerdns case still needs to be able to set the domain_name. This block should probably create the hash only if it's nil, then insert the value. Please add a test for that case as well.
|
@xtreme-conor-nosal @h4xnoodle Please review again. This time I didn't modify the original powerdns case but only add |
|
@xtreme-conor-nosal @h4xnoodle Any comments for this? |
|
@xtreme-conor-nosal @h4xnoodle Any update for this PR? We need it to be fixed. Thanks. |
src/bosh-director/lib/bosh/director/api/controllers/info_controller.rb
Outdated
Show resolved
Hide resolved
|
Both people you've requested for review have rotated off the team, so I apologize for the delay getting back to you. |
|
@mrosecrance I've addressed your comments. Anything else do I need to do before it can be merged? |
This is to fix issue #2236