Convert from params to data in module #181
Conversation
ghoneycutt
force-pushed
the
data_in_module
branch
from
96ebda6
to
7caea2b
Compare
ghoneycutt
changed the title
ghoneycutt
force-pushed
the
data_in_module
branch
from
7caea2b
to
ec67513
Compare
| Enum['present','absent'] $ensure = 'present', | ||
| Array[String[1]] $agentaddress = [ 'udp:127.0.0.1:161', 'udp6:[::1]:161' ], | ||
| Array[String[1]] $snmptrapdaddr = [ 'udp:127.0.0.1:162', 'udp6:[::1]:162' ], | ||
| Variant[Undef, String[1], Array[String[1]]] $ro_community = 'public', |
There was a problem hiding this comment.
does undef make sense here? As far as I know, you can pass ~ as a parameter, but than puppet will fallback to the default data, 'public' in this case. IMO the default needs to undef, or remove undef from the Variant.
There was a problem hiding this comment.
I did that to preserve existing functionality.
See these tests where ro_community is an empty array.
puppet-snmp/spec/acceptance/snmp_spec.rb
Line 36 in 021c1b2
There was a problem hiding this comment.
I agree that if the default isn’t undef, allowing undef is almost always wrong.
@bastelfreak using ~ in hiera would be the only way to override it to undef. You couldn’t do this via a class declaration though (where it’d be interpreted as use the default). What does the code do if it’s undef vs empty array?
There was a problem hiding this comment.
[*@ro_community].compact.each eew.
Splat operator, does that return anything for nil?? I guess so otherwise the rw_community default wouldn’t work.
So some users might be using hiera to set undef, but people declaring the class have to use an empty array.
Why don’t we remove all the mess and inconsistency by only allowing arrays, (including empty arrays)? This is a breaking change, but this is for a new major release anyway.
@ghoneycutt as you’ve seen in #170 removing all of this functionality has been threatened for a while. The readme actually says we’ll do this in 5.0. So let’s officially undeprecate traditional access control, but also simplify the code and template in the process as a compromise?
There was a problem hiding this comment.
I just wanted to do this piece of work to give back to the community and modernize the module a bit without changing the tests. I've gone as far as I'd like to with this module. Feel free to base your work on top of mine with additional commits.
There was a problem hiding this comment.
That’s fair enough and your improvements to this module are greatly appreciated.
I do think @bastelfreak was ok to point out allowing undef whilst defaulting to a value is possibly suspect and worth investigating. If worth doing, the simplification I suggested could always be done in a separate PR, (but before the 5.0 release).
There was a problem hiding this comment.
#170 is a separate issue. It would seem we need to keep the old functionality. Anyhow, let's keep that discussion over there.
There was a problem hiding this comment.
BTW, undef for ro_community does seem wrong. I was purposely coding against the tests that were already present as opposed to making feature enhancements.
| Variant[Undef, String[1], Array[String[1]]] $ro_community6 = 'public', | ||
| Variant[Undef, String[1], Array[String[1]]] $rw_community = undef, | ||
| Variant[Undef, String[1], Array[String[1]]] $rw_community6 = undef, | ||
| Variant[Array, Stdlib::IP::Address::V4, Stdlib::IP::Address::V4::CIDR] $ro_network = '127.0.0.1', |
There was a problem hiding this comment.
The array also only supports the ip types right? Can you move this to a custom datatype and enforce the datatypes within the array as well?
There was a problem hiding this comment.
Agreed this should happen, though maybe you can in version 5.1 :)
This is still a vast improvement on what was there, which is no type validation.
| @@ -21,16 +21,22 @@ disableAuthorization <%= @disable_authorization %> | |||
| ################################################################################ | |||
| # NOTIFICATION PROCESSING | |||
|
|
|||
| <% @trap_handlers.each do |handler| -%> | |||
| <% if not @trap_handlers.nil? -%> | |||
There was a problem hiding this comment.
you're probably not interested in turning this into an epp template, are you? :D
totally not required, but always nice to see.
There was a problem hiding this comment.
Not in the slightest.. Unfortunately I don't see any value in doing that conversion.
There was a problem hiding this comment.
My goal with this PR was to deprecate params.pp in favor of data in module. Part of that included adding data types to all the parameters. I want to keep this PR as small as possible to reach that goal.
|
Could this please be merged and then other changes can be made before the 5.0.0 breaking change release. |
|
This PR deprecates params.pp which is a private class and is coded to the existing spec tests, so actually we don't need a major version bump to merge this. Would be nice to see this merged as it is very de-motivating to spend a day doing a change this big to watch it just sit there. It also makes it really hard for others to contribute any changes to the module. |
|
Sorry, been super busy with new client. Will find some time this weekend to get this in. |
Needed for newer types such as `Stdlib::IP::Address::V4::CIDR`.
|
I'm going to merge this and make the changes I suggested in a separate PR. |
Use data in modules and specify data types for all parameters