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.

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

Conversation

SteveL-MSFT
Copy link
Member

PR Summary

  • args are now always processed regardless if how input is specified, so it's technically possible to have the input passed as both an arg and stdin/env
  • args now take a string or an object where the object currently supported looks like:
{
  "jsonInputArg": "<parameter>",
  "mandatory": true
}

The value of jsonInputArg is what the input gets passed to, so "jsonInputArg": "--foo" would be passed as command --foo $json effectively.

The mandatory indicates whether to pass the JSON even if empty. Default is false so it skips providing this parameter completely, but if true, it will always provide the parameter and an empty string if input is empty.

PR Context

Fix #218

@SteveL-MSFT SteveL-MSFT changed the title Arg object Refactor arguments to command resources an mixed strings and objects for extensibility Apr 5, 2024
@michaeltlombardi michaeltlombardi added the Schema-Impact Change requires updating a canonical schema for configs or manifests label Apr 10, 2024
Comment on lines -85 to -87
/// The input replaces arguments with this token in the command.
#[serde(rename = "arg")]
Arg(String),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than deleting this option entirely, I think it makes sense to allow resource authors to explicitly define the input kind as arg to preclude being sent the property bag as environment variables or over stdin and accounting for double processing.

If we do this, we could also update the JSON Schema (and code) to infer the input kind from the manifest - if the args array includes an object and input isn't defined, input is arg. If the args array is only made of strings and input isn't defined, input is stdin. The input is only ever env if it's explicitly defined.

We could then also use JSON schema to validate the args array - if it's explicitly defined as env or stdin, is must not include an object, only strings. If it's not explicitly defined or is defined as arg, it may only include one object in the array.

