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

Add support for additional DHCP listen interfaces #399

Merged
merged 1 commit into from
Jan 18, 2018

Conversation

antaflos
Copy link
Contributor

@antaflos antaflos commented Jan 9, 2018

This change adds a new parameter foreman_proxy::dhcp_additional_interfaces which defaults to the empty array []. The parameter's value must be an array of interface names (i.e. an array of strings). When set the managed DHCP server will listen for DHCP requests on these interfaces, in addition to the interface specified in foreman_proxy::dhcp_interface. No subnets will be provisioned for these additional DHCP listen interfaces.

Fixes GH-400.

@antaflos
Copy link
Contributor Author

The failing test is not related to my changes and will probably run successfully if re-triggered. So how do I get Travis to run the tests once more?

@antaflos
Copy link
Contributor Author

I'll close the PR and open it again, that should trigger Travis.

@antaflos antaflos closed this Jan 10, 2018
@antaflos antaflos reopened this Jan 10, 2018
@ekohl
Copy link
Member

ekohl commented Jan 10, 2018

That was https://www.traviscistatus.com/incidents/v4jnp6nkgzqx and I was too late with restarting the test. I'll have a look at the changes later.

@antaflos
Copy link
Contributor Author

The tests are now passing, FWIW :)

@@ -33,7 +33,7 @@
class { '::dhcp':
dnsdomain => $foreman_proxy::dhcp_option_domain,
nameservers => $nameservers,
interfaces => [$foreman_proxy::dhcp_interface],
interfaces => [$foreman_proxy::dhcp_interface] + [$foreman_proxy::dhcp_additional_interfaces].flatten.delete_undef_values,
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know this was possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, Puppet 4 introduced some nice language features :)

@@ -366,6 +368,7 @@
Array[String] $dhcp_option_domain = $::foreman_proxy::params::dhcp_option_domain,
Optional[Array[String]] $dhcp_search_domains = $::foreman_proxy::params::dhcp_search_domains,
String $dhcp_interface = $::foreman_proxy::params::dhcp_interface,
Optional[Variant[Array[String], String]] $dhcp_additional_interfaces = $::foreman_proxy::params::dhcp_additional_interfaces,
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if this was simply Array[String] which defaults to [] in params.pp since it removes the need to clean it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do.

@@ -174,6 +174,8 @@
#
# $dhcp_interface:: DHCP listen interface
#
# $dhcp_additional_interfaces:: Additional DHCP listen interfaces (in addition to dhcp_interface)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add that we don't provision any subnet for this by default (in contrast to dhcp_interface)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do.

@antaflos
Copy link
Contributor Author

I've implemented the requested changes. Please let me know if this needs further work.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

The code looks good, just minor formatting detail left.

@@ -174,6 +174,8 @@
#
# $dhcp_interface:: DHCP listen interface
#
# $dhcp_additional_interfaces:: Additional DHCP listen interfaces (in addition to dhcp_interface). Note: as opposed to dhcp_interface *no* subnet will be provisioned for any of the additional DHCP listen interfaces. Please configure any additional subnets using `dhcp::pool` and related resource types (provided by the theforeman/puppet-dhcp module).
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap this so it's not such a long line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, how should I wrap it? At 140 characters, like this?

# $dhcp_additional_interfaces:: Additional DHCP listen interfaces (in addition to dhcp_interface). Note: as opposed to dhcp_interface *no*
#                               subnet will be provisioned for any of the additional DHCP listen interfaces. Please configure any additional
#                               subnets using `dhcp::pool` and related resource types (provided by the theforeman/puppet-dhcp module).

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good.

This change adds a new parameter
foreman_proxy::dhcp_additional_interfaces which defaults to the empty
array `[]`. The parameter's value must be an array of interface names
(i.e. an array of strings). When set the managed DHCP server will
listen for DHCP requests on these interfaces, in addition to the
interface specified in foreman_proxy::dhcp_interface. No subnets will be
provisioned for these additional DHCP listen interfaces.
@antaflos
Copy link
Contributor Author

Ok, done.

@mmoll mmoll merged commit fcf6d82 into theforeman:master Jan 18, 2018
@mmoll
Copy link
Contributor

mmoll commented Jan 18, 2018

merged, thanks @antaflos!

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

Successfully merging this pull request may close these issues.

4 participants