Repository navigation
refactoring Jason/Mikey RFC - #1576
gaelcolas (gaelcolas) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds RFC 0001 and supporting documentation describing a contract for PowerShell class-based PSDSC resources to participate in DSC v3 semantics via the PowerShell adapter.
Changes:
- Introduces RFC0001 specifying class shape, method signatures, and adapter expectations for class-based PSDSC resources in DSC v3.
- Adds a conceptual guide with examples for authoring class-based PSDSC resources compatible with PSDSC v1/v2 and DSC v3.
- Updates the resources overview to link to the new authoring guide.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| RFC: RFC0001 # WG will set the number after submission | ||
| Author: jahelmic # <@GitHubUserName> | ||
| Sponsor: michaeltlombardi # <@GitHubUserName> | ||
| Status: Draft # <Draft | Experimental | Accepted | Final> | ||
| SupercededBy: null # <Superceding RFC Number> |
| static [string] InstanceJsonSchema() { | ||
| return ( | ||
| @{ | ||
| '$schema' = 'https://json-schema.org/draft/2020-12/schema' | ||
| type = 'object' | ||
| required = @('name') | ||
| properties = @{ | ||
| name = @{ type = 'string' } | ||
| version = @{ type = 'string' } | ||
| _exist = @{ | ||
| '$ref' = 'https://aka.ms/dsc/schemas/v3/resource/properties/exist.json' | ||
| } | ||
| } | ||
| } | ConvertTo-Json -Depth 10 -Compress |
| if ($null -eq $filteringInstance) { | ||
| throw 'Invalid operation' | ||
| } |
| | Set | `[void] Set()` | `static [void] Set(...)`, `static [<ResourceClass>] Set(...)`, or `static [System.Tuple[<ResourceClass>, String[]]] Set(...)` | | ||
| | Delete | not available | `static [void] Delete([<ResourceClass>]$instance)` | | ||
| | Export | not available | `static [<ResourceClass>[]] Export()` and/or `static [<ResourceClass>[]] Export([<ResourceClass>]$filteringInstance)` | | ||
| | Schema | MOF or class properties | `static [string] InstanceJsonSchema()` | |
There was a problem hiding this comment.
Wouldn't we just want the JSONSchema to be in the adapted manifest?
There was a problem hiding this comment.
I would partially agree here because I think they can be both. I think we should keep the possibility of doing validation through the adapter also. And if this static method is found, it can be used. Makes it an optional escape hatch, but you can also declare constraints that attributes cannot express.
There was a problem hiding this comment.
I'd like to move away from the adapter doing validation because it means you won't get good intellisense in the future
| | --- | --- | --- | | ||
| | Get | `[<ResourceClass>] Get()` | `static [<ResourceClass>] Get([<ResourceClass>]$instance)` | | ||
| | Test | `[bool] Test()` | `static [System.Tuple[bool, <ResourceClass>]] Test(...)` or `static [System.Tuple[bool, <ResourceClass>, String[]]] Test(...)` | | ||
| | Set | `[void] Set()` | `static [void] Set(...)`, `static [<ResourceClass>] Set(...)`, or `static [System.Tuple[<ResourceClass>, String[]]] Set(...)` | |
There was a problem hiding this comment.
How would --what-if work here for both set and delete?
There was a problem hiding this comment.
Just two dedicated methods?
static [System.Tuple[SoftwarePackage, string[]]] WhatIf([SoftwarePackage] $instance) # set preview
static [System.Tuple[SoftwarePackage, string[]]] DeleteWhatIf([SoftwarePackage] $instance) # delete previewI guess it would be addable in an adapted resource manifest, but regardless, the adapter would need a change.
There was a problem hiding this comment.
Could also add a check for a boolean final parameter, like
static [System.Tuple[SoftwarePackage, string[]]] Set(
[SoftwarePackage] $instance,
[bool ]$whatIf
) {
# Implementation
}
static [System.Tuple[SoftwarePackage, string[]]] Set(
[SoftwarePackage] $instance
) {
[SoftwarePackage]::Set($instance, $false)
}
static [System.Tuple[SoftwarePackage, string[]]] Delete(
[SoftwarePackage] $instance,
[bool] $whatIf
) {
# Implementation
}
static [System.Tuple[SoftwarePackage, string[]]] Delete(
[SoftwarePackage] $instance
) {
[SoftwarePackage]::Delete($instance, $false)
}| | Test | `[bool] Test()` | `static [System.Tuple[bool, <ResourceClass>]] Test(...)` or `static [System.Tuple[bool, <ResourceClass>, String[]]] Test(...)` | | ||
| | Set | `[void] Set()` | `static [void] Set(...)`, `static [<ResourceClass>] Set(...)`, or `static [System.Tuple[<ResourceClass>, String[]]] Set(...)` | | ||
| | Delete | not available | `static [void] Delete([<ResourceClass>]$instance)` | | ||
| | Export | not available | `static [<ResourceClass>[]] Export()` and/or `static [<ResourceClass>[]] Export([<ResourceClass>]$filteringInstance)` | |
There was a problem hiding this comment.
There's a PR out to add support for separate schema for export filtering. Reusing the same PS class for export would mean that key still needs to be populated even though it's not needed for export. Alternatively, we could say that the code uses the same type, but any constraints is purely defined in the JSONSchema.
For Jason Helmick (@theJasonHelmick) and Mikey Lombardi (He/Him) (@michaeltlombardi), I've tried to refactor the RFC into something lighter, easier to chew, by moving (maybe aggressively) all related explanations to
docs/in order to keep the RFC concise on the proposal.All parts link back to those docs.
Let me know how that look.