Skip to content
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.

Sign up for GitHub

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

Refactor arguments to command resources an mixed strings and objects for extensibility #385

Merged
merged 10 commits into from
Apr 13, 2024
Merged

Refactor arguments to command resources an mixed strings and objects for extensibility #385

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
79d30ca
Change how args get passed to commands
SteveL-MSFT Apr 5, 2024
32076fb
fix resource manifests
SteveL-MSFT Apr 5, 2024
acf9032
fix manifest for Test/Delete resource
SteveL-MSFT Apr 5, 2024
fdab2f1
fix registry resource
SteveL-MSFT Apr 6, 2024
eb66390
Merge branch 'main' into arg-object
SteveL-MSFT Apr 10, 2024
aeabe24
update test
Apr 10, 2024
6218dcf
Merge branch 'main' into arg-object
SteveL-MSFT Apr 12, 2024
6db080b
address Andrew and Tess' feedback
Apr 12, 2024
ba80ec7
fix clippy
Apr 13, 2024
bff600e
add tracing, fix winps resource manifest
Apr 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion dsc/assertion.dsc.resource.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"args": [
"config",
"validate"
]
],
"input": "stdin"
}
}
3 changes: 2 additions & 1 deletion dsc/group.dsc.resource.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"args": [
"config",
"validate"
]
],
"input": "stdin"
}
}
3 changes: 2 additions & 1 deletion dsc/parallel.dsc.resource.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"args": [
"config",
"validate"
]
],
"input": "stdin"
}
}
14 changes: 5 additions & 9 deletions dsc/tests/dsc_args.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,24 +54,20 @@ Describe 'config argument tests' {
$env:DSC_RESOURCE_PATH = $oldPath
}

