(MODULES-5842) Implement Generic DSC Invoker #12

jpogran · 2017-12-18T18:49:38Z

This commit adds the dsc type and provider, which allows executing
DSC Resources without the build system used by the previous pupppet dsc
module.

This is accomplished by adding a new type that accepts a DSC Resource
Name and Module Name, and a un-typed hash of properties. This is passed
as is to Invoke-DscResource.

Unit tests for the type are added, manual verification of these changes
are to be done as automated tests are to be added in subsequent tickets

Iristyle · 2017-12-18T21:48:08Z

lib/puppet_x/puppetlabs/dsc_lite/powershell_hash_formatter.rb

+
+        end
+
+        def self.format_string(value)


I think these format_ helpers can all be private as there are no other callers and all testing is against .format

I had more tests coming in MODULES-6323, or at least I expect whoever implements that ticket will be adding more tests here as we'll be formatting CimInstances

Yeah, I would mark these as private for now... if you need to add a more complex method for CimInstance formatting later and make that public for the sake of testing it individually, you can do that then.

Iristyle · 2017-12-18T21:48:57Z

lib/puppet_x/puppetlabs/dsc_lite/powershell_hash_formatter.rb

+      end
+    end
+  end
+end


nit - missing newline for some reason

newline added

Iristyle · 2017-12-18T21:51:03Z

lib/puppet/provider/base_dsc_lite/powershell.rb

    end
  end
+
+  def self.format_dsc_lite(dsc_value)


Is the code here the same as format_dsc_value above?

It's the same now, however I expect it to diverge, and since you want the old way sitting next to the new way, I can't change the format_dsc method

Iristyle · 2017-12-18T21:51:57Z

spec/integration/puppet_x/puppetlabs/powershell_hash_formatter_spec.rb

+    @formatter = PuppetX::PuppetLabs::DscLite::PowerShellHashFormatter
+  end
+
+  describe "formating ruby hash to powershell hash string" do


nit formatting

Iristyle · 2017-12-18T22:00:14Z

lib/puppet/type/dsc.rb

+    end
+  end
+
+  newparam(:dsc_resource_properties, :array_matching => :all) do


Hmm - not sure if :array_matching => :all does anything for us here?

Iristyle · 2017-12-18T22:07:34Z

spec/unit/puppet/type/dsc_spec.rb

+      }.to raise_error(Puppet::ResourceError, /A non-empty name must/)
+    end
+
+    [ 'value', 'value with spaces', 'UPPER CASE', '0123456789_-', 'With.Period' ].each do |value|


Aren't DSC resource naming guidelines stricter than this? I wouldn't expect <space> to be allowed? And I wouldn't expect values starting with numerics?

I'm having a tough time tracking down any specs / docs for the guidelines though - maybe buried somewhere in the CIM spec at https://www.dmtf.org/standards/cim/schemas?

I would think trying to truly validate naming would be a game of catchup we aren't interested in pursuing as we are trying to be loosely typed here, and not get back into the business of validating parts we don't own.

Looking at your code again, thought you had regexes in place for all of these, but I see there's only one for :name.

This is fine, though I don't expect PowerShell class names to be a moving target given they're well defined in the grammar specification. Changing that would be backward-incompatible, so I think the rules are pretty set in stone at this point.

Iristyle · 2017-12-18T22:17:06Z

spec/unit/puppet/type/dsc_spec.rb

+      }.to raise_error(Puppet::ResourceError, /A non-empty name must/)
+    end
+
+    [ 'value', 'value with spaces', 'UPPER CASE', '0123456789_-', 'With.Period' ].each do |value|


Same here for the spaces... PowerShell 3 language spec says this about simple-name:

Probably TMI, but ...

Lu is letter, uppercase - http://www.fileformat.info/info/unicode/category/Lu/index.htm

Ll is letter, lowercase - http://www.fileformat.info/info/unicode/category/Ll/index.htm

Lt is letter, titlecase - http://www.fileformat.info/info/unicode/category/Lt/index.htm

Lm is letter, modifier - http://www.fileformat.info/info/unicode/category/Lm/index.htm

Lo is letter, other - http://www.fileformat.info/info/unicode/category/Lo/index.htm

The important part is cannot start with Nd - number, decimal digit - http://www.fileformat.info/info/unicode/category/Nd/index.htm

Even if we don't check whether it starts with a number, I think we can disallow the whitespace.

Same as previous comment

Iristyle · 2017-12-18T22:27:34Z

spec/unit/puppet/type/dsc_spec.rb

+      }.to raise_error(Puppet::Error, /dsc_resource_properties should be a Hash/)
+    end
+  end
+end


nit - missing newline

newline added

Iristyle · 2017-12-18T22:31:54Z

spec/unit/puppet/type/dsc_spec.rb

+
+    it "requires a hash or array of hashes" do
+      expect {
+        resource[:dsc_resource_properties] = "hi"


Still working this out?

This was part of the exploration for supporting CimInstances, but not fully implemented past being a hash. It was left like this since we ticketed that work in MODULES-6323

If it's a hash shouldn't it be?

resource[:dsc_resource_properties] = { "hi" => "james" }

And shouldn't it reject a bare string like "hi"?

Apologies - I don't know what my last comment was about here - you are rejecting a bare string 🤦‍♂️

Iristyle · 2017-12-18T22:32:11Z

spec/unit/puppet/type/dsc_spec.rb

+        resource[:dsc_resource_properties] = "hi"
+      }.to raise_error(Puppet::Error, /dsc_resource_properties should be a Hash/)
+      expect {
+        resource[:dsc_resource_properties] = ["hi"]


Hmm - what does an array of hashes mean in this context? Multiple instances of the same resource? That may be complex to implement if that's the intent?

This was part of the exploration for supporting CimInstances, but not fully implemented past being a hash. It was left like this since we ticketed that work in MODULES-6323

Probably worth punting to next ticket / PR then

This commit adds the `dsc` type and provider, which allows executing DSC Resources without the build system used by the previous pupppet dsc module. This is accomplished by adding a new type that accepts a DSC Resource Name and Module Name, and a un-typed hash of properties. This is passed as is to `Invoke-DscResource`. Unit tests for the type are added, manual verification of these changes are to be done as automated tests are to be added in subsequent tickets

jpogran force-pushed the MODULES-5842-generic-dsc branch from b8929e0 to db16c4e Compare December 18, 2017 18:54

Iristyle reviewed Dec 18, 2017

View reviewed changes

This was referenced Dec 18, 2017

(MODULES-5842) WIP dsc type and invoker #9

Closed

(WIP) (MODULES-5842) Generic DSC invoker #2

Closed

jpogran force-pushed the MODULES-5842-generic-dsc branch 2 times, most recently from e3e3830 to 690bfbe Compare December 19, 2017 14:22

jpogran force-pushed the MODULES-5842-generic-dsc branch from 690bfbe to 164eab8 Compare January 3, 2018 18:03

Iristyle closed this in 2a2ba13 Jan 4, 2018

+                    end
+                  end
+                end
+              end

                
                    No newline at end of file

(MODULES-5842) Implement Generic DSC Invoker #12

(MODULES-5842) Implement Generic DSC Invoker #12

Uh oh!

Conversation

jpogran commented Dec 18, 2017

Uh oh!

Iristyle Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iristyle Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iristyle Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iristyle Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Iristyle Dec 18, 2017 •

edited

Loading

Iristyle Dec 19, 2017 •

edited

Loading

Iristyle Dec 18, 2017 •

edited

Loading

Iristyle Dec 19, 2017 •

edited

Loading