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

draft of command based resources #2

Closed
wants to merge 1 commit into from
Closed

draft of command based resources #2

wants to merge 1 commit into from

Conversation

SteveL-MSFT
Copy link
Member

command based resource for DSCv3 proposal


### Module based resources

Existing PowerShell class and script function resources packaged as a module will need the addition of a manifest file.
Copy link
Member Author

Choose a reason for hiding this comment

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

We may change this to NOT require a new JSON config file as most legacy modules authors will likely not make a change. Instead, we can have a meta property as part of the input JSON like _powershell that specifies powershell.exe or pwsh.exe to execute.

@@ -0,0 +1,377 @@
# Command based resources for DSC v3

Configuration is based on a declarative idempotent model that allows users to get, set, and test settings values.
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to also expose API and not just cmdline tool

Copy link
Collaborator

@gaelcolas gaelcolas Jan 20, 2023

Choose a reason for hiding this comment

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

  1. You start by describing "Configuration" that is the PoC you've built, shouldn't talk about the problem first?
  2. Why an API is needed? If the tool/utility accepts JSON input, is an API needed and why? I guess showing the purpose first would help understand the why.
    Creating an API for it means you already think about some use cases or integration you haven't described.

@@ -0,0 +1,377 @@
# Command based resources for DSC v3

Configuration is based on a declarative idempotent model that allows users to get, set, and test settings values.
Copy link
Collaborator

@gaelcolas gaelcolas Jan 20, 2023

Choose a reason for hiding this comment

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

  1. You start by describing "Configuration" that is the PoC you've built, shouldn't talk about the problem first?
  2. Why an API is needed? If the tool/utility accepts JSON input, is an API needed and why? I guess showing the purpose first would help understand the why.
    Creating an API for it means you already think about some use cases or integration you haven't described.

Comment on lines +8 to +9
However, we still need to support the existing resources that are written in PowerShell including
class based and script functions based resources.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
However, we still need to support the existing resources that are written in PowerShell including
class based and script functions based resources.

I'd say you don't need this for the purpose of the tool.
You want this to be supported, so that motivation should be recorded in the context (Why such tool would be useful? What's the business case?)

Go, Rust, etc...

Communication to and from the resource will be via STDIN or arguments for input and STDOUT for output along
with using the exit code to indicate success or failure and STDERR for informative messages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

All output should be serialised structured data (JSON), with schema.

### Resource discovery

Any "command" (which can be a script requiring a host runtime) that particpates would have a command manifest file with the name
"<command>.dscresource.json" that would be found within the `PATH` environment variable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the plug-in model I described above, the discovery should be handled by that plug-in.
Having config resource manifest in the PATH would not make sense for DSC resources.
If the plug-ins implement a discovery method/action, then config can ask the plugins for their available resources.
The DSC Resource plug-in would do a Get-DscResource (no need for JSON manifest anymore, the plug-in would return the JSON manifest in some ways, from the DSC resources it found).

Any "command" (which can be a script requiring a host runtime) that particpates would have a command manifest file with the name
"<command>.dscresource.json" that would be found within the `PATH` environment variable.

PowerShell based resources can have an optional `.dscresource.json` file to indicate if the resource should be hosted by powershell.exe or pwsh.exe (see below).
Copy link
Collaborator

Choose a reason for hiding this comment

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

PowerShell and pwsh should tell which DSC Resource is available, and the resource modules should say which one works and which one doesn't with a given version of PowerShell.
The plug-in specifies which version of WPS or pwsh it supports or not (i.e. pwsh_psdscv3 plug-in works with PS 7.3.1, so Get-DscResource in PS 7.3.1 won't show DSC Resources that are only supported in WPS such as SharePointDsc, because its Module manifest says so).
If a DSC Resource module "says" it supports both, then where to run it should be defined when declaring the configuration, similar to the "which user to run this config as" (PSDscRunAsCredential)...

## Resource input

The JSON input to the resource will be just the `settings` for that resource.
See https://microsoft.ghe.com/AzureCore-Compute/PowerShellTeam-Docs/pull/36/files for a higher level configuration example and the
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have access to that link, and I don't really understand :(


The exit code must be 0 for success.
Legacy PowerShell resources don't set the exit code so will default to 0.
Optional JSON output via STDOUT provides additional information and may be used by the orchestrator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Optional JSON output via STDOUT provides additional information and may be used by the orchestrator.
Optional JSON output via STDOUT provides additional information and may be used by the `config` utility.

Changed for consistency.
Orchestrator is another loaded term.


### Failure output

The exit code may be non-zero to indicate failure.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exit code should be important for the config utility, not what it runs.
What it runs should return JSON.

Comment on lines +296 to +297
> [TODO] Should define a common schema for error messages so an orchestrator can more easily parse
> and report the errors to the user.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
> [TODO] Should define a common schema for error messages so an orchestrator can more easily parse
> and report the errors to the user.
> [TODO] Should define a common schema for error messages so that the `config` utility, or higher level tools, can more easily parse
> and report the errors to the user.


For consistency, the following members are common to all configuration resource settings:

`_ensure`: Used to specify if that setting should be `present` or `absent`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

In some rare occasions this does not make sense for some resources...
For instance, some resources are not installable/removable, they will always be present, but maybe with different values...


Since STDERR is used for other types of output (verbose/debug messages), the top level member indicates failure.

> [TODO] Should define a common schema for error messages so an orchestrator can more easily parse
Copy link
Member Author

Choose a reason for hiding this comment

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

Should have a _reason property that contains ErrorId and Message per property if there is a failure, like Access Denied, or Not Found

Copy link
Collaborator

Choose a reason for hiding this comment

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

What if the Reason is not an error?
What if there's more than 1 reason per property? (User is not in group xyz and is also disabled)?

"2": "Invalid setting value",
"3": "Unknown setting"
},
"schema": "https://schemas.microsoft.com/configuration/myResource/20220621/schema.json"
Copy link
Member Author

Choose a reason for hiding this comment

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

Enable getting schema dynamically via the executable

"version": "1.2.3"
}
],
"exitCodes": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Allow way to execute command to get these, probably as JSON output making it easier to keep it all in the code rather than keeping code and manifest in sync

@SteveL-MSFT
Copy link
Member Author

Closing this. Will draft a new spec based on changes and learnings we've had.

@SteveL-MSFT SteveL-MSFT closed this Apr 4, 2023
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.

None yet

2 participants