It 'input is <type>' -Skip:(!$IsWindows) -TestCases @(
It 'input is <type>' -TestCases @(
@{ type = 'yaml'; text = @'
keyPath: HKLM\Software\Microsoft\Windows NT\CurrentVersion
valueName: ProductName
output: Hello There
'@ }
@{ type = 'json'; text = @'
{
"keyPath": "HKLM\\Software\\Microsoft\\Windows NT\\CurrentVersion",
"valueName": "ProductName"
"output": "Hello There"
}
'@ }
) {
param($text)
$output = $text | dsc resource get -r Microsoft.Windows/Registry
$output = $text | dsc resource get -r Test/Echo
$output = $output | ConvertFrom-Json
$output.actualState.keyPath | Should -BeExactly 'HKLM\Software\Microsoft\Windows NT\CurrentVersion'
$output.actualState.valueName | Should -BeExactly 'ProductName'
$output.actualState.valueData.String | Should -Match 'Windows .*'
$output.actualState.output | Should -BeExactly 'Hello There'
}

It '--format <format> is used even when redirected' -TestCases @(
Expand Down
9 changes: 2 additions & 7 deletions dsc/tests/dsc_discovery.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,8 @@ Describe 'tests for resource discovery' {
"get": {
"executable": "dsctest",
"args": [
"echo",
"--input",
"{json}"
],
"input": {
"arg": "{json}"
}
"echo"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it's not matching the

{
  "jsonInputArg": "<parameter>",
  "mandatory": true
}

syntax?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a test resource that doesn't get run to validate the semver. Maybe I'll rename the type to make this clear and remove the args since they aren't used at all.

]
},
"schema": {
"command": {
Expand Down
135 changes: 88 additions & 47 deletions dsc_lib/src/dscresources/command_resource.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@

use jsonschema::JSONSchema;
use serde_json::Value;
use std::{collections::HashMap, env, process::Command, io::{Write, Read}, process::Stdio};
use std::{collections::HashMap, env, io::{Read, Write}, process::{Command, Stdio}};
use crate::{dscerror::DscError, dscresources::invoke_result::{ResourceGetResponse, ResourceSetResponse, ResourceTestResponse}};
use crate::configure::config_result::ResourceGetResult;
use super::{dscresource::get_diff,resource_manifest::{Kind, ResourceManifest, InputKind, ReturnKind, SchemaKind}, invoke_result::{GetResult, SetResult, TestResult, ValidateResult, ExportResult}};
use super::{dscresource::get_diff, invoke_result::{ExportResult, GetResult, SetResult, TestResult, ValidateResult}, resource_manifest::{ArgKind, InputKind, Kind, ResourceManifest, ReturnKind, SchemaKind}};
use tracing::{error, warn, info, debug, trace};

pub const EXIT_PROCESS_TERMINATED: i32 = 0x102;
Expand Down Expand Up @@ -54,7 +54,7 @@ pub fn invoke_get(resource: &ResourceManifest, cwd: &str, filter: &str) -> Resul

let mut env: Option<HashMap<String, String>> = None;
let mut input_filter: Option<&str> = None;
let mut get_args = resource.get.args.clone();
let args = process_args(&resource.get.args, filter);
if !filter.is_empty() {
verify_json(resource, cwd, filter)?;

Expand All @@ -65,14 +65,11 @@ pub fn invoke_get(resource: &ResourceManifest, cwd: &str, filter: &str) -> Resul
InputKind::Stdin => {
input_filter = Some(filter);
},
InputKind::Arg(arg_name) => {
replace_token(&mut get_args, &arg_name, filter)?;
},
}
}

info!("Invoking get '{}' using '{}'", &resource.resource_type, &resource.get.executable);
let (exit_code, stdout, stderr) = invoke_command(&resource.get.executable, get_args, input_filter, Some(cwd), env)?;
let (exit_code, stdout, stderr) = invoke_command(&resource.get.executable, args, input_filter, Some(cwd), env)?;
log_resource_traces(&stderr);
if exit_code != 0 {
return Err(DscError::Command(resource.resource_type.clone(), exit_code, stderr));
Expand Down Expand Up @@ -114,7 +111,7 @@ pub fn invoke_get(resource: &ResourceManifest, cwd: &str, filter: &str) -> Resul
/// Error returned if the resource does not successfully set the desired state
#[allow(clippy::too_many_lines)]
pub fn invoke_set(resource: &ResourceManifest, cwd: &str, desired: &str, skip_test: bool) -> Result<SetResult, DscError> {
let Some(set) = resource.set.as_ref() else {
let Some(set) = &resource.set else {
return Err(DscError::NotImplemented("set".to_string()));
};
verify_json(resource, cwd, desired)?;
Expand Down Expand Up @@ -146,24 +143,21 @@ pub fn invoke_set(resource: &ResourceManifest, cwd: &str, desired: &str, skip_te

let mut get_env: Option<HashMap<String, String>> = None;
let mut get_input: Option<&str> = None;
let mut get_args = resource.get.args.clone();
let args = process_args(&resource.get.args, desired);
match &resource.get.input {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be simpler to add this match block to process_args or as its own function since it's repeated within the other commands?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is only for processing args, but you're right there's some redundancy. Let me pull out the common part as a function.

Some(InputKind::Env) => {
get_env = Some(json_to_hashmap(desired)?);
},
Some(InputKind::Stdin) => {
get_input = Some(desired);
},
Some(InputKind::Arg(arg_token)) => {
replace_token(&mut get_args, arg_token, desired)?;
},
None => {
// leave input as none
},
}

info!("Getting current state for set by invoking get {} using {}", &resource.resource_type, &resource.get.executable);
let (exit_code, stdout, stderr) = invoke_command(&resource.get.executable, get_args, get_input, Some(cwd), get_env)?;
let (exit_code, stdout, stderr) = invoke_command(&resource.get.executable, args, get_input, Some(cwd), get_env)?;
log_resource_traces(&stderr);
if exit_code != 0 {
return Err(DscError::Command(resource.resource_type.clone(), exit_code, stderr));
Expand All @@ -183,16 +177,16 @@ pub fn invoke_set(resource: &ResourceManifest, cwd: &str, desired: &str, skip_te

let mut env: Option<HashMap<String, String>> = None;
let mut input_desired: Option<&str> = None;
let mut args = set.args.clone();
let args = process_args(&set.args, desired);
match &set.input {
InputKind::Env => {
Some(InputKind::Env) => {
env = Some(json_to_hashmap(desired)?);
},
InputKind::Stdin => {
Some(InputKind::Stdin) => {
input_desired = Some(desired);
},
InputKind::Arg(arg_token) => {
replace_token(&mut args, arg_token, desired)?;
None => {
// leave input as none
},
}

Expand Down Expand Up @@ -290,16 +284,16 @@ pub fn invoke_test(resource: &ResourceManifest, cwd: &str, expected: &str) -> Re

let mut env: Option<HashMap<String, String>> = None;
let mut input_expected: Option<&str> = None;
let mut args = test.args.clone();
let args = process_args(&test.args, expected);
match &test.input {
InputKind::Env => {
Some(InputKind::Env) => {
env = Some(json_to_hashmap(expected)?);
},
InputKind::Stdin => {
Some(InputKind::Stdin) => {
input_expected = Some(expected);
},
InputKind::Arg(arg_token) => {
replace_token(&mut args, arg_token, expected)?;
None => {
// leave input as none
},
}

Expand Down Expand Up @@ -401,39 +395,40 @@ fn invoke_synthetic_test(resource: &ResourceManifest, cwd: &str, expected: &str)
}

/// Invoke the delete operation against a command resource.
///
///
/// # Arguments
///
///
/// * `resource` - The resource manifest for the command resource.
/// * `cwd` - The current working directory.
/// * `filter` - The filter to apply to the resource in JSON.
///
///
/// # Errors
///
///
/// Error is returned if the underlying command returns a non-zero exit code.
pub fn invoke_delete(resource: &ResourceManifest, cwd: &str, filter: &str) -> Result<(), DscError> {
let Some(delete) = &resource.delete else {
return Err(DscError::NotImplemented("delete".to_string()));
};

verify_json(resource, cwd, filter)?;

let mut env: Option<HashMap<String, String>> = None;
let mut input_filter: Option<&str> = None;
let mut delete_args = delete.args.clone();
verify_json(resource, cwd, filter)?;
let args = process_args(&delete.args, filter);
match &delete.input {
InputKind::Env => {
Some(InputKind::Env) => {
env = Some(json_to_hashmap(filter)?);
},
InputKind::Stdin => {
Some(InputKind::Stdin) => {
input_filter = Some(filter);
},
InputKind::Arg(arg_name) => {
replace_token(&mut delete_args, arg_name, filter)?;
None => {
// leave input as none
},
}

info!("Invoking delete '{}' using '{}'", &resource.resource_type, &delete.executable);
let (exit_code, _stdout, stderr) = invoke_command(&delete.executable, delete_args, input_filter, Some(cwd), env)?;
let (exit_code, _stdout, stderr) = invoke_command(&delete.executable, args, input_filter, Some(cwd), env)?;
log_resource_traces(&stderr);
if exit_code != 0 {
return Err(DscError::Command(resource.resource_type.clone(), exit_code, stderr));
Expand Down Expand Up @@ -464,7 +459,22 @@ pub fn invoke_validate(resource: &ResourceManifest, cwd: &str, config: &str) ->
return Err(DscError::NotImplemented("validate".to_string()));
};

let (exit_code, stdout, stderr) = invoke_command(&validate.executable, validate.args.clone(), Some(config), Some(cwd), None)?;
let mut env: Option<HashMap<String, String>> = None;
let mut input_config: Option<&str> = None;
let args = process_args(&validate.args, config);
match &validate.input {
Some(InputKind::Env) => {
env = Some(json_to_hashmap(config)?);
},
Some(InputKind::Stdin) => {
input_config = Some(config);
},
None => {
// leave input as none
},
}

let (exit_code, stdout, stderr) = invoke_command(&validate.executable, args, input_config, Some(cwd), env)?;
log_resource_traces(&stderr);
if exit_code != 0 {
return Err(DscError::Command(resource.resource_type.clone(), exit_code, stderr));
Expand Down Expand Up @@ -536,7 +546,32 @@ pub fn invoke_export(resource: &ResourceManifest, cwd: &str, input: Option<&str>
return Err(DscError::Operation(format!("Export is not supported by resource {}", &resource.resource_type)))
};

let (exit_code, stdout, stderr) = invoke_command(&export.executable, export.args.clone(), input, Some(cwd), None)?;

let mut env: Option<HashMap<String, String>> = None;
let mut export_input: Option<&str> = None;
let args: Option<Vec<String>>;
if let Some(input) = input {
if !input.is_empty() {
verify_json(resource, cwd, input)?;
match &export.input {
Some(InputKind::Env) => {
env = Some(json_to_hashmap(input)?);
},
Some(InputKind::Stdin) => {
export_input = Some(input);
},
None => {
// leave input as none
},
}
}

args = process_args(&export.args, input);
} else {
args = process_args(&export.args, "");
}

let (exit_code, stdout, stderr) = invoke_command(&export.executable, args, export_input, Some(cwd), env)?;
log_resource_traces(&stderr);
if exit_code != 0 {
return Err(DscError::Command(resource.resource_type.clone(), exit_code, stderr));
Expand Down Expand Up @@ -634,24 +669,30 @@ pub fn invoke_command(executable: &str, args: Option<Vec<String>>, input: Option
Ok((exit_code, stdout, stderr))
}

fn replace_token(args: &mut Option<Vec<String>>, token: &str, value: &str) -> Result<(), DscError> {
fn process_args(args: &Option<Vec<ArgKind>>, value: &str) -> Option<Vec<String>> {
let Some(arg_values) = args else {
return Err(DscError::Operation("No args to replace".to_string()));
debug!("No args to process");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of too generic message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted a debug message that may be useful in the future to know that there weren't any args

return None;
};

let mut found = false;
let mut processed_args = Vec::<String>::new();
for arg in arg_values {
if arg == token {
found = true;
*arg = value.to_string();
}
}
match arg {
ArgKind::String(s) => {
processed_args.push(s.clone());
},
ArgKind::Json { json_input_arg, mandatory } => {
if value.is_empty() && *mandatory != Some(true) {
continue;
}

if !found {
return Err(DscError::Operation(format!("Token {token} not found in args")));
processed_args.push(json_input_arg.clone());
processed_args.push(value.to_string());
},
}
}

Ok(())
Some(processed_args)
}

fn verify_json(resource: &ResourceManifest, cwd: &str, json: &str) -> Result<(), DscError> {
Expand Down
Loading
Loading