Skip to content

(GH-1563) Enhance the PSScript resources - #1657

Draft
Mikey Lombardi (He/Him) (michaeltlombardi) wants to merge 6 commits into
PowerShell:mainfrom
michaeltlombardi:gh-1563/main/psscript-enhance
Draft

(GH-1563) Enhance the PSScript resources#1657
Mikey Lombardi (He/Him) (michaeltlombardi) wants to merge 6 commits into
PowerShell:mainfrom
michaeltlombardi:gh-1563/main/psscript-enhance

Conversation

@michaeltlombardi

Copy link
Copy Markdown
Collaborator

PR Summary

This change:

  1. Canonicalizes the JSON Schema for the transitional PowerShell script resources by

    • Adding the title and description keywords for every property.
    • Embedding the canonical property definition for _inDesiredState and referencing it instead of redefining it.
    • Using a shared subschema for the script properties that ensures they are strings with a minimum length of 1 and indicates that the expected content is PowerShell script code in with the contentMediaType keyword.
    • Adding constraints to verify the shape of an instance for input (requiring one or more script property) and output (requiring an empty object, an object with output, or an object with _inDesiredState).
    • Allowing users to pass non-integer numbers for input and forbidding passing null
  2. Updates the emitted data for output. Prior to this change, the output property was always an array. This change checks whether the resource emitted any output and:

  • Emits an empty object when the script doesn't emit any data.
  • Emits an object with output defined as a scalar value when the script only emitted a single item.
  • Emits an object with output defined as an array when the script emitted two or more items.
  1. Updates the version of the resource to reflect the changes in schema and behavior.

PR Context

Addresses issues with the schema and implementation of the PSScript resources and fixes #1563.

Prior to this change, the PSScript resource manifests embedded a schema
that minimally defined the `_inDesiredState` canonical property instead
of referencing the canonical schema for that property.

This change updates the embedded schemas to reference the canonical
schema and adds the embedded schema by ID to the `$defs` keyword.
Prior to this change, the embedded schemas in the manifests for the
transitional PowerShell script resources defined the script properties
with a minimal schema allowing the value to be any string or null. They
didn't include any other keywords.

This change:

1. Defines a shared subschema for the script properties, which they now
   reference instead of redefining the constraints for each property.
1. Removed `null` from the allowed types, since defining a script
    property as `null` isn't valid but _not_ specifying the property at
    all _is_ valid.
1. Added the `writeOnly` keyword to the script properties, since
   they're only used for input and not output.
1. Added the `contentMediaType` keyword to the script properties to
   clearly indicate---but not validate---that the value is expected to
   be a PowerShell script.
1. Added the `minLength` keyword to the script properties to ensure that
   an empty string isn't used as a script value.
1. Adds the `title` and `description` keywords to the script properties
   to provide more context for each property in the schema.
Prior to this change, the `input` property for the PSScript resource
schemas defined the valid types with two problems:

1. It allowed `null` as a valid type, which caused failures when the
   resource invoked the scripts.
1. It didn't allow non-integer numbers, erroneously preventing users
   from defining input values like `3.14`, even though `[3.14]` was
   accepted.

This change:

1. Removes `null` from the valid types for `input`.
1. Adds `number` to the valid types for `input`, allowing non-integer
   numbers to be used as input values.
1. Adds the `title` and `description` keywords to the `input` property,
   providing better documentation for users.
Prior to this change, the embedded schemas for the PSScript resources
didn't require any properties to be defined. For input, a user should
always define at least one script property. For output, the resource
should always return one of three value shapes:

1. For `test` operations, the resource should always define the
  `inDesiredState` property.
1. For implemented `get` and `set` operations, the resource should
   define the `output` property when the script emits any output to the
   success stream.
1. When `get` and `set` operations aren't implemented or don't emit any
   output, the resource should return an empty object.

This change adds constraints to the embedded schemas with the `oneOf`
keyword and nested `oneOf`/`anyOf` keywords to enforce these rules.
Prior to this change, the embedded schema for the PSScript resources
didn't define the `title` or `description` keywords.

This change:

1. Adds `title` and `description` keywords to the embedded schema for
   the PSScript resources to provide some documentation for them.
1. Defines the `$schema` keyword explicitly.
Prior to this change, the PSScript resources:

1. Always returned an array of output objects, even if there was only
   one object.
1. Didn't ensure that enums in script output were serialized as
   strings, causing enums to emit as integers, which is likely not what
   the user intended and makes review more difficult.

This change:

1. Ensures that if a script returns a single object, it's serialized as
   that object instead of nested in an array.
1. Adds the `-EnumAsString` parameter to the `ConvertTo-Json` call for
   emitting script output when invoked through PowerShell to ensure
   that enums are serialized as strings. This enhancement doesn't
   affect Windows PowerShell, which doesn't support the `-EnumAsString`
   parameter.
1. Updates the tests to reflect the new behavior of returning a single
   object instead of an array.
1. Fixes the tests to use correct casing for the script properties,
   now that the schema validation checks for those properties.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Microsoft.DSC.Transitional/*Script design

2 participants