Skip to content

update property names for JSON camel casing, add inDesiredState property#115

Merged
SteveL-MSFT merged 6 commits intoPowerShell:mainfrom
SteveL-MSFT:indesiredstate
Aug 1, 2023
Merged

update property names for JSON camel casing, add inDesiredState property#115
SteveL-MSFT merged 6 commits intoPowerShell:mainfrom
SteveL-MSFT:indesiredstate

Conversation

@SteveL-MSFT
Copy link
Member

PR Summary

Add .DS_Store to .gitignore so I don't have to keep explicitly avoid including it
Rename JSON property names to be camelCase
Add inDesiredState to TestResult, rename diff_properties to spell out differingProperties and make it mandatory

cc @michaeltlombardi schema impact

PR Context

Fix #108

@SteveL-MSFT SteveL-MSFT requested a review from anmenaga July 29, 2023 00:17
Comment on lines +37 to +44
#[serde(rename = "actualState")]
pub actual_state: Value,
/// Whether the resource was in the desired state.
#[serde(rename = "inDesiredState")]
pub in_desired_state: bool,
/// The properties that were different from the expected state.
pub diff_properties: Option<Vec<String>>,
#[serde(rename = "differingProperties")]
pub diff_properties: Vec<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What effect, if any, does this have on resources that implement their own test operation? I think this change set only applies to how DSC returns the information about a test operation, but better to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although not implemented yet, the expectation is that resources that implement test (with state and/or diff) match the schema that dsc itself returns

Copy link
Collaborator

Choose a reason for hiding this comment

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

And in those cases, if I follow correctly:

  • For State, dsc generates the differingProperties array for the resource
  • For StateAndDiff, the resource is responsible for returning the array of differing properties itself

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is the expectation

Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this change interact with the _inDesiredState well-known property? In the current schema/docs, we have resources that implement test using that property as part of the instance's schema, but this implies that resources which implement test return something other than the data blob for the resource instance.

In other words, given this resource's manifest snippet:

  "test": {
    // defines the command etc
    "return": "state"
  },
  "schema": {
    "embedded": {
      "$schema": "https://json-schema.org/draft/2020-12/schema",
      "type": "object",
      "properties": {
        "foo": { "type": "string" }
      }
    }
  }

The resource should return:

{"actualState": { "foo": "bar" }, "inDesiredState": false}

instead of the prior contract (which would've had _inDesiredState in the instance schema):

{ "foo": "bar", "_inDesiredState": false}

Copy link
Member Author

Choose a reason for hiding this comment

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

So the change here is that resources don't emit _inDesiredState anymore unless they return state then it will look like the TestResult schema.

@SteveL-MSFT SteveL-MSFT merged commit 26856c4 into PowerShell:main Aug 1, 2023
@SteveL-MSFT SteveL-MSFT deleted the indesiredstate branch August 1, 2023 19:42
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.

Add explicit _inDesiredState to test operation

2 participants