BREAKING: Drop puppet 3 support. Replace validate_* functions with Puppet 4 data type validations #264

jkroepke · 2017-02-19T20:47:04Z

Added Puppet Data types for every parameter.

remove deprecated validate_* and is_* functions from stdlib.

jkroepke

There are some lint check results which does not make sense.

jkroepke · 2017-02-19T20:50:38Z

manifests/artifactory.pp

@@ -67,7 +67,9 @@
    $file_path = $path
  }

-  validate_absolute_path($file_path)
+  if $file_path !~ Stdlib::Compat::Absolute_path {
+    fail("archive::artifactory[${name}]: \$name or \$archive_path must be an absolute path!")


manifests/artifactory.pp:71:trailing_comma:WARNING:missing trailing comma after last element

jkroepke · 2017-02-19T20:51:10Z

manifests/go.pp

@@ -26,7 +26,9 @@
    $file_path = $path
  }

-  validate_absolute_path($file_path)
+  if $file_path !~ Stdlib::Compat::Absolute_path {
+    fail("archive::go[${name}]: \$name or \$archive_path must be an absolute path!")


manifests/go.pp:30:trailing_comma:WARNING:missing trailing comma after last element

alexjfisher · 2017-02-19T21:00:01Z

When using Optional, do parameters still need to default to undef?

jkroepke · 2017-02-19T21:04:20Z

Optional[X] is just an alias to Variant[Undef, X].

See: https://docs.puppet.com/puppet/4.9/lang_data_abstract.html#optional

So it's still request to set a default value unless you want to have required parameters.

We can discuss about non undef values for example cleanup should be true; https://github.com/voxpupuli/puppet-archive/blob/master/lib/puppet/type/archive.rb#L121 but it should be a own issue/pr.

alexjfisher · 2017-02-19T21:10:12Z

@jkroepke Yeah, ok. I did a bit more digging. The reason this, (for example), works is that the ntp module also uses 'data in modules' and defaults the parameter to undef by using ~ in the yaml here.

jkroepke · 2017-02-19T21:13:27Z

@alexjfisher correct. But the ntp module used the already deprecated hiera v4 as demo.

Data in modules is imho a task for the puppet 5 age.

alexjfisher · 2017-02-20T20:48:32Z

@jkroepke Good work opening the puppet-lint issue. Meanwhile, (and so we can get this merged), how about using control-comments to disable the check? You can also include why you've disabled the check.

For the sake of your memory (and your coworkers), any text in your comment after lint:ignore: will be considered the reason for ignoring the check and will be displayed when showing ignored problems.

jkroepke · 2017-02-20T21:01:09Z

@alexjfisher Done

jkroepke · 2017-02-20T21:11:50Z

@alexjfisher I have no idea, how to use modern facts on the line like here:

Facter.stubs(:value).with(:osfamily).returns 'RedHat'

How to modify it to use facts[os][family] ?

jkroepke · 2017-02-20T21:27:47Z

@alexjfisher Does it make sense to check failures like

 1) archive::nexus nexus archive with allow_insecure => 'foobar' should fail to compile and raise an error matching /"foobar" is not a boolean/
     Failure/Error: it { is_expected.to compile.and_raise_error(%r{"foobar" is not a boolean}) }
       error during compilation: Evaluation Error: Error while evaluating a Resource Statement, Archive::Nexus[/tmp/artifact.war]: parameter 'allow_insecure' expects a Boolean value, got String at line 1 on node testing-docker-98956c12-9e09-4fc4-86af-1202defc7162.ec2.internal
     # ./spec/defines/nexus_spec.rb:150:in `block (3 levels) in <top (required)>'

If yes, I haven't the knowlage to adjust this test. Sorry.

alexjfisher · 2017-02-20T23:07:05Z

@jkroepke That test does need to be fixed, but it should be pretty simple. "foobar" is not a boolean is the puppet error that the validate_bool function would have spat out. As such I think all you need to do is update the regular expression to match the new compilation error output. ie try replacing this line with...

it { is_expected.to compile.and_raise_error(%r{parameter 'allow_insecure' expects a Boolean value, got String}) }

alexjfisher · 2017-02-20T23:14:13Z

@jkroepke That looks better. I don't understand why one of the combinations still failed. Very odd... Maybe something to do with the recently introduced parallel rspec??

alexjfisher · 2017-02-20T23:17:18Z

and thanks for the rebase/squash!

alexjfisher · 2017-02-20T23:20:29Z

@jkroepke Other than squashing, did you make any other changes? Tests are all green this time.

jkroepke · 2017-02-20T23:20:35Z

¯\_(ツ)_/¯

Now its fine.

jkroepke · 2017-02-20T23:22:46Z

Other than squashing, did you make any other changes? Tests are all green this time.

No, just a squash.

alexjfisher

LGTM. I've asked on IRC if anyone else would like to review too. IMO, changes to puppet-archive probably deserve a bit more scrutiny than most other modules.

igalic

This looks very good!
I would however consider reusing (and enriching) our Tea module

igalic · 2017-02-20T23:29:32Z

manifests/artifactory.pp

+  Optional[String]                        $extract_path = undef,
+  Optional[String]                        $creates      = undef,
+  Optional[Boolean]                       $cleanup      = undef,
+  Optional[Stdlib::Compat::Absolute_path] $archive_path = undef,


