WIP: Simpify traditional access control parameters #183
Conversation
| Integer $services = 72, | ||
| Array[String[1]] $com2sec = [ 'notConfigUser default public' ], | ||
| Array[String[1]] $com2sec6 = [ 'notConfigUser default public' ], | ||
| Enum['present','absent'] $ensure = 'present', |
There was a problem hiding this comment.
In most modules we align the =, but I'm not sure if we should enforce this.
There was a problem hiding this comment.
It's become pretty hard to do this and not have it look ridiculous.
There was a problem hiding this comment.
With regards to styling of class parameters, I'm no longer doing any special alignment between the data type, parameter and value and am only doing a single space between each.
Data types break down the ability to have a style that makes the code readable. With the advent of REFERENCE.md, you can always look there to see all the parameters and their default values.
There was a problem hiding this comment.
Align the = is noisy when using git diff.
Due to this, IMO we should reconsider our default policy about this.
I am in favor of no longer aligning the =
Instead of allowing `undef`, plain strings or arrays of strings, just allow arrays of strings. Parameters that previously defaulted to `undef` now default to empty arrays. This allows us to simplify the templates and we no longer have `Optional` parameters that have default values set. (`ro_community` defaulting to `'public'` but allowing `undef` didn't make much sense previously.) View-based Access Control Model (VACM) is still recommended, but traditional access control is no longer deprecated.
alexjfisher
force-pushed
the
simplify_and_undeprecate_trad_auth
branch
from
a20afd1
to
ab5c009
Compare
| @@ -92,8 +76,8 @@ To change the SNMP community from the default value and limit the netblocks that | |||
| ```puppet | |||
| class { 'snmp': | |||
| agentaddress => [ 'udp:161', ], | |||
| ro_community => 'myPassword', | |||
| ro_network => '192.168.0.0/16', | |||
| ro_community => ['myPassword'], | |||
There was a problem hiding this comment.
I don't see the point in this as it breaks backward compatibility though in the next release you want to deprecate the option anyhow.
There was a problem hiding this comment.
Maybe this was a step too far. I'll have another think.
Empty string I agree with, but puppet-snmp/spec/acceptance/snmp_spec.rb Line 36 in a27f22d |
alexjfisher
changed the title
Oups my fault. |
From our review guidelines:
|
|
Dear @alexjfisher, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
Instead of allowing
undef, plain strings or arrays of strings, justallow arrays of strings. Parameters that previously defaulted to
undefnow default to empty arrays.
This allows us to simplify the templates and we no longer have
Optionalparameters that have default values set. (ro_communitydefaulting to
'public'but allowingundefdidn't make much sensepreviously.)
View-based Access Control Model (VACM) is still recommended, but
traditional access control is no longer deprecated.
Pull Request (PR) description
This Pull Request (PR) fixes the following issues