-
Notifications
You must be signed in to change notification settings - Fork 63
feat: Add --what-if for Microsoft.Windows/FirewallRuleList
#1530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Gijsreyn
wants to merge
3
commits into
PowerShell:main
Choose a base branch
from
Gijsreyn:gh-1529/main/add-whatif-firewall
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -262,3 +262,302 @@ someError = "Failed to do something: %{error}" | |
| #### Build and Deployment | ||
|
|
||
| - The resource should be built using `build.ps1 -project <resource_name>` from the root of the repository, which will handle building the Rust code and ensure it is found in PATH for testing | ||
|
|
||
| ## What-If support | ||
|
|
||
| Follow this pattern exactly when adding what-if (a.k.a. `dsc config set --what-if`) support to any resource so the implementation path, manifest changes, tests, and naming stay consistent across the repository. | ||
|
|
||
| What-if must: | ||
|
|
||
| 1. Project the **final state** the resource would produce, without mutating the system. | ||
| 2. Echo back the relevant input fields (`keyPath`, `valueName`, `valueData`, etc.) so the engine can diff before/after. | ||
| 3. Attach human-readable "would do …" messages under `_metadata.whatIf` (an array of strings). | ||
| 4. Exit `0` on success — what-if is not an error path. | ||
|
|
||
| ### 1. Resource manifest changes | ||
|
|
||
| In the resource's `*.dsc.resource.json`, add `whatIfArg` to the `set` (and `delete`, if it supports what-if) args array, and declare `whatIfReturns: "state"` on `set`: | ||
|
|
||
| ```json | ||
| "set": { | ||
| "executable": "<resource_name>", | ||
| "args": [ | ||
| "config", "set", | ||
| { "jsonInputArg": "--input", "mandatory": true }, | ||
| { "whatIfArg": "-w" } | ||
| ], | ||
| "whatIfReturns": "state" | ||
| }, | ||
| "delete": { | ||
| "executable": "<resource_name>", | ||
| "args": [ | ||
| "config", "delete", | ||
| { "jsonInputArg": "--input", "mandatory": true }, | ||
| { "whatIfArg": "-w" } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| - `whatIfArg` is the literal CLI flag DSC will append when the user runs `dsc config set --what-if`. Always use `"-w"` (short form) for consistency across resources. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since most resources won't also be used as CLI tools (although technically they could), would prefer to advise agent to use long form in the manifest |
||
| - `whatIfReturns: "state"` tells DSC the executable prints the projected post-state JSON on stdout (same shape as `get`/`set` returns). | ||
| - The `--list` (bulk) variant of a resource uses the same two manifest additions; do not invent new flag names. | ||
|
|
||
| ### 2. CLI args (`args.rs`) changes | ||
|
|
||
| Add a `-w` / `--what-if` boolean to every `ConfigSubCommand` variant that can support what-if: | ||
|
|
||
| ```rust | ||
| #[derive(Debug, PartialEq, Eq, Subcommand)] | ||
| pub enum ConfigSubCommand { | ||
| #[clap(name = "set", about = t!("args.configSetAbout").to_string())] | ||
| Set { | ||
| #[clap(short, long, required = true, help = t!("args.configArgsInputHelp").to_string())] | ||
| input: String, | ||
| #[clap(short = 'w', long, help = t!("args.configArgsWhatIfHelp").to_string())] | ||
| what_if: bool, | ||
| }, | ||
| #[clap(name = "delete", about = t!("args.configDeleteAbout").to_string())] | ||
| Delete { | ||
| #[clap(short, long, required = true, help = t!("args.configArgsInputHelp").to_string())] | ||
| input: String, | ||
| #[clap(short = 'w', long, help = t!("args.configArgsWhatIfHelp").to_string())] | ||
| what_if: bool, | ||
| }, | ||
| } | ||
| ``` | ||
|
|
||
| Naming is fixed: clap field is `what_if`, short flag is `-w`, long flag is `--what-if`, help key is `args.configArgsWhatIfHelp`. | ||
|
|
||
| ### 3. `main.rs` dispatch | ||
|
|
||
| In each `Set` / `Delete` arm, destructure `what_if`, call `helper.enable_what_if()` when true, and print the returned projected state on stdout. **Never** mutate state when `what_if` is true. | ||
|
|
||
| ```rust | ||
| args::ConfigSubCommand::Set { input, what_if } => { | ||
| trace!("Set input: {input}, what_if: {what_if}"); | ||
| let mut helper = match Helper::new_from_json(&input) { | ||
| Ok(h) => h, | ||
| Err(err) => { error!("{err}"); exit(EXIT_INVALID_INPUT); } | ||
| }; | ||
| if what_if { helper.enable_what_if(); } | ||
|
|
||
| match helper.set() { | ||
| Ok(Some(state)) => { | ||
| // Set returns Some(state) when what_if is true (projected state) | ||
| // OR when whatIfReturns == "state" and the resource emits final state. | ||
| let json = serde_json::to_string(&state).unwrap(); | ||
| println!("{json}"); | ||
| } | ||
| Ok(None) => {} | ||
| Err(err) => { error!("{err}"); exit(EXIT_RESOURCE_ERROR); } | ||
| } | ||
| exit(EXIT_SUCCESS); | ||
| } | ||
| ``` | ||
|
|
||
| For the `--list` bulk variant, accumulate projected states in a `Vec`, then print the whole list once at the end. | ||
|
|
||
| ### 4. Library / helper changes | ||
|
|
||
| In the resource's `dsc-lib-*` crate: | ||
|
|
||
| - Add a `what_if: bool` field on the helper struct, defaulting to `false` in every constructor. | ||
| - Expose `pub fn enable_what_if(&mut self) { self.what_if = true; }`. | ||
| - Change `set()` (and `remove()`) to return `Result<Option<T>, Error>` where `Some(T)` is the projected state when `what_if` is true. | ||
| - Inside `set` / `remove`, build a `Vec<String> what_if_metadata`, push localized "Would …" strings at each side-effecting branch, and **short-circuit** before the real OS call when `self.what_if`: | ||
|
|
||
| ```rust | ||
| if self.what_if { | ||
| what_if_metadata.push(t!("<resource>_helper.whatIfCreate<Thing>", name = name).to_string()); | ||
| } else { | ||
| // perform the real OS mutation | ||
| } | ||
| ``` | ||
|
|
||
| - Return the projected state with the metadata attached: | ||
|
|
||
| ```rust | ||
| return Ok(Some(<ResourceState> { | ||
| // identity + projected fields the engine needs to diff | ||
| metadata: if what_if_metadata.is_empty() { | ||
| None | ||
| } else { | ||
| Some(Metadata { what_if: Some(what_if_metadata) }) | ||
| }, | ||
| ..Default::default() | ||
| })); | ||
| ``` | ||
|
|
||
| - Add a `handle_error_or_what_if(error)` helper that, in what-if mode, turns an error into a projected state whose `_metadata.whatIf` contains the error message, instead of failing the run: | ||
|
|
||
| ```rust | ||
| fn handle_error_or_what_if(&self, error: Error) -> Result<Option<T>, Error> { | ||
| if self.what_if { | ||
| return Ok(Some(T { | ||
| // identity fields from self.config | ||
| metadata: Some(Metadata { what_if: Some(vec![error.to_string()]) }), | ||
| ..Default::default() | ||
| })); | ||
| } | ||
| Err(error) | ||
| } | ||
| ``` | ||
|
|
||
| - Handle the `_exist: false` delete case inside `set()` by routing through `remove()` (with `what_if` honored), so users get a single what-if message describing the deletion. | ||
|
|
||
| ### 5. Types (`config.rs` / `types.rs`) changes | ||
|
|
||
| Add a `_metadata` field of type `Option<Metadata>` to every public state struct, and define `Metadata` exactly once per crate: | ||
|
|
||
| ```rust | ||
| #[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] | ||
| #[serde(rename = "<ResourceName>", deny_unknown_fields)] | ||
| pub struct <ResourceName> { | ||
| // ... resource properties ... | ||
|
|
||
| #[serde(rename = "_metadata", skip_serializing_if = "Option::is_none")] | ||
| pub metadata: Option<Metadata>, | ||
|
|
||
| #[serde(rename = "_exist", skip_serializing_if = "Option::is_none")] | ||
| pub exist: Option<bool>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct Metadata { | ||
| #[serde(rename = "whatIf", skip_serializing_if = "Option::is_none")] | ||
| pub what_if: Option<Vec<String>>, | ||
| } | ||
| ``` | ||
|
|
||
| Naming is fixed: JSON field is `_metadata`, nested array is `whatIf`, Rust field is `what_if: Option<Vec<String>>`. | ||
|
|
||
| ### 6. Localization strings | ||
|
|
||
| Add localized what-if messages to `locales/en-us.toml` under the helper's section, all starting with the verb **"Would"**: | ||
|
|
||
| ```toml | ||
| [<resource>_helper] | ||
| whatIfCreate<Thing> = "Would create %{name}" | ||
| whatIfUpdate<Thing> = "Would update %{name} to '%{value}'" | ||
| whatIfDelete<Thing> = "Would delete %{name} '%{value}'" | ||
| ``` | ||
|
|
||
| Examples: | ||
|
|
||
| ```toml | ||
| whatIfCreate<Thing> = "<Thing> '%{name}' not found, would create it" | ||
| whatIfDelete<Thing> = "Would delete <thing> '%{name}'" | ||
| ``` | ||
|
|
||
| Also add `args.configArgsWhatIfHelp = "Run the operation in what-if mode"` (or equivalent) for the clap flag. | ||
|
|
||
| ### 7. What-if Pester tests | ||
|
|
||
| Create one dedicated test file per resource variant. Names are fixed: | ||
|
|
||
| - `<resource>.config.whatif.tests.ps1` — single-instance what-if | ||
| - `<resource>list_whatif.tests.ps1` — `--list` bulk what-if (only if the resource has a list variant) | ||
|
|
||
| Structure: | ||
|
|
||
| ```powershell | ||
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT License. | ||
|
|
||
| Describe '<resource> config whatif tests' { | ||
| BeforeAll { | ||
| # Ensure a clean starting state | ||
| } | ||
|
|
||
| AfterEach { | ||
| # Roll back anything a test may have created | ||
| } | ||
|
|
||
| It 'Can whatif a new <thing>' -Skip:(!$IsWindows) { | ||
| $json = @' | ||
| { "<key>": "<value>" } | ||
| '@ | ||
| # 1. Capture pre-state | ||
| $get_before = <resource> config get --input $json 2>$null | ||
|
|
||
| # 2. Run what-if | ||
| $result = <resource> config set -w --input $json 2>$null | ConvertFrom-Json | ||
|
|
||
| # 3. Assert success + projected state + whatIf metadata | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $result.<key> | Should -Be '<value>' | ||
| $result._metadata.whatIf[0] | Should -Match '.*<expected fragment>.*' | ||
|
|
||
| # 4. Assert NO mutation happened | ||
| $get_after = <resource> config get --input $json 2>$null | ||
| $get_before | Should -EQ $get_after | ||
| } | ||
|
|
||
| It 'Can whatif delete an existing <thing> using _exist is false' -Skip:(!$IsWindows) { | ||
| # ... arrange real state via plain `config set` ... | ||
| $whatif_delete = @' | ||
| { "<key>": "<value>", "_exist": false } | ||
| '@ | ||
| $result = <resource> config set -w --input $whatif_delete 2>$null | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $result._metadata.whatIf | Should -Match "Would delete .*" | ||
| } | ||
|
|
||
| It 'Can whatif delete an existing <thing>' -Skip:(!$IsWindows) { | ||
| # Same as above, but via the `delete` subcommand: | ||
| $result = <resource> config delete -w --input $whatif_delete 2>$null | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $result._metadata.whatIf | Should -Match "Would delete .*" | ||
| # For delete what-if, payload should only include identity fields (and _metadata) | ||
| ($result.psobject.properties | Where-Object { $_.Name -ne '_metadata' } | Measure-Object).Count | | ||
| Should -Be 1 | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Rules for what-if tests: | ||
|
|
||
| - Always call the executable directly (`<resource> config set -w --input ...`), **not** via `dsc resource`, so the test pins the CLI contract used by the manifest. | ||
| - Use `-w` (not `--what-if`) in tests to lock in the short flag. | ||
| - Redirect stderr with `2>$null` to keep test output clean; for failing-test debugging, prefer `2>$testdrive/error.log` + `-Because`. | ||
| - Pipe through `ConvertFrom-Json` and assert on `_metadata.whatIf` entries with `Should -Match`. | ||
| - Always include at least one assertion that the system state did **not** change (compare `config get` before/after). | ||
| - Always include both `set -w` (with and without `_exist: false`) and `delete -w` coverage if the manifest exposes `delete` what-if. | ||
|
Gijsreyn marked this conversation as resolved.
|
||
| - Top-level `Describe` block uses `-Skip:(!$IsWindows)` for Windows-only resources (or the appropriate platform guard). | ||
|
|
||
| ### 8. Naming convention summary (do not deviate) | ||
|
|
||
| | Concern | Name | | ||
| |---|---| | ||
| | CLI short flag | `-w` | | ||
| | CLI long flag | `--what-if` | | ||
| | Clap field | `what_if: bool` | | ||
| | Helper field | `what_if: bool` | | ||
| | Helper enable method | `enable_what_if()` | | ||
| | Manifest arg entry | `{ "whatIfArg": "-w" }` | | ||
| | Manifest declaration | `"whatIfReturns": "state"` (on `set`) | | ||
| | State field | `_metadata` (`metadata: Option<Metadata>` in Rust) | | ||
| | Metadata field | `whatIf` (`what_if: Option<Vec<String>>` in Rust) | | ||
| | Locale section | `[<resource>_helper]` | | ||
| | Locale key prefix | `whatIfCreate*`, `whatIfUpdate*`, `whatIfDelete*` | | ||
| | Locale message style | starts with `"Would "` | | ||
| | Error-to-whatif helper | `handle_error_or_what_if(error)` | | ||
| | Test file (single) | `<resource>.config.whatif.tests.ps1` | | ||
| | Test file (list) | `<resource>list_whatif.tests.ps1` | | ||
| | Describe block title | `'<resource> config whatif tests'` | | ||
|
|
||
| ### 9. Implementation checklist | ||
|
|
||
| When asked to add what-if to a new resource, perform these steps in order: | ||
|
|
||
| 1. Update the resource manifest: add `whatIfArg` to `set` (and `delete`) args, add `whatIfReturns: "state"` to `set`. | ||
| 2. Add `what_if: bool` to the relevant clap `ConfigSubCommand` variants in `args.rs`. | ||
| 3. Destructure `what_if` in `main.rs`, call `helper.enable_what_if()`, print projected state JSON. | ||
| 4. Add `what_if` field, `enable_what_if()` method, and `handle_error_or_what_if()` helper to the resource library struct. | ||
| 5. Change `set()` / `remove()` to short-circuit OS mutations when `what_if`, accumulate `Vec<String>` of "Would …" messages, return `Some(state)` with `_metadata.whatIf` attached. | ||
| 6. Add `_metadata: Option<Metadata>` to public state structs and define `Metadata { what_if: Option<Vec<String>> }` with the JSON renames shown above. | ||
| 7. Add `whatIf*` localized strings under `[<resource>_helper]` and `args.configArgsWhatIfHelp` in `locales/en-us.toml`. | ||
| 8. Create `<resource>.config.whatif.tests.ps1` (and the list variant if applicable) following the test template; cover create, update, delete-via-`_exist`, and `delete -w`. | ||
| 9. Build with `./build.ps1 -project <resource_name>` and run the new Pester file. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.