how about we use Tea::Absolutepath here?

hum, why you want to add additional dependencies? The stdlib data types based on the tea types. Not?

The ::compat:: types are specifically designed to match the horror that was the validate_ function that went before. There is Stdlib::Absolutepath to do better path checking.

so we should then, officially, deprecate Tea in favour for Stdlib… not that it was ever in widespread use to begin with…

…

i hope

Improve standards instead build a own world. That why i'm doing this PR. :)

alexjfisher · 2017-02-20T23:33:57Z

I've reworded the PR title as it'll probably get used in the changelog when that's next updated.

sacres

Thanks for the hard work on this! I think we should specify puppetversion >= 4.6.1 rather than 4.4.0.

sacres

Thanks!

jkroepke commented Feb 19, 2017

View reviewed changes

jkroepke mentioned this pull request Feb 19, 2017

False positive: trailing comma for functions in if clause voxpupuli/puppet-lint-trailing_comma-check#4

Closed

Use Puppet 4 validations

29f9ca7

jkroepke force-pushed the puppet4 branch from 2bfafe5 to 29f9ca7 Compare February 20, 2017 23:15

alexjfisher approved these changes Feb 20, 2017

View reviewed changes

alexjfisher changed the title ~~Puppet 4 validations~~ Replace validate_* functions with Puppet 4 data type validations Feb 20, 2017

igalic reviewed Feb 20, 2017

View reviewed changes

sacres requested changes Feb 20, 2017

View reviewed changes

Bump puppet and stdlib minimum requirements

27fe709

sacres approved these changes Feb 20, 2017

View reviewed changes

bastelfreak merged commit dba7f0f into voxpupuli:master Feb 21, 2017

jkroepke deleted the puppet4 branch February 21, 2017 00:17

alexjfisher changed the title ~~Replace validate_* functions with Puppet 4 data type validations~~ BREAKING: Drop puppet 3 support. Replace validate_* functions with Puppet 4 data type validations Aug 24, 2017

alexjfisher added backwards-incompatible enhancement New feature or request labels Aug 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING: Drop puppet 3 support. Replace validate_* functions with Puppet 4 data type validations #264

BREAKING: Drop puppet 3 support. Replace validate_* functions with Puppet 4 data type validations #264

jkroepke commented Feb 19, 2017 •

edited

Loading

jkroepke left a comment

jkroepke Feb 19, 2017

jkroepke Feb 19, 2017

alexjfisher commented Feb 19, 2017

jkroepke commented Feb 19, 2017

alexjfisher commented Feb 19, 2017

jkroepke commented Feb 19, 2017

alexjfisher commented Feb 20, 2017

jkroepke commented Feb 20, 2017

jkroepke commented Feb 20, 2017

jkroepke commented Feb 20, 2017

alexjfisher commented Feb 20, 2017

alexjfisher commented Feb 20, 2017

alexjfisher commented Feb 20, 2017

alexjfisher commented Feb 20, 2017

jkroepke commented Feb 20, 2017 •

edited

Loading

jkroepke commented Feb 20, 2017

alexjfisher left a comment

igalic left a comment

igalic Feb 20, 2017

jkroepke Feb 20, 2017

DavidS Feb 21, 2017

igalic Feb 21, 2017

jkroepke Feb 21, 2017

alexjfisher commented Feb 20, 2017

sacres left a comment •

edited

Loading

sacres left a comment

BREAKING: Drop puppet 3 support. Replace validate_* functions with Puppet 4 data type validations #264

BREAKING: Drop puppet 3 support. Replace validate_* functions with Puppet 4 data type validations #264

Conversation

jkroepke commented Feb 19, 2017 • edited Loading

jkroepke left a comment

Choose a reason for hiding this comment

jkroepke Feb 19, 2017

Choose a reason for hiding this comment

jkroepke Feb 19, 2017

Choose a reason for hiding this comment

alexjfisher commented Feb 19, 2017

jkroepke commented Feb 19, 2017

alexjfisher commented Feb 19, 2017

jkroepke commented Feb 19, 2017

alexjfisher commented Feb 20, 2017

jkroepke commented Feb 20, 2017

jkroepke commented Feb 20, 2017

jkroepke commented Feb 20, 2017

alexjfisher commented Feb 20, 2017

alexjfisher commented Feb 20, 2017

alexjfisher commented Feb 20, 2017

alexjfisher commented Feb 20, 2017

jkroepke commented Feb 20, 2017 • edited Loading

jkroepke commented Feb 20, 2017

alexjfisher left a comment

Choose a reason for hiding this comment

igalic left a comment

Choose a reason for hiding this comment

igalic Feb 20, 2017

Choose a reason for hiding this comment

jkroepke Feb 20, 2017

Choose a reason for hiding this comment

DavidS Feb 21, 2017

Choose a reason for hiding this comment

igalic Feb 21, 2017

Choose a reason for hiding this comment

jkroepke Feb 21, 2017

Choose a reason for hiding this comment

alexjfisher commented Feb 20, 2017

sacres left a comment • edited Loading

Choose a reason for hiding this comment

sacres left a comment

Choose a reason for hiding this comment

jkroepke commented Feb 19, 2017 •

edited

Loading

jkroepke commented Feb 20, 2017 •

edited

Loading

sacres left a comment •

edited

Loading