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 input: arg for improved UX/Schema #218

Closed
michaeltlombardi opened this issue Oct 4, 2023 · 1 comment · Fixed by #385
Closed

Refactor input: arg for improved UX/Schema #218

michaeltlombardi opened this issue Oct 4, 2023 · 1 comment · Fixed by #385
Assignees
Labels
Issue-Enhancement The issue is a feature or idea
Milestone

Comments

@michaeltlombardi
Copy link
Collaborator

Summary of the new feature / enhancement

In #213 we added functionality for defining an input kind that passes the instance property bag JSON to an executable by replacing a templated argument in the list:

#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
pub enum InputKind {
/// The input replaces arguments with this token in the command.
#[serde(rename = "arg")]
Arg(String),
/// The input is accepted as environmental variables.
#[serde(rename = "env")]
Env,
/// The input is accepted as a JSON object via STDIN.
#[serde(rename = "stdin")]
Stdin,
}

I'm not sure about this change/implementation from the user-facing perspective and the schema - it moves the schema from a set of enum values (in JSON Schema syntax) to a oneOf that is more complicated and harder for users to adjust/grok.

Compare the three available input kinds for the get command:

"get": {
  "executable": "registry",
  "args": ["config", "get"],
  "input": "env"
}
"get": {
  "executable": "registry",
  "args": ["config", "get"],
  "input": "stdin"
}
"get": {
  "executable": "registry",
  "args": ["config", "get", "--input", "{{JSON}}"],
  "input": { "arg": "{{JSON}}" }
}

The latter option requires the author to:

  1. Decide on a string to replace with interpolation.
  2. Define input as an object with the property arg set to that string value instead of as a string.

This both complicates the JSON schema and adds a (small) complexity of choice where I'm not sure it fully matters. It also makes supporting full IntelliSense and contextual help during the authoring process more difficult, as we can't rely on the markdownEnumDescriptions metadata in the same way.

Proposed technical implementation details (optional)

I would propose an enum value like Arg (arg in the JSON) that always replaces a static string argument in the list, instead of allowing an arbitrary string. If that static string argument isn't included, we could either error (the manifest is invalid) or add the JSON as the last argument, after all defined arguments.

We could have that argument string be something like {{instance_json}} to be very unlikely to conflict with any actual arguments.

Then the manifest would look like:

// explicit argument
"set": {
  "executable": "registry",
  "args": ["config", "set", "--input", "{{instance_json}}"],
  "input": "arg"
}
// implicit argument, if supported
"set": {
  "executable": "registry",
  "args": ["config", "set"],
  "input": "arg"
}

From a schema perspective, we can then validate that when input is arg that args includes the static string and not when input is any other value.

Here's a sample of the schema definition, assuming no implicit handling:

allOf:
  - if: { properties: { input: { const: arg } } }
    then:
      properties:
        args:
          contains:  { const: '{{instance_json}}' }
          minContains: 1
          maxContains: 1
    else:
      properties:
        args:
          not: { contains:  { const: '{{instance_json}}' } }

This would make defining the commands more unified across input types, easier to provide IntelliSense/validation/contextual help for, and simplify manifest authoring.

The primary drawback to this that I see is for resources that can't use the static string for the replacement. Are there cases where users will need to specify a custom value for the replacement string? I can't think of any.

This change also allows, but does not require, handling the input JSON argument implicitly as the last argument. I think the implementation change can/should be made regardless of whether we decide to support the implicit argument case.

@michaeltlombardi michaeltlombardi added the Issue-Enhancement The issue is a feature or idea label Oct 4, 2023
@michaeltlombardi
Copy link
Collaborator Author

michaeltlombardi commented Oct 10, 2023

From verbal discussion and investigation:

Proposal

  1. Convert input to an enum list of arg, env, and stdin.
  2. Update the schema for args to support two data types: string and object. String instances work as previously designed. When an instance is an object, the type and name properties are required.
    • type is an enum of verbose, debug, whatif, and input.
    • name is an array of strings representing the arguments to insert.
    • For the input type argument object, the name defines the argument to insert before the instance JSON. For the other types, the name defines the arguments to insert when that context is set by DSC.
  3. Update the schema so that when input is arg, the args array must contain an object where type is input. If input is any other value, args must not contain that object.

Pseudo-schema

# <HOST>/<PREFIX>/<VERSION>/definitions/inputKind.yaml
type:  string
enum: [arg, env, stdin]
# <HOST>/<PREFIX>/<VERSION>/definitions/commandArgs.yaml
type: array
items:
  type:     [string, object]
  required: [type, name]
  properties:
    type:
      title:       Argument Type
      description: Defines which handler DSC should use for the argument.
      type:        string
      enum:       [input, debug, verbose, whatif]
    name:
      title:       Argument Name
      description: Defines the name of one or more options to insert for the argument.
      type:        array
      items:     { type: string }
      minItems:    1
# <HOST>/<PREFIX>/<VERSION>/resource/manifest.set.yaml
type:      object
required: [executable, input]
properties:
  executable:        { $ref: /<PREFIX>/<VERSION>/definitions/commandExecutable.yaml }
  args:              { $ref: /<PREFIX>/<VERSION>/definitions/commandArgs.yaml       }
  input:             { $ref: /<PREFIX>/<VERSION>/definitions/inputKind.yaml         }
  return:            { $ref: /<PREFIX>/<VERSION>/definitions/returnKind.yaml        }
  implementsPretest: { type: boolean, default: false                                }

# if input=arg, require the input-type object, else forbid it.
allOf:
  - if: { properties: { input: { const: arg } } }
    then:
      properties:
        args:
          contains:  { type: object, properties: { type: { const: input } } }
          minContains: 1
          maxContains: 1
    else:
      properties:
        args:
          not: { contains:  { type: object, properties: { type: { const: input } } } }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement The issue is a feature or idea
Projects
Status: Done
2 participants