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

Terminate process tree on ctrl+c #213

Merged
merged 9 commits into from Oct 4, 2023

Conversation

SteveL-MSFT
Copy link
Member

PR Summary

  • move test_group_resource under tools where all test specific code should go
  • created dsctest exe to be used with Test/Sleep resource for manually validating ctrl+c. Future test resources should be subcommands of this exe.
  • fn to discover process tree under the current dsc process and terminate each one
  • fix build.ps1 to correctly replace debug and release paths on non-Windows
  • add InputKind = Arg to allow passing the input JSON as an argument. See Test/Sleep resource on example usage.
  • added a few info traces to help see what was happening during validation

PR Context

Terminate subprocesses upon ctrl+c

@SteveL-MSFT SteveL-MSFT changed the title Terminate process Terminate process tree on ctrl+c Oct 2, 2023
Comment on lines +53 to +55
/// The input replaces arguments with this token in the command.
#[serde(rename = "arg")]
Arg(String),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this change/implementation from the user-facing perspective and the schema - it moves the schema from a set of enum values (in JSON Schema syntax) to a oneOf that is more complicated and harder for users to adjust/grok.

I would propose an enum value like ReplaceArg (replaceArg in the JSON) that always replaces a static string argument in the list, instead of allowing an arbitrary string.

We could have that argument be something like {{instance_json}} to be very unlikely to conflict with any actual arguments.

Then the manifest would look like:

"set": {
  "executable": "foo",
  "args": ["config", "set", "--input", "{{instance_json}}"],
  "input": "replaceArg"
}

From a schema perspective, we can then validate that when input is replaceArg that args includes the static string and not when input is any other value.

Are there cases where users will need to specify a custom value for the replacement string?

allOf:
  - if: { properties: { input: { const: replaceArg } } }
    then:
      properties:
        args:
          contains:  { const: '{{instance_json}}' }
          minContains: 1
          maxContains: 1
    else:
      properties:
        args:
          not: { contains:  { const: '{{instance_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.

@michaeltlombardi please open a new issue to discuss

tools/dsctest/src/sleep.rs Outdated Show resolved Hide resolved
dsc/src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@miroman9364 miroman9364 left a comment

Choose a reason for hiding this comment

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

Code-wise, this looks good to me. If the arg processing needs to change, it can be done in an update.

@SteveL-MSFT SteveL-MSFT added this pull request to the merge queue Oct 4, 2023
Merged via the queue into PowerShell:main with commit 5b92b58 Oct 4, 2023
4 checks passed
@SteveL-MSFT SteveL-MSFT deleted the terminate-process branch October 4, 2023 18:04
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Oct 5, 2023
This change updates the project changelog, reference documentation, and
schemas for the recent PRs merged for the project:

- PowerShell#206
- PowerShell#208
- PowerShell#211
- PowerShell#213
- PowerShell#215
- PowerShell#216
- PowerShell#217

The updates include:

- Documenting the new `_exist` property, replacing `_ensure` for
  resources. This documentation update includes the schema definition,
  but doesn't regenerate the schema, which will be handled separately.
- Documenting the new `completer` command.
- Documenting the new `--input` and `--input-file` global options.
- Adding a deprecation notice to the `_ensure` documentation.
- Adding entries for all user-impacting changes to the changelog.
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