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

allow dhcp::host to set "on EVENT" handlers #216

Merged
merged 1 commit into from
Mar 10, 2019

Conversation

jflorian
Copy link
Contributor

Pull Request (PR) description

This adds 3 new optional parameters to dhcp::host:

  • on_commit
  • on_release
  • on_expiry

Each of these accepts an array of strings representing statements that dhcpd is to perform when that event occurs for that host.

Signed-off-by: John Florian jflorian@doubledog.org

This Pull Request (PR) fixes the following issues

none have been created

@@ -7,6 +7,9 @@
Hash $options = {},
String $comment ='',
Boolean $ignored = false,
Optional[Array[String]] $on_commit = undef,
Copy link
Member

Choose a reason for hiding this comment

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

Please align the = and enforce the minimal string length:

Suggested change
Optional[Array[String]] $on_commit = undef,
Optional[Array[String[1]]] $on_commit = undef,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call on this one. Update coming...

@bastelfreak bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels Mar 8, 2019
Boolean $ignored = false,
Optional[Integer] $default_lease_time = undef,
Optional[Integer] $max_lease_time = undef,
Optional[Array[String[1]]] $on_commit = undef,
Copy link
Member

Choose a reason for hiding this comment

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

ah crap, I was a bit blind when I reviewed this the last time. For array we try to default to empty arrays instead of making them optional. From our review guidelines:

Are there new params with datatype Hash or Array? If possible, they should default to empty Hash/Array instead of undef. You can also enforce the datastructure like Array[String[1]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be nice if the tests caught this. I honestly don't see what difference it makes, but a fix is in the works if it makes this project happy. IMHO, I think undef is far more appropriate to mean "not set", but that's me.

@jflorian jflorian force-pushed the host_on_event branch 2 times, most recently from ece9e95 to a88dc6f Compare March 10, 2019 18:06
This adds 3 new optional parameters to dhcp::host:

* on_commit
* on_release
* on_expiry

Each of these accepts an array of strings representing statements that
dhcpd is to perform when that event occurs for that host.

Signed-off-by: John Florian <jflorian@doubledog.org>
@bastelfreak bastelfreak removed the needs-work not ready to merge just yet label Mar 10, 2019
@bastelfreak
Copy link
Member

thanks @jflorian !

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

Successfully merging this pull request may close these issues.

None yet

2 participants