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

Direct resource set no longer requires test #197

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

SteveL-MSFT
Copy link
Member

PR Summary

  • Better error message when using set and no input is provided
  • Make dsc resource set no longer tries a test before
  • Rename preTest to implementsPretest to make it more clear it's the resource implementing it or not

PR Context

Some usability issues identified when @mgreenegit was writing a bash resource example

@mgreenegit
Copy link
Member

mgreenegit commented Sep 16, 2023

On the behavior of implementsPreTest, if we detect that Test is not implemented, then we know PreTest is not implemented.

@SteveL-MSFT
Copy link
Member Author

@mgreenegit the implementsPretest is an optimization where if true (default is false) then don't even try to execute test (if implemented or synthesize it if not). implementsPretest = true means "don't bother checking if already in desired state, call set anyways"

@michaeltlombardi
Copy link
Collaborator

@SteveL-MSFT - I think I mostly agree with this, with the exception that it should be possible to indicate that I want the test operation to be carried out before set. This is important for resources that rely on DSC's synthetic testing imo, and mostly for interactive/script usage. Without this option, I have to do something like:

$Settings = @{ foo = 'bar' } | ConvertTo-Json
$Result = $Settings | dsc resource test --resource My/Resource
if (-not $Result.inDesiredState) {
  $Result = $Settings | dsc resource set --resource My/Resource
  if ($LastExitCode -ne 0) {
    # Handle error
  }
}
# Continue processing

With a --preTest flag (or whatever), I could do:

$Settings = @{ foo = 'bar' } | ConvertTo-Json
$Result = $Settings | dsc resource set --resource My/Resource --preTest
if ($LastExitCode -ne 0) {
  # Handle error
}
# Continue processing

Regarding the schema update/rename, can we mark preTest as a deprecated value that has been renamed, but still handle it in this release? I can adjust the published schemas to that effect (we can define a deprecation message in the VS Code schema to indicate they should rename the value). We might want to consider this sort of model and how we want to go about schema modifications now, while usage and impact are still low to feel out what the best practice will be in the long run.

@johlju
Copy link

johlju commented Sep 18, 2023

I agree with @michaeltlombardi that it should be possible for test to run before the set, but wouldn't it also be preferred to indicate what properties that are not in desired state too from test so that can be passed into set? That way set knows what properties it actually need to set. Please correct me if that is already possible (not been able to keep up to speed with DSC v3 just yet), and it might be out of scope here. 🙂

@mgreenegit
Copy link
Member

@mgreenegit the implementsPretest is an optimization where if true (default is false) then don't even try to execute test (if implemented or synthesize it if not). implementsPretest = true means "don't bother checking if already in desired state, call set anyways"

This makes sense. My suggestion is, if the manifest does not contain a Test section, the correct value for implementsPreTest is always going to be True, but we default to False which creates a "broken by default" edge case.

@SteveL-MSFT
Copy link
Member Author

@mgreenegit Set and Test are intended to be independent. I would expect any resource that implements Test to internally use it for Set, but maybe they decided not to do that. In this case, the logic is:

  • Is implementsPretest == true? If so, DSC just calls set directly
  • if not set or false, then does the resource implement test, if so, call that and if it fails (not in desired state), call set
  • if test is not implemented, perform a get and compare to desired state input (synthetic test), if nto in desired state, call set

I don't think we can assume that because test is implemented, it means set internally does a test first.

I think adding a --pretest switch to set operation makes sense. However, I think that should be a separate PR after this one is merged as this issue is hitting folks.

@SteveL-MSFT SteveL-MSFT merged commit b3f44c5 into PowerShell:main Sep 19, 2023
4 checks passed
@SteveL-MSFT SteveL-MSFT deleted the resource-set branch September 19, 2023 20:22
@SteveL-MSFT
Copy link
Member Author

@michaeltlombardi schema changes during alpha are inevitable. Maybe we just need to call them out in the changelog?

michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 27, 2023
This change updates the schemas to reflect the renaming of the
`set.preTest` setting in the resource manifest to the more
readable and semantically correct `set.implementsPretest` to
reflect the change in PowerShell#197.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 27, 2023
This change updates the schemas to reflect the renaming of the
`set.preTest` setting in the resource manifest to the more
readable and semantically correct `set.implementsPretest` to
reflect the change in PowerShell#197.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 27, 2023
This change updates the reference documentation and the changelog
to reflect the changes that were included in the `alpha.3` release:

- PowerShell#197
- PowerShell#198
- PowerShell#199
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 29, 2023
This change updates the reference documentation and the changelog
to reflect the changes that were included in the `alpha.3` release:

- PowerShell#197
- PowerShell#198
- PowerShell#199
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 29, 2023
This change updates the schemas to reflect the renaming of the
`set.preTest` setting in the resource manifest to the more
readable and semantically correct `set.implementsPretest` to
reflect the change in PowerShell#197.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 29, 2023
This change updates the reference documentation and the changelog
to reflect the changes that were included in the `alpha.3` release:

- PowerShell#197
- PowerShell#198
- PowerShell#199
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 30, 2023
This change updates the reference documentation and the changelog
to reflect the changes that were included in the `alpha.3` release:

- PowerShell#197
- PowerShell#198
- PowerShell#199
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 30, 2023
This change updates the reference documentation and the changelog
to reflect the changes that were included in the `alpha.3` release:

- PowerShell#197
- PowerShell#198
- PowerShell#199
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants