Skip to content

Commit 6195657

Browse files
tgauthtgauth
committed
address review feedback
1 parent 1cb1b50 commit 6195657

File tree

5 files changed

+63
-46
lines changed

5 files changed

+63
-46
lines changed

‎dsc/tests/dsc_whatif.tests.ps1

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ Describe 'whatif tests' {
4141
$what_if_result.results.result.beforeState._exist | Should -Be $set_result.results.result.beforeState._exist
4242
$what_if_result.results.result.beforeState.keyPath | Should -Be $set_result.results.result.beforeState.keyPath
4343
$what_if_result.results.result.afterState.KeyPath | Should -Be $set_result.results.result.afterState.keyPath
44-
$what_if_result.results.result.changedProperties | Should -Be $set_result.results.result.changedProperties
44+
# can be changed back to the following once _metadata is pulled out of resource return
45+
# $what_if_result.results.result.changedProperties | Should -Be $set_result.results.result.changedProperties
46+
$what_if_result.results.result.changedProperties | Should -Be @('_metadata', '_exist')
4547
$what_if_result.hadErrors | Should -BeFalse
4648
$what_if_result.results.Count | Should -Be 1
4749
$LASTEXITCODE | Should -Be 0

‎registry/src/config.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ pub struct Registry {
2121
#[serde(rename = "keyPath")]
2222
pub key_path: String,
2323
/// The information from a config set --what-if operation.
24-
#[serde(flatten, skip_serializing_if = "Option::is_none")]
25-
pub what_if: Option<WhatIf>,
24+
#[serde(rename = "_metadata", skip_serializing_if = "Option::is_none")]
25+
pub metadata: Option<Metadata>,
2626
/// The name of the registry value.
2727
#[serde(rename = "valueName", skip_serializing_if = "Option::is_none")]
2828
pub value_name: Option<String>,
@@ -35,7 +35,7 @@ pub struct Registry {
3535

3636
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema)]
3737
#[serde(deny_unknown_fields)]
38-
pub struct WhatIf {
39-
#[serde(skip_serializing_if = "Option::is_none")]
40-
pub error: Option<String>
38+
pub struct Metadata {
39+
#[serde(rename = "whatIf", skip_serializing_if = "Option::is_none")]
40+
pub what_if: Option<Vec<String>>
4141
}

‎registry/src/main.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ fn main() {
4545
args::SubCommand::Config { subcommand } => {
4646
match subcommand {
4747
args::ConfigSubCommand::Get{input} => {
48-
let reg_helper = match RegistryHelper::new(&input) {
48+
let reg_helper = match RegistryHelper::new(&input, false) {
4949
Ok(reg_helper) => reg_helper,
5050
Err(err) => {
5151
eprintln!("Error: {err}");
@@ -64,14 +64,14 @@ fn main() {
6464
}
6565
},
6666
args::ConfigSubCommand::Set{input, what_if} => {
67-
let reg_helper = match RegistryHelper::new(&input) {
67+
let reg_helper = match RegistryHelper::new(&input, what_if) {
6868
Ok(reg_helper) => reg_helper,
6969
Err(err) => {
7070
eprintln!("Error: {err}");
7171
exit(EXIT_INVALID_INPUT);
7272
}
7373
};
74-
match reg_helper.set(what_if) {
74+
match reg_helper.set() {
7575
Ok(reg_config) => {
7676
if let Some(config) = reg_config {
7777
let json = serde_json::to_string(&config).unwrap();
@@ -85,7 +85,7 @@ fn main() {
8585
}
8686
},
8787
args::ConfigSubCommand::Delete{input} => {
88-
let reg_helper = match RegistryHelper::new(&input) {
88+
let reg_helper = match RegistryHelper::new(&input, false) {
8989
Ok(reg_helper) => reg_helper,
9090
Err(err) => {
9191
eprintln!("Error: {err}");

‎registry/src/registry_helper.rs

Lines changed: 43 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,18 @@
33

44
use registry::{Data, Hive, RegKey, Security, key, value};
55
use utfx::{U16CString, UCString};
6-
use crate::config::{Registry, RegistryValueData, WhatIf};
6+
use crate::config::{Metadata, Registry, RegistryValueData};
77
use crate::error::RegistryError;
88

99
pub struct RegistryHelper {
1010
config: Registry,
1111
hive: Hive,
1212
subkey: String,
13+
what_if: bool,
1314
}
1415

1516
impl RegistryHelper {
16-
pub fn new(config: &str) -> Result<Self, RegistryError> {
17+
pub fn new(config: &str, what_if: bool) -> Result<Self, RegistryError> {
1718
let registry: Registry = match serde_json::from_str(config) {
1819
Ok(config) => config,
1920
Err(e) => return Err(RegistryError::Json(e)),
@@ -26,6 +27,7 @@ impl RegistryHelper {
2627
config: registry,
2728
hive,
2829
subkey: subkey.to_string(),
30+
what_if
2931
}
3032
)
3133
}
@@ -76,49 +78,54 @@ impl RegistryHelper {
7678
}
7779
}
7880

79-
pub fn set(&self, is_what_if: bool) -> Result<Option<Registry>, RegistryError> {
81+
pub fn set(&self) -> Result<Option<Registry>, RegistryError> {
82+
let mut what_if_metadata: Vec<String> = Vec::new();
8083
let reg_key = match self.open(Security::Write) {
8184
Ok((reg_key, _subkey)) => Some(reg_key),
8285
// handle NotFound error
8386
Err(RegistryError::RegistryKeyNotFound(_)) => {
8487
// if the key doesn't exist, some of the parent keys may
8588
// not exist either, so we need to find the valid parent key
8689
// and then create the subkeys that don't exist
87-
if is_what_if {
88-
None
89-
}
90-
else {
91-
let (parent_key, subkeys) = self.get_valid_parent_key_and_subkeys()?;
92-
let mut reg_key = parent_key;
93-
for subkey in subkeys {
94-
let Ok(path) = UCString::<u16>::from_str(subkey) else {
95-
return Err(RegistryError::Utf16Conversion("subkey".to_string()));
96-
};
90+
let (parent_key, subkeys) = self.get_valid_parent_key_and_subkeys()?;
91+
let mut reg_key = parent_key;
92+
for subkey in subkeys {
93+
let Ok(path) = UCString::<u16>::from_str(subkey) else {
94+
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("subkey".to_string()));
95+
};
9796

97+
if self.what_if {
98+
what_if_metadata.push(format!("key: {subkey} not found, would create it"));
99+
}
100+
else {
98101
reg_key = reg_key.create(path, Security::CreateSubKey)?;
99102
}
100-
103+
}
104+
if self.what_if {
105+
None
106+
}
107+
else {
101108
Some(self.open(Security::Write)?.0)
102109
}
103110
},
104-
Err(e) => return self.handle_error_or_what_if(e, is_what_if)
111+
Err(e) => return self.handle_error_or_what_if(e)
105112
};
106113

107114
if let Some(value_data) = &self.config.value_data {
108115
let Ok(value_name) = U16CString::from_str(self.config.value_name.as_ref().unwrap()) else {
109-
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("valueName".to_string()), is_what_if);
116+
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("valueName".to_string()));
110117
};
111118

112119
let data = match value_data {
113120
RegistryValueData::String(s) => {
114121
let Ok(utf16) = U16CString::from_str(s) else {
115-
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("valueData".to_string()), is_what_if);
122+
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("valueData".to_string()));
116123
};
117124
Data::String(utf16)
118125
},
119126
RegistryValueData::ExpandString(s) => {
120127
let Ok(utf16) = U16CString::from_str(s) else {
121-
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("valueData".to_string()), is_what_if);
128+
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("valueData".to_string()));
122129
};
123130
Data::ExpandString(utf16)
124131
},
@@ -132,7 +139,7 @@ impl RegistryHelper {
132139
let mut m16: Vec<UCString<u16>> = Vec::<UCString<u16>>::new();
133140
for s in m {
134141
let Ok(utf16) = U16CString::from_str(s) else {
135-
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("valueData".to_string()), is_what_if);
142+
return self.handle_error_or_what_if(RegistryError::Utf16Conversion("valueData".to_string()));
136143
};
137144
m16.push(utf16);
138145
}
@@ -143,12 +150,12 @@ impl RegistryHelper {
143150
},
144151
};
145152

146-
if is_what_if {
153+
if self.what_if {
147154
return Ok(Some(Registry {
148155
key_path: self.config.key_path.clone(),
149156
value_data: Some(convert_reg_value(&data)?),
150157
value_name: self.config.value_name.clone(),
151-
what_if: Some(WhatIf { error: None }),
158+
metadata: if what_if_metadata.is_empty() { None } else { Some(Metadata { what_if: Some(what_if_metadata) })},
152159
..Default::default()
153160
}));
154161
}
@@ -158,10 +165,10 @@ impl RegistryHelper {
158165
};
159166
}
160167

161-
if is_what_if {
168+
if self.what_if {
162169
return Ok(Some(Registry {
163170
key_path: self.config.key_path.clone(),
164-
what_if: Some(WhatIf { error: None }),
171+
metadata: if what_if_metadata.is_empty() { None } else { Some(Metadata { what_if: Some(what_if_metadata) })},
165172
..Default::default()
166173
}));
167174
}
@@ -240,12 +247,12 @@ impl RegistryHelper {
240247
Ok((parent_key, subkeys))
241248
}
242249

243-
fn handle_error_or_what_if(&self, error: RegistryError, is_what_if: bool) -> Result<Option<Registry>, RegistryError> {
250+
fn handle_error_or_what_if(&self, error: RegistryError) -> Result<Option<Registry>, RegistryError> {
244251
// TODO: return error message via metadata instead of as property within set state
245-
if is_what_if {
252+
if self.what_if {
246253
return Ok(Some(Registry {
247254
key_path: self.config.key_path.clone(),
248-
what_if: Some(WhatIf { error: Some(error.to_string()) }),
255+
metadata: Some(Metadata { what_if: Some(vec![error.to_string()]) }),
249256
..Default::default()
250257
}));
251258
}
@@ -310,7 +317,7 @@ fn convert_reg_value(value: &Data) -> Result<RegistryValueData, RegistryError> {
310317

311318
#[test]
312319
fn get_hklm_key() {
313-
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKEY_LOCAL_MACHINE"}"#).unwrap();
320+
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKEY_LOCAL_MACHINE"}"#, false).unwrap();
314321
let reg_config = reg_helper.get().unwrap();
315322
assert_eq!(reg_config.key_path, r#"HKEY_LOCAL_MACHINE"#);
316323
assert_eq!(reg_config.value_name, None);
@@ -319,7 +326,7 @@ fn get_hklm_key() {
319326

320327
#[test]
321328
fn get_product_name() {
322-
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKLM\\SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion","valueName":"ProductName"}"#).unwrap();
329+
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKLM\\SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion","valueName":"ProductName"}"#, false).unwrap();
323330
let reg_config = reg_helper.get().unwrap();
324331
assert_eq!(reg_config.key_path, r#"HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion"#);
325332
assert_eq!(reg_config.value_name, Some("ProductName".to_string()));
@@ -328,7 +335,7 @@ fn get_product_name() {
328335

329336
#[test]
330337
fn get_nonexisting_key() {
331-
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DoesNotExist"}"#).unwrap();
338+
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DoesNotExist"}"#, false).unwrap();
332339
let reg_config = reg_helper.get().unwrap();
333340
assert_eq!(reg_config.key_path, r#"HKCU\DoesNotExist"#);
334341
assert_eq!(reg_config.value_name, None);
@@ -338,7 +345,7 @@ fn get_nonexisting_key() {
338345

339346
#[test]
340347
fn get_nonexisting_value() {
341-
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\Software","valueName":"DoesNotExist"}"#).unwrap();
348+
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\Software","valueName":"DoesNotExist"}"#, false).unwrap();
342349
let reg_config = reg_helper.get().unwrap();
343350
assert_eq!(reg_config.key_path, r#"HKCU\Software"#);
344351
assert_eq!(reg_config.value_name, Some("DoesNotExist".to_string()));
@@ -348,8 +355,8 @@ fn get_nonexisting_value() {
348355

349356
#[test]
350357
fn set_and_remove_test_value() {
351-
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DSCTest\\DSCSubKey","valueName":"TestValue","valueData": { "String": "Hello"} }"#).unwrap();
352-
reg_helper.set(false).unwrap();
358+
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DSCTest\\DSCSubKey","valueName":"TestValue","valueData": { "String": "Hello"} }"#, false).unwrap();
359+
reg_helper.set().unwrap();
353360
let result = reg_helper.get().unwrap();
354361
assert_eq!(result.key_path, r#"HKCU\DSCTest\DSCSubKey"#);
355362
assert_eq!(result.value_name, Some("TestValue".to_string()));
@@ -360,7 +367,7 @@ fn set_and_remove_test_value() {
360367
assert_eq!(result.value_name, Some("TestValue".to_string()));
361368
assert_eq!(result.value_data, None);
362369
assert_eq!(result.exist, Some(false));
363-
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DSCTest"}"#).unwrap();
370+
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DSCTest"}"#, false).unwrap();
364371
let result = reg_helper.get().unwrap();
365372
assert_eq!(result.key_path, r#"HKCU\DSCTest"#);
366373
assert_eq!(result.value_name, None);
@@ -375,13 +382,13 @@ fn set_and_remove_test_value() {
375382

376383
#[test]
377384
fn delete_tree() {
378-
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DSCTest2\\DSCSubKey","valueName":"TestValue","valueData": { "String": "Hello"} }"#).unwrap();
379-
reg_helper.set(false).unwrap();
385+
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DSCTest2\\DSCSubKey","valueName":"TestValue","valueData": { "String": "Hello"} }"#, false).unwrap();
386+
reg_helper.set().unwrap();
380387
let result = reg_helper.get().unwrap();
381388
assert_eq!(result.key_path, r#"HKCU\DSCTest2\DSCSubKey"#);
382389
assert_eq!(result.value_name, Some("TestValue".to_string()));
383390
assert_eq!(result.value_data, Some(RegistryValueData::String("Hello".to_string())));
384-
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DSCTest2"}"#).unwrap();
391+
let reg_helper = RegistryHelper::new(r#"{"keyPath":"HKCU\\DSCTest2"}"#, false).unwrap();
385392
reg_helper.remove().unwrap();
386393
let result = reg_helper.get().unwrap();
387394
assert_eq!(result.key_path, r#"HKCU\DSCTest2"#);

‎registry/tests/registry.config.whatif.tests.ps1

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ Describe 'registry config whatif tests' {
2020
$result = registry config set -w --input $json | ConvertFrom-Json
2121
$LASTEXITCODE | Should -Be 0
2222
$result.keyPath | Should -Be 'HKCU\1\2\3'
23+
$result.metadata.whatIf[0] | Should -Match '.*1.*'
24+
$result.metadata.whatIf[1] | Should -Match '.*2.*'
25+
$result.metadata.whatIf[2] | Should -Match '.*3.*'
2326
$get_after = registry config get --input $json
2427
$get_before | Should -EQ $get_after
2528
}
@@ -39,6 +42,9 @@ Describe 'registry config whatif tests' {
3942
$result.keyPath | Should -Be 'HKCU\1\2\3'
4043
$result.valueName | Should -Be 'Hello'
4144
$result.valueData.String | Should -Be 'World'
45+
$result.metadata.whatIf[0] | Should -Match '.*1.*'
46+
$result.metadata.whatIf[1] | Should -Match '.*2.*'
47+
$result.metadata.whatIf[2] | Should -Match '.*3.*'
4248
}
4349

4450
It 'Can whatif an existing key with new value' -Skip:(!$IsWindows) {
@@ -62,6 +68,7 @@ Describe 'registry config whatif tests' {
6268
$result.keyPath | Should -Be 'HKCU\1\2'
6369
$result.valueName | Should -Be 'Hello'
6470
$result.valueData.String | Should -Be 'World'
71+
($result.psobject.properties | Measure-Object).Count | Should -Be 3
6572
}
6673

6774
It 'Can whatif an existing deeply nested key and value' -Skip:(!$IsWindows) {
@@ -89,6 +96,7 @@ Describe 'registry config whatif tests' {
8996
$result.keyPath | Should -Be 'HKCU\1\2\3'
9097
$result.valueName | Should -Be 'Hello'
9198
$result.valueData.String | Should -Be 'World-WhatIf'
99+
($result.psobject.properties | Measure-Object).Count | Should -Be 3
92100
}
93101

94102
It 'Can whatif an existing key with nested values' -Skip:(!$IsWindows) {

0 commit comments

Comments
 (0)