We could use this to provide better author-time feedback and validation for the resource manifest and direct authors towards best practices.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick json schema example (a little pseudo code, bc it's a little complex to write initially but functional/useful):

# set property in manifest
input:
  type: string
  enum: [arg, env, stdin]
args:
  type: array
  contains: { type: object } # check if array includes objects
  minContains: 0 # allow string-only
  maxContains: 1 # allow json arg only once
  items:
    anyOf:
      - type: string
      - type: object # validate object structure
        required: [jsonInputArg]
        properties:
          jsonInputArg:
            type: string
          mandatory:
            type: boolean
            default: false
# Helper for validation/authoring
allOf:
  - if: # args items are only strings
    then:
      properties:
        input: { default: stdin }
  - if: # args items includes one object
    then:
      properties:
        input: { default: arg }
  - if: # input is arg (default or explicit)
      properties: { input: { const: arg } }
    then: # require object arg exactly once
      properties:
        args: { minContains: 1 }
    else: # forbid object arg entirely
      properties:
        args: { maxContains: 0 }

Copy link
Member Author

Choose a reason for hiding this comment

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

The resource author has to opt into any acceptance of the JSON whether it's env, stdin, or args so there isn't a case where it's accidentally sent. However, there might be a case where the resource might want the simplified reception of the JSON as env, but also keep a raw copy as an arg to pass to something else.

@@ -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.

"input": {
"arg": "{json}"
}
"echo"
Copy link
Collaborator

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.

let Some(arg_values) = args else {
return Err(DscError::Operation("No args to replace".to_string()));
debug!("No args to process");
Copy link
Collaborator

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

@SteveL-MSFT SteveL-MSFT added this pull request to the merge queue Apr 13, 2024
Merged via the queue into PowerShell:main with commit 385cfac Apr 13, 2024
4 checks passed
@SteveL-MSFT SteveL-MSFT deleted the arg-object branch April 13, 2024 02:52
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Apr 16, 2024
This change updates the definition of the command arguments from
an array of strings to an array that can include strings and a
single object representing a JSON input argument, as implemented
in PowerShell#385.

The shared definition includes default snippets to simplify adding
arguments to an `args` list, but not validation for the maximum
number of JSON input arguments.

It updates the definition for the schema command and adapter list
command to expect arrays of strings instead of the shared definition
for the command arguments, as those commands never get JSON data
for input and their struct definition in Rust expects an optional
vector of strings, not command arguments.

It updates the definition for the `delete`, `export`, `get`, `set`,
`test`, and `validate` commands to:

1. Make `input` optional, if it was required before, because that
   matches the struct definitions and DSC no longer requires the
   `input` kind when `args` includes a JSON input argument.
1. Add validation using three branches of the `oneOf` keyword,
   to handle mixed support for the `contains`, `minContains`, and
   `maxContains` keywords across the JSON and YAML support in VS
   Code. It raises specific error messages now when:

   - `input` isn't defined and `args` doesn't include a JSON input
     argument, or includes more than one JSON input argument.
   - `input` is defined and `args` includes more than one JSON
     input argument.
1. Update the default snippets to enable users to quickly define
   the command without any arguments, with only string arguments,
   and with a JSON input argument.
1. Update the in-editor help and clarify that the command must
   define `input`, a JSON input argument in `args`, or both.

This change updates both the source and composed schemas.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Apr 16, 2024
This change updates the definition of the command arguments from
an array of strings to an array that can include strings and a
single object representing a JSON input argument, as implemented
in PowerShell#385.

The shared definition includes default snippets to simplify adding
arguments to an `args` list, but not validation for the maximum
number of JSON input arguments.

It updates the definition for the schema command and adapter list
command to expect arrays of strings instead of the shared definition
for the command arguments, as those commands never get JSON data
for input and their struct definition in Rust expects an optional
vector of strings, not command arguments.

It updates the definition for the `delete`, `export`, `get`, `set`,
`test`, and `validate` commands to:

1. Make `input` optional, if it was required before, because that
   matches the struct definitions and DSC no longer requires the
   `input` kind when `args` includes a JSON input argument.
1. Add validation using three branches of the `oneOf` keyword,
   to handle mixed support for the `contains`, `minContains`, and
   `maxContains` keywords across the JSON and YAML support in VS
   Code. It raises specific error messages now when:

   - `input` isn't defined and `args` doesn't include a JSON input
     argument, or includes more than one JSON input argument.
   - `input` is defined and `args` includes more than one JSON
     input argument.
1. Update the default snippets to enable users to quickly define
   the command without any arguments, with only string arguments,
   and with a JSON input argument.
1. Update the in-editor help and clarify that the command must
   define `input`, a JSON input argument in `args`, or both.

This change updates both the source and composed schemas.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Apr 18, 2024
This change updates the definition of the command arguments from
an array of strings to an array that can include strings and a
single object representing a JSON input argument, as implemented
in PowerShell#385.

The shared definition includes default snippets to simplify adding
arguments to an `args` list, but not validation for the maximum
number of JSON input arguments.

It updates the definition for the schema command and adapter list
command to expect arrays of strings instead of the shared definition
for the command arguments, as those commands never get JSON data
for input and their struct definition in Rust expects an optional
vector of strings, not command arguments.

It updates the definition for the `delete`, `export`, `get`, `set`,
`test`, and `validate` commands to:

1. Make `input` optional, if it was required before, because that
   matches the struct definitions and DSC no longer requires the
   `input` kind when `args` includes a JSON input argument.
1. Add validation using three branches of the `oneOf` keyword,
   to handle mixed support for the `contains`, `minContains`, and
   `maxContains` keywords across the JSON and YAML support in VS
   Code. It raises specific error messages now when:

   - `input` isn't defined and `args` doesn't include a JSON input
     argument, or includes more than one JSON input argument.
   - `input` is defined and `args` includes more than one JSON
     input argument.
1. Update the default snippets to enable users to quickly define
   the command without any arguments, with only string arguments,
   and with a JSON input argument.
1. Update the in-editor help and clarify that the command must
   define `input`, a JSON input argument in `args`, or both.

This change updates both the source and composed schemas.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Apr 18, 2024
This change updates the definition of the command arguments from
an array of strings to an array that can include strings and a
single object representing a JSON input argument, as implemented
in PowerShell#385.

The shared definition includes default snippets to simplify adding
arguments to an `args` list, but not validation for the maximum
number of JSON input arguments.

It updates the definition for the schema command and adapter list
command to expect arrays of strings instead of the shared definition
for the command arguments, as those commands never get JSON data
for input and their struct definition in Rust expects an optional
vector of strings, not command arguments.

It updates the definition for the `delete`, `export`, `get`, `set`,
`test`, and `validate` commands to:

1. Make `input` optional, if it was required before, because that
   matches the struct definitions and DSC no longer requires the
   `input` kind when `args` includes a JSON input argument.
1. Add validation using three branches of the `oneOf` keyword,
   to handle mixed support for the `contains`, `minContains`, and
   `maxContains` keywords across the JSON and YAML support in VS
   Code. It raises specific error messages now when:

   - `input` isn't defined and `args` doesn't include a JSON input
     argument, or includes more than one JSON input argument.
   - `input` is defined and `args` includes more than one JSON
     input argument.
1. Update the default snippets to enable users to quickly define
   the command without any arguments, with only string arguments,
   and with a JSON input argument.
1. Update the in-editor help and clarify that the command must
   define `input`, a JSON input argument in `args`, or both.

This change updates both the source and composed schemas.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc-Impact Schema-Impact Change requires updating a canonical schema for configs or manifests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor input: arg for improved UX/Schema
4 participants