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

remove support to retrieve schema from URL #457

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

SteveL-MSFT
Copy link
Member

PR Summary

The support for schema URL was initially intended for intellisense support. However, we can still resolve resources for intellisense using existing other means to get the json schema. We should not allow reaching out to internet during configuration deployment.

PR Context

@SteveL-MSFT SteveL-MSFT added Doc-Impact Schema-Impact Change requires updating a canonical schema for configs or manifests labels Jun 12, 2024
@michaeltlombardi
Copy link
Collaborator

What about resolving references in the embedded-or-command-returned schema? Will we require bundling for resource JSON schemas?

@SteveL-MSFT
Copy link
Member Author

@michaeltlombardi yeah, forgot about referenced schema. We should discuss this further.

@SteveL-MSFT
Copy link
Member Author

@michaeltlombardi actually, I just remembered that the JsonSchema crate has its own built in way to retrieve web referenced schema so this PR is literally only removing it as an option the manifest so I think we're ok

@michaeltlombardi
Copy link
Collaborator

I mean that, from a guidance perspective, if the concern is either offline-functionality or no-web-calls-on-apply, we should advise authors on whether and when to bundle their schemas.

@SteveL-MSFT
Copy link
Member Author

@michaeltlombardi with this change, we require schema to be shipped with the resource whether it's embedded or via command. We should recommend that those schema should not use web references as that would mean their resource won't work on some systems that limit outbound connections.

@michaeltlombardi
Copy link
Collaborator

@SteveL-MSFT One dangling question then:

In our docs, we recommend that users reference the well-known properties instead of redefining them, e.g.

"_rebootRequested": {
  "$ref": "https://raw.githubusercontent.com/PowerShell/DSC/main/schemas/2024/04/resource/properties/rebootRequested.json"
}

This ensures that users don't accidentally mis-defined the properties. I think we can tell users either that they can reference DSC schemas and subschemas, or that they need to bundle these into their full definition. I prefer the former, for simplicity - but some bundling tools will also find and pull those references in for them, too.

@SteveL-MSFT
Copy link
Member Author

@SteveL-MSFT One dangling question then:

In our docs, we recommend that users reference the well-known properties instead of redefining them, e.g.

"_rebootRequested": {
  "$ref": "https://raw.githubusercontent.com/PowerShell/DSC/main/schemas/2024/04/resource/properties/rebootRequested.json"
}

This ensures that users don't accidentally mis-defined the properties. I think we can tell users either that they can reference DSC schemas and subschemas, or that they need to bundle these into their full definition. I prefer the former, for simplicity - but some bundling tools will also find and pull those references in for them, too.

Referencing schema should still be recommended and this change shouldn't affect that. This should only affect having the url type of schema in the manifest.

@SteveL-MSFT SteveL-MSFT added this pull request to the merge queue Jul 3, 2024
Merged via the queue into PowerShell:main with commit b0e6ed9 Jul 3, 2024
4 checks passed
@SteveL-MSFT SteveL-MSFT deleted the remove-schema-url branch July 3, 2024 00:59
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Aug 22, 2024
This change removes the `url` option for defining the resource
instance schema in a resource manifest, as that option was
removed in PowerShell#457.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Aug 22, 2024
This change removes the `url` option for defining the resource
instance schema in a resource manifest, as that option was
removed in PowerShell#457.
michaeltlombardi added a commit to michaeltlombardi/DSC that referenced this pull request Aug 22, 2024
This change removes the `url` option for defining the resource
instance schema in a resource manifest, as that option was
removed in PowerShell#457.
anmenaga pushed a commit to anmenaga/DSC-New that referenced this pull request Aug 24, 2024
This change removes the `url` option for defining the resource
instance schema in a resource manifest, as that option was
removed in PowerShell#457.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc-Impact Schema-Impact Change requires updating a canonical schema for configs or manifests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants