Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added option dns search domain #96

Merged
merged 1 commit into from
Jun 21, 2016

Conversation

jskarpe
Copy link
Contributor

@jskarpe jskarpe commented Jun 20, 2016

No description provided.

@alexjfisher
Copy link
Member

@yuav - Thanks!
Are you able to add a spec test for this?

@jskarpe
Copy link
Contributor Author

jskarpe commented Jun 20, 2016

Fixed

@@ -15,6 +15,9 @@ log-facility <%= @logfacility %>;
# ----------
option domain-name "<%= @dnsdomain_real.first %>";
option domain-name-servers <%= @nameservers.join(', ') %>;
<% if @dnssearchdomains -%>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an empty array is also true...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, future parser. Nice catch!

I'll add another test :-)

@jskarpe
Copy link
Contributor Author

jskarpe commented Jun 20, 2016

Should be less bad now

@@ -15,6 +15,9 @@ log-facility <%= @logfacility %>;
# ----------
option domain-name "<%= @dnsdomain_real.first %>";
option domain-name-servers <%= @nameservers.join(', ') %>;
<% if @dnssearchdomains && @dnssearchdomains != [] -%>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if @dnssearchdomains && [email protected]? is possibly better from a style perspective?

Also matches other templates. See https://github.com/voxpupuli/puppet-dhcp/blob/master/templates/dhcpd.conf.ntp.erb#L2

@jskarpe
Copy link
Contributor Author

jskarpe commented Jun 20, 2016

Noted, fixed

@alexjfisher
Copy link
Member

@yuav Great! Looking good to me. Looks like you need a rebase though. Could you also squash your commits? Thanks.

@jskarpe
Copy link
Contributor Author

jskarpe commented Jun 21, 2016

Rebased.

@alexjfisher
Copy link
Member

@yuav Thanks, but it's still showing conflicts. Perhaps you only rebased against your local master?? Did you forget to fetch and merge upstream master into your master first?

@jskarpe
Copy link
Contributor Author

jskarpe commented Jun 21, 2016

History was messed up. Should be less broken now

@alexjfisher alexjfisher merged commit 222f063 into voxpupuli:master Jun 21, 2016
@jskarpe jskarpe deleted the dnssearchdomain branch June 22, 2016 07:49
)
end

it 'has domain-search option with empty array' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an extremely confusingly named test, it suggests that the expected result is a domain-search option with nothing after it except an empty array, which I don't believe can actually be represented in dhcpd.conf.

The test however checks that the domain-search option is absent entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed in PR #115

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants