(wip) Adds Bulk Feature Setting #107
Conversation
| foreach ($Feature in $FeaturesToSet.GetEnumerator()) { | ||
| $name = $Feature.Key | ||
| $state = $Feature.Value | ||
|
|
||
| $shouldBeEnabled = $state -eq "enabled" | ||
| $isEnabled = $featureStates.$name | ||
| if ($name -notin $featureStates.Keys) { | ||
| $message = "Invalid feature name '$name' specified, valid features are: $($featureStates.Keys -join ', ')" | ||
| Assert-TaskFailed -Message $message | ||
| } |
There was a problem hiding this comment.
initial offhand thought - would probably be better to collate a list of the features and values that were invalid so we can report all of them rather than just the first one. Nobody likes playing whack-a-mole with error messages :D
There was a problem hiding this comment.
I like that! I was worried about compatibility for a lazy things like -notin, but will figure something / show that it works in PS2.0 environments!
There was a problem hiding this comment.
From memory, -in and -notin are in PSv2, it's -contains and -notcontains that are missing. 🙂
Also, for this we're OK limiting support to PSv3 as that's Ansible's minimum requirement.
There was a problem hiding this comment.
First thought here is that collating the list is good here, as it's features and then are boolean. We're either turning something on or off, or you used a feature that's not in the list.
This is going to be trickier in config as you can supply junk data and chocolatey will happily apply it. Unless that's been fixed in 1.x and I wasn't made aware.
But doing something like choco config set SomeJunkKey SomeJunkValue will happily create that for ya.
There was a problem hiding this comment.
I feel like that's hard for us to validate in this scope - to an extent I feel that if Chocolatey doesn't, we shouldn't do so here.
There was a problem hiding this comment.
can supply junk data
Do you mean the below, or something else?
But doing something like choco config set SomeJunkKey SomeJunkValue will happily create that for ya.
That's a feature, not junk data. Chocolatey CLI will allow you to create your own config keys and values. So if you want to create SomeJunkKey with SomeJunkValue then Chocolatey CLI intentionally lets you do that. This isn't something we plan to remove.
JPRuskin
force-pushed
the
addsFeaturesDict
branch
from
9373ad6
to
af346dd
Compare
JPRuskin
force-pushed
the
addsFeaturesDict
branch
from
af346dd
to
ceab60d
Compare
Heavily inspired by win_environment, this adds the ability to set many features in one call.
JPRuskin
force-pushed
the
addsFeaturesDict
branch
from
ceab60d
to
7dfd031
Compare
| $state = $module.Params.state | ||
| $featuresToSet = if ($module.Params.features) { | ||
| $module.Params.features | ||
| } else { |
There was a problem hiding this comment.
We are using cuddled else's here. Nowhere else in the code (that I could see) is using that. We need to make sure we are consistent in our code.
|
Hey @JPRuskin, could I trouble you to open an issue to tag this PR to? I note this currently specifies #108, but I don't think that's accurate (#108 is about filtering the facts that the win_chocolatey_facts module will gather and return rather than doing them all every time, and this PR is about performing a bulk config operation)? |
Heavily inspired by win_environment / ps1, this adds the ability to set many features in one call.
Adding as a draft as I need to add test, add tests, and ask around to see if this would be actually useful? I think there would significant speed benefits if we could adopt this pattern across more of the module.
Description Of Changes
This adds a new parameter to win_chocolatey_feature,
features, which accepts a dict which (should) be feature names and state.This should allow folk to bring features away from drift, at once, without the penalties associated with using a multitude of separate calls.
Motivation and Context
It's quite slow to correct an entire config file.
Testing
Change Types Made
Related Issue
Somewhat inspired by 105.
Addresses #108
Change Checklist
This is not ready to merge. Needs to have a version-from inserted on the docs page, tests added, and to be checked for PowerShell compatibility at least.