BREAKING: Drop puppet 3 support. Replace validate_* calls with datatypes in location.pp #1050

bastelfreak · 2017-03-10T01:14:28Z

No description provided.

vinzent · 2017-03-13T18:05:39Z

manifests/resource/location.pp

+  String $server                      = undef,
+  Optional[String] $www_root                    = undef,
+  Optional[String] $autoindex                   = undef,
+  Optional[Array ]$index_files                 = [


unnecessary whitespace

vinzent · 2017-03-13T18:05:58Z

manifests/resource/location.pp

@@ -153,66 +153,65 @@
 #  }

 define nginx::resource::location (
-  $ensure                      = present,
+  Enum['present', 'absent'] $ensure                      = present,


unnecessary whitespace

woah shit. somehow forogt to reformat. sorry!

vinzent · 2017-03-13T18:06:35Z

manifests/resource/location.pp

-  $location_custom_cfg         = undef,
-  $location_cfg_prepend        = undef,
-  $location_cfg_append         = undef,
+  Optional[String] $proxy                       = undef,


unnecessary whitespace (for all param lines))

vinzent · 2017-03-13T18:07:41Z

manifests/resource/location.pp

+  Optional[String] $auth_basic_user_file        = undef,
+  Optional[String] $auth_request                = undef,
+  Array $rewrite_rules               = [],
+  Integer $priority                    = 500,
  $mp4                         = false,
  $flv                         = false,


missing type

vinzent · 2017-03-13T18:08:36Z

manifests/resource/location.pp

@@ -153,66 +153,65 @@
 #  }

 define nginx::resource::location (
-  $ensure                      = present,
+  Enum['present', 'absent'] $ensure                      = present,
  $internal                    = false,


missing type

vinzent · 2017-03-13T18:08:48Z

manifests/resource/location.pp

+  Optional[Variant[String, Array]] $raw_append                  = undef,
+  Optional[Hash] $location_custom_cfg         = undef,
+  Optional[Hash] $location_cfg_prepend        = undef,
+  Optional[Hash] $location_cfg_append         = undef,
  $location_custom_cfg_prepend = undef,


missing type

vinzent · 2017-03-13T18:08:56Z

manifests/resource/location.pp

+  Optional[Variant[String, Array]] $raw_append                  = undef,
+  Optional[Hash] $location_custom_cfg         = undef,
+  Optional[Hash] $location_cfg_prepend        = undef,
+  Optional[Hash] $location_cfg_append         = undef,
  $location_custom_cfg_prepend = undef,
  $location_custom_cfg_append  = undef,


missing type

vinzent · 2017-03-13T18:09:03Z

manifests/resource/location.pp

+  Optional[String] $auth_basic_user_file        = undef,
+  Optional[String] $auth_request                = undef,
+  Array $rewrite_rules               = [],
+  Integer $priority                    = 500,
  $mp4                         = false,


missing type

vinzent · 2017-03-13T18:13:05Z

manifests/resource/location.pp

-  if !is_integer($priority) {
-    fail('$priority must be an integer.')
-  }
-  validate_array($rewrite_rules)
  if (($priority + 0) < 401) or (($priority + 0) > 599) {


the integer data types accepts min max params: https://docs.puppet.com/puppet/4.9/lang_data_number.html#the-integer-data-type

this conditional can be directly specified with the param type.

vinzent · 2017-03-13T18:59:25Z

manifests/resource/location.pp

+  String $server                                      = undef,
+  Optional[String] $www_root                          = undef,
+  Optional[String] $autoindex                         = undef,
+  Optional[Array ]$index_files                        = [


whitespace inside [] but not after.

vinzent · 2017-03-13T19:19:12Z

spec/defines/resource_server_spec.rb

@@ -904,18 +904,6 @@
        it { is_expected.to contain_nginx__resource__location("#{title}-default").with_ssl_only(true) }
      end

-      context 'SSL cert missing' do


what is legacy about the tests?

the datatypes enforce the correct param, so we don't need to test wrong/missing parameter. We dropped these kinds of tests on all other puppet4 only modules as well.

Dropping the tests is easier than fixing them, but what's testing that you've correctly chosen the right data types when refactoring?

that should be handled by the reviewer + the existing tests. They will fail when wrong datatypes are supplied.

if there are other tests. I don't have massively strong feelings though. It was more devil's advocate. They probably are more trouble than they're worth.

yes. In general we need more tests. but this is more of a global issue on all of our modules.

vinzent · 2017-03-13T19:27:51Z

did you run the beaker acceptance tests? are the ok?

bastelfreak · 2017-03-23T00:16:57Z

mhm beaker... https://gist.github.com/bastelfreak/947477bff061607dc797119189a008ac
it is somehow broken. I will take a look at this in the next days, but maybe needs some time. if anybody else wants to test this... feel free :)

igalic

I'm very happy with these changes
and… i have no idea what's up with beaker ☹

igalic · 2017-03-23T10:18:17Z

manifests/resource/location.pp

+  Optional[String] $auth_basic                        = undef,
+  Optional[String] $auth_basic_user_file              = undef,
+  Optional[String] $auth_request                      = undef,
+  Array $rewrite_rules                                = [],


how come this isn't Optional?

it was already an existing array. I didn't want to change the datatype to undef and make it optional.

alexjfisher · 2017-03-23T10:50:46Z

manifests/resource/location.pp

+  Enum['present', 'absent'] $ensure                   = present,
+  Boolean $internal                                   = false,
+  String $location                                    = $name,
+  String $server                                      = undef,


Does defaulting to undef make sense if you're not using Optional[String] here?

yes, that makes it a required parameter. It currently already is a required param, just not handled nicely:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/location.pp#L385-L387
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/location.pp#L230-L232

It would have been more clear if it didn't default to undef but to nothing.

Yeah, old code does look a bit funky. Do you want to remove the default then since now it is much more obvious it provides no value.

I'm not sure what the best practice is. in the other PRs we tried to always default to undef on required params without default value

dominics · 2017-08-03T05:45:13Z

manifests/resource/location.pp

+  Optional[Hash] $location_cfg_prepend                = undef,
+  Optional[Hash] $location_cfg_append                 = undef,
+  Optional[Hash] $location_custom_cfg_prepend         = undef,
+  Optional[Hash] $location_custom_cfg_append          = undef,


Should probably have been Optional[Variant[Hash, Array]] to match existing behaviour (#1100)

Possibly, but maybe not. See #1100 (comment)

replace validate_* calls with datatypes in location.pp

bastelfreak added 3 commits March 10, 2017 02:13

replace validate_* calls with datatypes in location.pp

b071da8

fix wrong datatype in server.pp

307a7b4

drop legacy tests

40a70fc

bastelfreak changed the title ~~replace validate_* calls with datatypes in~~ replace validate_* calls with datatypes in location.pp Mar 10, 2017

bastelfreak self-assigned this Mar 10, 2017

bastelfreak added backwards-incompatible enhancement New feature or request labels Mar 10, 2017

vinzent reviewed Mar 13, 2017

View reviewed changes

bastelfreak added 2 commits March 13, 2017 19:40

reformat location.pp params

430c31b

improve $priority data types with range for integer

5e2801f

vinzent reviewed Mar 13, 2017

View reviewed changes

add datatypes for all params in location.pp

0e56fa0

bastelfreak force-pushed the puppet4 branch from 9e8e0d7 to 0e56fa0 Compare March 13, 2017 23:30

yastupin approved these changes Mar 23, 2017

View reviewed changes

igalic reviewed Mar 23, 2017

View reviewed changes

alexjfisher reviewed Mar 23, 2017

View reviewed changes

bastelfreak merged commit 66b3523 into voxpupuli:master Mar 23, 2017

bastelfreak deleted the puppet4 branch March 23, 2017 20:19

alexjfisher changed the title ~~replace validate_* calls with datatypes in location.pp~~ BREAKING: Drop puppet 3 support. Replace validate_* calls with datatypes in location.pp Aug 1, 2017

dominics mentioned this pull request Aug 3, 2017

Regression in 0.7.0: incorrect data type on location_custom_cfg_prepend and similar parameters #1100

Open

dominics reviewed Aug 3, 2017

View reviewed changes

cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019

Merge pull request voxpupuli#1050 from bastelfreak/puppet4

66d3ee1

replace validate_* calls with datatypes in location.pp

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020

Merge pull request voxpupuli#1050 from bastelfreak/puppet4

5a7d8ac

replace validate_* calls with datatypes in location.pp

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING: Drop puppet 3 support. Replace validate_* calls with datatypes in location.pp #1050

BREAKING: Drop puppet 3 support. Replace validate_* calls with datatypes in location.pp #1050

bastelfreak commented Mar 10, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

bastelfreak Mar 13, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

vinzent Mar 13, 2017

bastelfreak Mar 23, 2017

alexjfisher Mar 23, 2017

bastelfreak Mar 23, 2017

alexjfisher Mar 23, 2017

bastelfreak Mar 23, 2017

vinzent commented Mar 13, 2017

bastelfreak commented Mar 23, 2017

igalic left a comment

igalic Mar 23, 2017

bastelfreak Mar 23, 2017

alexjfisher Mar 23, 2017

bastelfreak Mar 23, 2017

alexjfisher Mar 23, 2017

bastelfreak Mar 23, 2017

dominics Aug 3, 2017

alexjfisher Aug 3, 2017

BREAKING: Drop puppet 3 support. Replace validate_* calls with datatypes in location.pp #1050

BREAKING: Drop puppet 3 support. Replace validate_* calls with datatypes in location.pp #1050

Conversation

bastelfreak commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinzent commented Mar 13, 2017

bastelfreak commented Mar 23, 2017

igalic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment