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

Dependson #175

Merged
merged 5 commits into from
Sep 1, 2023
Merged

Dependson #175

merged 5 commits into from
Sep 1, 2023

Conversation

SteveL-MSFT
Copy link
Member

PR Summary

Implement dependsOn support for configurations:

  • refactor config mod.rs to use new function to get resource invocation order
  • order is based on dependencies first then the resource
  • detect invalid dependencies and have strict validation of syntax
  • detect circular dependencies
  • added unit tests

PR Context

#45

@@ -71,6 +71,7 @@ pub struct Resource {
/// A friendly name for the resource instance
pub name: String, // friendly unique instance name
#[serde(rename = "dependsOn", skip_serializing_if = "Option::is_none")]
#[schemars(regex(pattern = r"^\[resourceId\('[a-zA-Z0-9\.]+/[a-zA-Z0-9]+','[a-zA-Z0-9 ]+'\)]$"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this allow for an optional space between the type name and the instance name? Does it have to use single quotes?

Thinking about examples like:

resources:
- name: current user registry
  type: Microsoft.Windows/Registry
  properties:
    keyPath: HKCU\example
    _ensure: Present
  dependsOn:
  # Current implementation
  - "[resourceId('DSC/AssertionGroup','my assertions')]"
  # With outer quotes as single quotes
  - '[resourceId("DSC/AssertionGroup","my assertions")]'
  # With a space between the names
  - "[resourceId('DSC/AssertionGroup', 'my assertions')]"

The regex for the type name also doesn't match our current regex - I'm okay with adjusting either of them:

$id: <HOST>/<PREFIX>/<VERSION>/definitions/resourceType.yaml
title: DSC Resource fully qualified type name
description: |
The namespaced name of the DSC Resource, using the syntax:
owner[.group][.area]/name
For example:
- Microsoft.SqlServer/Database
- Microsoft.SqlServer.Database/User
type: string
pattern: ^\w+(\.\w+){0,2}\/\w+$

We also don't have a regex currently applied to instance names, we allow arbitrary strings. Should that be more limited?

Copy link
Member Author

Choose a reason for hiding this comment

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

Best I can tell from ARM, whitespace is allowed. I'll adjust the regex.

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 updated whitespace, dangling questions are:

  1. Can the regex also support using double quotes instead of single quotes?
  2. Can the instance names be arbitrary strings or are they limited to alphanumeric characters and spaces? If they're limited, I can adjust the docs and schemas instead of allowing arbitrary strings. Right now, this regex makes the following instance names invalid:
- name: Security.PublicAccess.Disable
- name: Security/PublicAccess/Disable
- name: Security - Disable public access
- name: 'Security: Disable public access'

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 best I can tell looking at the ARM docs it's single quote only because it's in JSON which would get wrapped by double quotes. Don't see any specific rules on characters, but the examples are all alphanumeric with spaces allowed, so I think that's ok unless you can find something more specific to what ARM allows

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 can update this as a separate PR as needed

@SteveL-MSFT SteveL-MSFT merged commit 4a2a391 into PowerShell:main Sep 1, 2023
4 checks passed
@SteveL-MSFT SteveL-MSFT deleted the dependson branch September 1, 2023 18:41
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 1, 2023
Prior to this change, the schema for the `dependsOn` keyword used the PSDSC
syntax and the schema for instance names allowed arbitrary strings.

This change:

- Updates the schema and documentation for instance names to clarify that
  they must be non-empty strings containing only letters, numbers, and
  spaces.
- Updates the schema and documentation for the `dependsOn` property to
  use the new `resourceId()` function syntax as added in PowerShell#175.

For now, this change does not define new reference documentation for the
configuration functions. When more configuration functions are added, we
can define a separate set of reference documentation for those functions.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 1, 2023
This change regenerates the canonical schemas based on the changes to
the schemas for the `dependsOn` keyword and instance names.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 5, 2023
Prior to this change, the schema for the `dependsOn` keyword used the PSDSC
syntax and the schema for instance names allowed arbitrary strings.

This change:

- Updates the schema and documentation for instance names to clarify that
  they must be non-empty strings containing only letters, numbers, and
  spaces.
- Updates the schema and documentation for the `dependsOn` property to
  use the new `resourceId()` function syntax as added in PowerShell#175.

For now, this change does not define new reference documentation for the
configuration functions. When more configuration functions are added, we
can define a separate set of reference documentation for those functions.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Sep 5, 2023
This change regenerates the canonical schemas based on the changes to
the schemas for the `dependsOn` keyword and instance names.
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.

3 participants