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

WIP: Simpify traditional access control parameters #183

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexjfisher
Copy link
Member

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.

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Integer $services = 72,
Array[String[1]] $com2sec = [ 'notConfigUser default public' ],
Array[String[1]] $com2sec6 = [ 'notConfigUser default public' ],
Enum['present','absent'] $ensure = 'present',
Copy link
Member

Choose a reason for hiding this comment

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

In most modules we align the =, but I'm not sure if we should enforce this.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's become pretty hard to do this and not have it look ridiculous.

Copy link
Member

@ghoneycutt ghoneycutt Feb 24, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Dan33l Dan33l Feb 25, 2019

Choose a reason for hiding this comment

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

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 alexjfisher force-pushed the simplify_and_undeprecate_trad_auth branch from a20afd1 to ab5c009 Compare February 24, 2019 13:12
Copy link
Member

@ghoneycutt ghoneycutt left a comment

Choose a reason for hiding this comment

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

I do not think we should encourage the use of empty data sets like '', [] and {} and should instead use undef.

@@ -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'],
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this was a step too far. I'll have another think.

@alexjfisher
Copy link
Member Author

I do not think we should encourage the use of empty data sets like '', [] and {} and should instead use undef.

Empty string I agree with, but [] and {} I think are often preferable to undef and this pattern is extremely popular. In this case in particular, where we are defaulting to a value, allowing undef doesn't make sense. I also don't like multiple ways of expressing no traditional access control communities. The acceptance tests already use empty arrays and not undef.

ro_community => [],

@alexjfisher alexjfisher changed the title Simpify traditional access control parameters WIP: Simpify traditional access control parameters Feb 24, 2019
@Dan33l
Copy link
Member

Dan33l commented Feb 25, 2019

The acceptance tests already use empty arrays and not undef.

Oups my fault.

@bastelfreak
Copy link
Member

I do not think we should encourage the use of empty data sets like '', [] and {} and should instead use undef.

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]

@vox-pupuli-tasks
Copy link

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

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