[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 24652224+dave-q@users.noreply.github.com

Description

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

Issues Resolved

476

Check List

• All tests pass. See TESTING.md for details.
• New functionality includes testing.
• 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

[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

[xorima]

Why are we controlling the service in the install resource now?

[dave-q]

Every recipe in the cookbook was doing the same things really.

Include the default recipe, add another windows feature and then sometimes one other resource.

The default recipe would install iis and then start it.

Moving the starting into the resource (bare in mind it's optional and doesn't by default to keep past behaviour the same for users of the resource) meant that

1. Every recipe could now call the install resource and pass its own additional features along with, removing the need to use windows_feature in almost every recipe.
2. Now there was only one place windows_feature is being used. The install resource, this allows us to keep the 'translation of feature names' just contained to that resource.
3. All the recipes are dead simple now and if at some point in the future sous-chefs want to remove them it will be much easier for people to transition and keep the same behaviour

[bmhughes]

It's been a while since i've had to use IIS but i'm pretty sure the w3svc service is enabled by default when IIS is installed? So I don't think this is necessary. Also, it's adding unexpected functionality to the resource, I don't expect an install resource to do anything other than install.

If people need to manage the IIS service then it's not a big deal to do that in the implementing wrapper as there's no abstraction required outside of the core Chef resources.

[dave-q]

It doesn't start it by default you have to set the property.
The reason I added it was it allows the rest of the recipes to be much simpler, since they all called the default recipe anyway and the default recipe started the service and I wanted to ensure I wasn't changed anything for existing people who don't want to have to update how they use this cookbook. It means that all recipes rely on the install resource to install their features and we can keep that feature name translation in one place rather than having to spread it around the cookbook.

For outside users the install resource really hasn't changed if they don't want. By default it won't try and start the service (which according yourself is probably done anyway), personally I think this gives us the benefit of new functionality with out breaking any existing features/APIs, whilst also laying the ground work for the future desire of just exposing resources rather than these recipes. The recipes are now very basic so it's easy for people to 're-implement' them in any future breaking change

[damacus]

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

[kitchen-porter]

Released as: 8.1.0