[GH-476] Allow specifying install method for windows_feature resources

[dave-q]

• Introduces new attribute ['iis']['windows_feature_install_method'] if the node is present its value is passed into any windows_feature resources as the install_method property

Signed-off-by: dave-q [email protected]

Description

Exposes the ability to dictate what install method is used by any windows_feature resources.

Issues Resolved

476

Check List

• [x] All tests pass. See TESTING.md for details.
• [x] New functionality includes testing.
• [x] New functionality has been documented in the README if applicable.

[xorima]

@dave-q please can you also add an entry in the changelog file under the ## Unreleased header

[dave-q]

@Xorima So after a bit of fiddling I've got the test's passing. The code in the PR is all working but in my opinion I need to give it one more pass over and tidy it up. But I was hoping to get your opinion on a couple of things.

1. I've added this helper function to translate 'dism-format' feature names into 'powershell-format', if the install method is powershell - While this works pretty well and saves having to have a conditional in each place that wants a feature managed (to decide which name to use). It also makes the api more flexible, it can accept the additional features node containing either format. However one could argue that its extra complexity and in a way the fact that the 'api' is so 'forgiving' could infact be a little confusing.. I'd be interested to hear your's (or anyone elses) opinion on this. The other option is that all the recipes that install windows features decide which format to use in the recipe and then use that with windows_feature
2. If we think the that 'translating' the feature name depending on the install method is good then that leaves me with one more questions. Currently I've introduce a helper method in a library AND a wrapper resource. I think it should be one or the other. Either a library that is called in every recipe, that then goes on to called windows_feature. Or we continue to have the wrapper resource and the resource can just define the functionality for 'feature name translation'. Again I'm not sure whats better. I'd lean towards the wrapper resource.. but that maybe due to my background of OOP...

Let me know your thoughts and I can update the PR

[xorima]

@rlaveycal would you be able to take a look over these changes as a heavy user of the iis cookbook?

[rlaveycal]

I'd lean more to the library-in-resource and use window_feature approach because it avoids having to keep the wrapper resource in API sync. It also avoids the temptation for people to use this resource themselves.

[bmhughes]

Definetly go with the library method route imo.

The conversion helper method can be refactored to make it more idiomatic ruby wise:

1. Use map instead of each which will shorten it and remove several intermediate variables
2. Instead of using splat to pass the feature names in it would be better served by passing an Array to a single parameter. Splat is idomatically used to make a variable number of different parameters into a method, as this takes a single parameter (that may be of variable length) it doesn't really make sense to use splat. Wrap it in Array() to cover the cases where just a string is passed in.

[dave-q]

@bmhughes @rlaveycal

Thanks for taking the time to review my PR. I've updated it with your suggestions and all the tests are passing.

I have a couple of questions.

1. I wasn't sure about Monkey patching the ::Chef::DSL::Recipe class with my helper, but I was told and from doing some reading too, this is the correct way to do this, anyway.. I'm happy if you guys are but if you have a suggestion for a different way, I'm all ears
2. My initial attempt was incorrect (i just called include IISCookbook::WindowsFeatureHelper directly in the recipe) however I didn't notice that until the site inspec test ran... I would have caught it much sooner, if there was just a basic 'it converges' spec test for all recipes. I'm happy to add them if you think its worth it.

Thanks

[bmhughes]

@bmhughes @rlaveycal

Thanks for taking the time to review my PR. I've updated it with your suggestions and all the tests are passing.

I have a couple of questions.

1. I wasn't sure about Monkey patching the ::Chef::DSL::Recipe class with my helper, but I was told and from doing some reading too, this is the correct way to do this, anyway.. I'm happy if you guys are but if you have a suggestion for a different way, I'm all ears

2. My initial attempt was incorrect (i just called include IISCookbook::WindowsFeatureHelper directly in the recipe) however I didn't notice that until the site inspec test ran... I would have caught it much sooner, if there was just a basic 'it converges' spec test for all recipes. I'm happy to add them if you think its worth it.

Thanks

You've got a couple of options here depending on where you need the helper method to be available. From a quick scan through I can't see you needing it outside of the custom resource so I wouldn't mix it into the recipe DSL class for this, just include it in the outer resource definition (which I think you already have done) and drop the include to the recipe class.

Something to note is that with how it is now, the method will only be mixed into the outer resource definition class (properties, load_current etc) and won't be in the action_class(es), if you needed the module methods within the action class(es) as well then that's where the action_class block comes in handy as each action class will yield that on compiliation and include the module within all the actions of the resource so you don't have to repeat yourself.

[dave-q]

