tdevelioglu/refurbish #428

tdevelioglu · 2017-01-27T16:28:09Z

This is a combined PR for the following issues:

MODULES-4349 - Concat: Fix obsolete Rubocop config
MODULES-4350 - Concat: Fix path parameter not being a namevar.
MODULES-4351 - Concat: fix :order parameter accepting arbitrary values in concat_file
MODULES-4352 - Concat: Restrict parameter :replace to values accepted by file
MODULES-4359 - Concat: Fix native types so they can be used directly.

tdevelioglu · 2017-03-07T10:20:11Z

I know this is a rather large commit, would you like me to split it up into a number of smaller PR's instead ?

hunner

The use of tag instead of path/target was so that concat_fragment resources could define their fragment WITHOUT knowing what the target would be, say in the case of various profiles creating fragments to be picked up elsewhere based on the path chosen by concat_file, or even across exported resources when the exporting hosts do not know where the collecting host will put them.

The other changes are valuable though!

hunner · 2017-03-23T17:12:59Z

lib/puppet/type/concat_file.rb

@@ -58,68 +54,98 @@ def exists?
  end

  newparam(:order) do
-    desc "Controls the ordering of fragments. Can be set to alphabetical or numeric."
-    defaultto 'numeric'
+    desc "Controls the ordering of fragments. Can be set to :alpha or :numeric."


Docs should not have :'s for the argument values.

Fixed in #433

I see. The intent of my change was to allow the types to be used more easily directly but the tag normalization was making it tricky. By removing the tag and using the path or title as the target my hope was to get around it.

It is very valuable to be able to use the types without the wrappers however. My testing shows a ~10% reduction in total resource count, and ~20% decrease (2.5M -> 2M) in catalog size on one of our concat heavy roles. This adds up real quickly in real-world numbers when you consider an environment of 20-30k hosts which make heavily use of concat, especially on the inventory/puppetdb side of things, since all file content effectively gets duplicated when it gets passed through the wrappers.

Perhaps I can re-factor it to collect on both tag and target, title, how does that sound ?

~~I took another look at this and concat_fragment already has to know what the target will be, it can not be declared otherwise, see:~~

~~https://github.com/puppetlabs/puppetlabs-concat/blob/master/lib/puppet/type/concat_fragment.rb#L45-L49~~

Ah, I missed that it only emits a warning but it makes it seem as if the intent is that fragments should target concat_file by path or title.

It does raise the question what the actual purpose of the target parameter is then, because it's not used for anything except emitting that warning.

I think the target exists for backwards-compatibility reasons. We noticed that the order in the manifest defaults to alpha and that the order in the type defaults to numeric though, so I guess that it wasn't backwards-compatible with the .pp files anyway. Whoops. This is ticketed in MODULES-4600

hunner · 2017-03-23T17:15:10Z

lib/puppet/type/concat_file.rb

  end

-  newparam(:replace) do
+  newparam(:replace, :boolean => true, :parent => Puppet::Parameter::Boolean) do


I think if you make this :boolean => true then you don't need the parent?

Fixed in #433

:boolean => true just adds a param? method to the type, you still need the parent for value checking and coercion.

hunner · 2017-03-23T17:15:25Z

lib/puppet/type/concat_file.rb

  end

  newparam(:backup) do
    desc "Controls the filebucketing behavior of the final file and see File type reference for its use."
-    defaultto 'puppet'


Why the removal of this default? Is that backwards incompatible?

Oh, I see that the defaultto 'puppet' is set in lib/puppet/type/file.rb in puppet's source.

hunner · 2017-03-23T17:58:17Z

lib/puppet/type/concat_file.rb

@@ -85,40 +92,56 @@ def exists?
  end

  # Inherit File parameters
-  newparam(:selinux_ignore_defaults) do
-  end
+  newparam(:selinux_ignore_defaults, :boolean => true, :parent => Puppet::Parameter::Boolean)


No parent needed.

Well what do you know, apparently it does need a parent. Is this because it's a parameter, and only properties don't need a parent?

Technically no, they're not any different. It's just that setting Puppet::Property::Boolean as a parent of a property breaks the property which is why often you see the same bits copied directly into the property.

I removed the parent, then tests failed. It apparently accepted lots of truthy values because of :boolean => true, but didn't sanitize them on the way back. I didn't track down as to why yet though. It could have just been poorly defined behavior checking in the unit tests.

Oh how I wish we had actual manifest/catalog type & provider unit testing. Someday.

hunner · 2017-03-23T18:04:49Z

#433 brings in the first four commits (yay! Thank you!) with a few tweaks, so the review is just for the last commit about tag/path.

hunner · 2017-03-23T18:06:53Z

lib/puppet/type/concat_file.rb

  # End file parameters

  # Autorequire the file we are generating below
+  # Why is this necessary ?


¯_(ツ)_/¯ I don't recall. It seems like the generated file will be contained by the concat_file resource and wouldn't need this.

tdevelioglu · 2017-05-08T14:56:04Z

@hunner I've refactored the types to work with both tag and target only. Can you take a look and let me know what you think?

hunner · 2017-05-09T22:39:10Z

spec/acceptance/native/backup_spec.rb

@@ -0,0 +1,115 @@
+require 'spec_helper_acceptance'


It looks like all of the acceptance tests except warn_header_spec.rb were copied from spec/acceptance/ to spec/acceptance/native/ but the originals were not modified. Was this on purpose?

The examples in spec/acceptance/native/ test the native types directly. They're modified originals.

This commit makes it possible to use the native types directly without the :tag parameter supplied by the defined type wrappers. The concat_file resource uses a (re-declared) :tag parameter to find its desired fragments, however, because the tag parameter has special requirements on its value, the concat and concat::fragment defined types normalize the 'target' parameter before passing it to :tag. The :target parameter, is not used at all. This makes the native types clumsy to use directly (i.e. without the defined types). This commit lets the concat_file type also collect fragments by its :target parameter (which is not used at all anymore?) while it keeps backward compatibility with the tag design. - It makes concat_file collect fragments of which the :target parameter matches its resource title or :path parameter. - It implements the parameter validations from the defined types directly in concat_file and concat_fragment.

tdevelioglu/refurbish

tdevelioglu force-pushed the tdevelioglu/refurbish branch 4 times, most recently from 8c39172 to 2a297ed Compare January 31, 2017 11:26

tdevelioglu force-pushed the tdevelioglu/refurbish branch 2 times, most recently from f1d99b2 to a4ad15d Compare March 3, 2017 16:58

hunner mentioned this pull request Mar 23, 2017

Four commits #433

Merged

hunner suggested changes Mar 23, 2017

View reviewed changes

hunner reviewed Mar 23, 2017

View reviewed changes

tdevelioglu force-pushed the tdevelioglu/refurbish branch from a4ad15d to 3c21e64 Compare March 24, 2017 09:02

tdevelioglu force-pushed the tdevelioglu/refurbish branch 3 times, most recently from 88d5458 to 64921d9 Compare April 25, 2017 11:00

tdevelioglu mentioned this pull request Apr 28, 2017

Bump puppetlabs-concat, puppetlabs-stdlib and Puppet minimum versions voxpupuli/puppet-nginx#1081

Merged

tdevelioglu force-pushed the tdevelioglu/refurbish branch from 64921d9 to 146d456 Compare April 28, 2017 16:14

tdevelioglu force-pushed the tdevelioglu/refurbish branch 2 times, most recently from 71eb30c to a134033 Compare May 8, 2017 12:40

hunner suggested changes May 9, 2017

View reviewed changes

tdevelioglu force-pushed the tdevelioglu/refurbish branch from a134033 to 14db7f4 Compare June 19, 2017 08:52

hunner approved these changes Jun 27, 2017

View reviewed changes

hunner merged commit 14db7f4 into puppetlabs:master Jun 27, 2017

hunner added a commit that referenced this pull request Jun 27, 2017

Merge pull request #428 from tdevelioglu/tdevelioglu/refurbish

cde8196

tdevelioglu/refurbish

LukasAud added the feature label Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tdevelioglu/refurbish #428

tdevelioglu/refurbish #428

tdevelioglu commented Jan 27, 2017 •

edited

Loading

tdevelioglu commented Mar 7, 2017

hunner left a comment

hunner Mar 23, 2017

hunner Mar 23, 2017

tdevelioglu Mar 24, 2017

tdevelioglu Mar 24, 2017 •

edited

Loading

tdevelioglu Mar 24, 2017

hunner Mar 24, 2017 •

edited

Loading

hunner Mar 23, 2017

hunner Mar 23, 2017

tdevelioglu Mar 24, 2017

hunner Mar 23, 2017

hunner Mar 23, 2017

hunner Mar 23, 2017

hunner Mar 23, 2017

tdevelioglu Mar 24, 2017

hunner Mar 24, 2017

hunner commented Mar 23, 2017

hunner Mar 23, 2017

tdevelioglu commented May 8, 2017

hunner May 9, 2017

tdevelioglu May 10, 2017

tdevelioglu/refurbish #428

tdevelioglu/refurbish #428

Conversation

tdevelioglu commented Jan 27, 2017 • edited Loading

tdevelioglu commented Mar 7, 2017

hunner 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

tdevelioglu Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunner Mar 24, 2017 • edited Loading

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

hunner commented Mar 23, 2017

Choose a reason for hiding this comment

tdevelioglu commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdevelioglu commented Jan 27, 2017 •

edited

Loading

tdevelioglu Mar 24, 2017 •

edited

Loading

hunner Mar 24, 2017 •

edited

Loading