Add explicit parameter types to all parameters #1189

JacobHenner · 2018-03-15T03:02:08Z

This is a WIP to specify parameter datatypes where missing, and reconsider some potentially inconsistent types.

JacobHenner · 2018-03-15T04:30:00Z

As I continue to work through this, I notice that there are a bunch of odd situations that should probably discussed.

Intermingling boolean and 'on'/'off' strings.
Strings being used for some fields that could use integers, but not others
The implicit use of Enum['opt 1', 'opt 2', false] rather than Optional[Enum['opt1 ', 'opt 2']
Optional[Boolean]

I'll add examples in contextual comments once I get my branch passing unit tests.

ekohl · 2018-03-15T10:21:13Z

In #1155 I started doing the same. Could be good to align the two.

JacobHenner · 2018-03-15T11:01:58Z

Agreed. After I update my branch to pass all tests, I think it would be best to gather consensus regarding the odd situations I mentioned, to determine if the definition or behavior within the module needs to change.

ekohl · 2018-03-15T11:09:17Z

For now I added the backwards-incompatible label because I don't know if any type changed. This may not be true and we could remove it later. I'd be happy to change some parameters so the types are consistent and parameters aligned.

As I continue to work through this, I notice that there are a bunch of odd situations that should probably discussed.

I noticed those as well when I started to work on it. I'd be happy if we could be consistent.

JacobHenner · 2018-03-16T04:21:48Z

Pending #1190 and #1191 (to confirm the final failing spectest isn't related to this change).

After confirmation, I'll comment on the individual changes and seek consensus for the ambiguous/poorly typed parameters.

JacobHenner · 2018-03-17T15:51:38Z

Now that #1190 is fixed, it looks like all tests are passing. When I have some time (hopefully this weekend), I will comment on the cases that should be discussed.

JacobHenner · 2018-03-20T01:00:10Z

One pattern I've noticed a bunch of times is the use of strings "on" and "off" to match nginx's config representation, rather than booleans. Do we want to standardize these values, and restrict these parameters to Enum["on","off"], or should a breaking change be introduced to set these as boolean parameters?

ekohl · 2018-03-20T11:40:07Z

That's something I've struggled with as well. I believe there are option that accept on, off and others which would argue in favor of the Enum solution. For now I've followed that, but I'd be open to other solutions.

JacobHenner · 2018-03-28T00:45:04Z

Is there any consensus on whether Enums with ["on","off"], booleans, or the current hybrid of the two and the optional type should be used? Since this will be a breaking change, I want to make sure there's some agreement before standardizing it one way or another.

JacobHenner · 2018-04-21T01:59:26Z

I believe this is ready for review and discussion.

bastelfreak · 2018-04-21T18:57:42Z

manifests/init.pp

+  Optional[String[1]] $daemon_group                                             = undef,
+  String[1] $global_owner                                                       = $::nginx::params::global_owner,
+  String[1] $global_group                                                       = $::nginx::params::global_group,
+  Stdlib::Filemode $global_mode                                                 = $::nginx::params::global_mode,


great that we finally have a datatype for this

Can you, in a seperate PR, bump the stdlib requirement to at least 4.22.0 in metadata.json?

bastelfreak · 2018-04-21T19:02:44Z

manifests/init.pp

+  Variant[String[1], Boolean] $pid                                              = $::nginx::params::pid,
+  String[1] $proxy_temp_path                                                    = $::nginx::params::proxy_temp_path,
+  String[1] $root_group                                                         = $::nginx::params::root_group,
+  String[1] $run_dir                                                            = $::nginx::params::run_dir,


We can you Stdlib::Absolutepath here?

bastelfreak · 2018-04-21T19:26:56Z

manifests/resource/server.pp

@@ -300,10 +300,10 @@
  # Check to see if SSL Certificates are properly defined.
  if $ssl {
    if $ssl_cert == undef {
-      fail('nginx: ssl_cert must be set to false or to a fully qualified path')
+      warning('nginx: ssl enabled but ssl_cert undef')


why did you change this? Doesn't a fail() make more sense?

bastelfreak · 2018-04-21T19:32:57Z

manifests/resource/streamhost.pp

+  Optional[String[1]] $proxy                                  = undef,
+  Nginx::Duration $proxy_read_timeout                         = $::nginx::proxy_read_timeout,
+  Nginx::Duration $proxy_connect_timeout                      = $::nginx::proxy_connect_timeout,
+  Array[String[1]] $resolver                                  = [],


now resolver is a required parameter, it needs to contain a string, this wasn't the case before. Should we change it back?

JacobHenner · 2018-06-14T01:38:13Z

Is there still interest in this?

bastelfreak · 2018-06-17T19:44:25Z

There sure is! I raised a few inline questions within the code, can you take a look? In the meantime we also defined some guidelines for a consistent code style: https://voxpupuli.org/docs/#reviewing-a-module-pr

ekohl · 2018-10-01T16:30:44Z

To get this merged I think it's easier if we split this up into manageable pieces. Would you be willing to submit a PR per file?

bastelfreak · 2019-06-10T10:36:16Z

Ping @JacobHenner :)

vox-pupuli-tasks · 2019-12-04T14:44:21Z

Dear @JacobHenner, thanks for the PR!

This is pccibot, 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

vox-pupuli-tasks · 2019-12-04T14:44:22Z

Dear @JacobHenner, thanks for the PR!

This is pccibot, 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

vox-pupuli-tasks · 2020-01-05T13:50:03Z

Dear @JacobHenner, thanks for the PR!

This is pccibot, 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

vox-pupuli-tasks · 2020-01-05T13:54:52Z

Dear @JacobHenner, thanks for the PR!

This is pccibot, 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

ekohl · 2020-08-30T22:18:10Z

Replacement in #1413

JacobHenner force-pushed the add_parameter_types branch from b21e1b2 to a06ecf8 Compare March 15, 2018 03:07

ekohl added the backwards-incompatible label Mar 15, 2018

JacobHenner force-pushed the add_parameter_types branch from 1631dc2 to 60b171b Compare March 17, 2018 13:19

bastelfreak added the needs-work not ready to merge just yet label Mar 17, 2018

bastelfreak mentioned this pull request Mar 29, 2018

explain params that map to config options voxpupuli/voxpupuli.github.io#129

Merged

JacobHenner force-pushed the add_parameter_types branch 6 times, most recently from 1b08230 to 72f4f98 Compare April 4, 2018 03:00

JacobHenner force-pushed the add_parameter_types branch from 72f4f98 to cfed312 Compare April 14, 2018 12:19

JacobHenner closed this Apr 21, 2018

JacobHenner reopened this Apr 21, 2018

JacobHenner changed the title ~~WIP: Add explicit parameter types to all parameters~~ Add explicit parameter types to all parameters Apr 21, 2018

JacobHenner force-pushed the add_parameter_types branch from cfed312 to 1a30582 Compare April 21, 2018 02:04

bastelfreak reviewed Apr 21, 2018

View reviewed changes

JacobHenner force-pushed the add_parameter_types branch from 1a30582 to 3677b16 Compare April 21, 2018 19:29

bastelfreak reviewed Apr 21, 2018

View reviewed changes

JacobHenner mentioned this pull request Apr 21, 2018

Update minimum version of puppetlabs/stdlib to 4.22.0 #1207

Merged

JacobHenner force-pushed the add_parameter_types branch from 3677b16 to cfc9f32 Compare April 22, 2018 11:40

JacobHenner force-pushed the add_parameter_types branch from cfc9f32 to 8f12b01 Compare May 31, 2018 18:51

Add explicit parameter types to all parameters

24f7831

JacobHenner force-pushed the add_parameter_types branch from 8f12b01 to 24f7831 Compare June 1, 2018 01:51

towo added the needs-rebase label Mar 26, 2019

vox-pupuli-tasks bot added the merge-conflicts label Dec 4, 2019

dhoppe removed the merge-conflicts label Jan 5, 2020

vox-pupuli-tasks bot added the merge-conflicts label Jan 5, 2020

dhoppe removed the merge-conflicts label Jan 5, 2020

vox-pupuli-tasks bot added the merge-conflicts label Jan 5, 2020

ekohl mentioned this pull request Aug 30, 2020

Add stricter parameter types #1413

Draft

ekohl closed this Aug 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit parameter types to all parameters #1189

Add explicit parameter types to all parameters #1189

JacobHenner commented Mar 15, 2018

JacobHenner commented Mar 15, 2018 •

edited

Loading

ekohl commented Mar 15, 2018

JacobHenner commented Mar 15, 2018 via email

ekohl commented Mar 15, 2018

JacobHenner commented Mar 16, 2018

JacobHenner commented Mar 17, 2018

JacobHenner commented Mar 20, 2018

ekohl commented Mar 20, 2018

JacobHenner commented Mar 28, 2018

JacobHenner commented Apr 21, 2018

bastelfreak Apr 21, 2018

bastelfreak Apr 21, 2018

bastelfreak Apr 21, 2018

bastelfreak Apr 21, 2018

bastelfreak Apr 21, 2018

JacobHenner commented Jun 14, 2018

bastelfreak commented Jun 17, 2018

ekohl commented Oct 1, 2018

bastelfreak commented Jun 10, 2019

vox-pupuli-tasks bot commented Dec 4, 2019

vox-pupuli-tasks bot commented Dec 4, 2019

vox-pupuli-tasks bot commented Jan 5, 2020

vox-pupuli-tasks bot commented Jan 5, 2020

ekohl commented Aug 30, 2020

Add explicit parameter types to all parameters #1189

Add explicit parameter types to all parameters #1189

Conversation

JacobHenner commented Mar 15, 2018

JacobHenner commented Mar 15, 2018 • edited Loading

ekohl commented Mar 15, 2018

JacobHenner commented Mar 15, 2018 via email

ekohl commented Mar 15, 2018

JacobHenner commented Mar 16, 2018

JacobHenner commented Mar 17, 2018

JacobHenner commented Mar 20, 2018

ekohl commented Mar 20, 2018

JacobHenner commented Mar 28, 2018

JacobHenner commented Apr 21, 2018

bastelfreak Apr 21, 2018

Choose a reason for hiding this comment

bastelfreak Apr 21, 2018

Choose a reason for hiding this comment

bastelfreak Apr 21, 2018

Choose a reason for hiding this comment

bastelfreak Apr 21, 2018

Choose a reason for hiding this comment

bastelfreak Apr 21, 2018

Choose a reason for hiding this comment

JacobHenner commented Jun 14, 2018

bastelfreak commented Jun 17, 2018

ekohl commented Oct 1, 2018

bastelfreak commented Jun 10, 2019

vox-pupuli-tasks bot commented Dec 4, 2019

vox-pupuli-tasks bot commented Dec 4, 2019

vox-pupuli-tasks bot commented Jan 5, 2020

vox-pupuli-tasks bot commented Jan 5, 2020

ekohl commented Aug 30, 2020

JacobHenner commented Mar 15, 2018 •

edited

Loading