@lamont-granquist @bmhughes I appreciate the feedback. You've both made a couple of comments that seemed to be based around the thought that the only place we are using the new attribute (windows_feature_install_method) is in the install.rb resource. It is used in almost every recipe (any starting with mod_) since all those recipes use the windows_feature resource, so we need to pass that into those resources.

We also kind of need to use that Module helper method I added, in all those mod_* recipes since we need to translate the feature name. We could do a switch on the install mode and 'hardcode' the powershell version of feature name in there too

something like

feature_name = ''
if install_method == :windows_powershell do
  feature_name = <powershell format>
else
  feature_name = <dism format>
end

personally I think thats a little messy, but I can see an argument for doing it

anyway, I'm going to hold off making any changes right now just until this is a bit clearer

[bmhughes]

I know the sticks in the mud hate it but is there any reason those recipes can't go away and be replaced by a resource? A lot of them just seem to be doing the same thing from a quick skim through.

[dave-q]

I know the sticks in the mud hate it but is there any reason those recipes can't go away and be replaced by a resource? A lot of them just seem to be doing the same thing from a quick skim through.

I've got no strong opinion one way or the other really, but just so I'm clear what you are suggesting.

The recipes wouldn't exist at all? And people should call them as resources?? That would definitely be a big breaking change wouldn't it? Since anyway who utilised those recipes in a downstream cookbook would get their cookbook broken right?

If the recipes still existed..they would just be calling in to a resource.. much like 90% of them do now anyway. It would just be a resource from this cookbook rather than a core resource. I did take a stab at that in one of my earlier iterations, essentially a windows_feature wrapper that did all of the stuff with install modes and symbols and feature names. But we opted to move away from that as it would be exposed as a resource for others to use and also we'd have to keep the API in sync. I'm happy to put that back on the table if you think that's better.

[bmhughes]

Yes that's what I meant (I'd wait for someone else to chime in incase I'm missing something obvious though), apologies for the garden path route to it.

The massive breaking change is why I made the sticks in the mud comment, in general we're (sous-chefs) trying to move to resource-only cookbooks where the final implementation is created by the user in a wrapper cookbook that wraps the resources to what they need rather than trying to cover 90% of use cases with a generic recipe. Some people are aginst this because they have to create org specific wrapper cookbooks, but with the right style and some forethough they're very quick to create and have very little substance (the guts of the logic are implemented by the resources) so aren't the end of the world. It's the same paradigm as terraform/ansible when you add a provider or module.

I wouldn't create (or think of it as) a wrapper for the windows_feature though, I (personally) would call it module_install or something along those lines. The fact that is will mostly be a wrapper around windows_feature is irrelevant as the desired outcome of the resource is to install an IIS module, it just happens to use windows_feature to achieve that in the same way as a lot of the other resources in sous cookbooks use package, template etc. The core resources don't undergo massive API changes in general without giving you plenty of notice and warning, so I don't see that as an issue.

That's my 2p anyway. I reserve the right to be completely wrong 😂.

edit: It'd be a major version bump given that change, but that's fine I've ripped up plenty of cookbooks already.

[dave-q]

I can definitely see the benefits of that model for sure. I'm to familiar with the use cases of chef. I know at my work we have our own internal supermarket so creating a wrapper would be simple enough. But what about companies that don't have that infrastructure and just want to use public recipes directly with some other orchestrating framework? I suppose I'm just speculating now really.

Anyway... For this PR, would a good compromise be to introduce a new resource 'module_install' (much better name) that wraps all that logic nicely and then for now update the recipes to just call that resource so they essentially stay the same but just call the new resource.

Then in the future it would be simple to remove the recipes in a larger breaking change release?

[bmhughes]

I'm open to the general concensus on that, personally I'd just rip it all out including the attributes and move everything to library/resources. But it certainly is an alternative option and does deal with removing the need to mix that library (or for it to exist at all as @lamont-granquist points out) into the recipe DSL unnecessarily.

[dave-q]

So I've taken a lot of what you both suggested on board @bmhughes @lamont-granquist

I noticed that with a little refactoring, the install resource could do all the new work translating feature names and optionally do the starting of the W3SVC service if desired.

This makes the recipes really really basic now, they all just call into the same resource (some call other internal resources too)

If we want to take the route @bmhughes said of removing the recipes, this would be very easy in the future.

I added some new spec tests for the install resource since it had a little more 'behavior' in it.

I also added a spec test for all the mod_* recipes. I think its a good habit and gives some quick confidence that things aren't broken.

Look forward to hearing your thoughts

Thanks

[damacus]

@Xorima and @bmhughes are you both happy with the way this current PR is?

[kitchen-porter]

Released as: 8.1.0