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

Add JSON schema for Development Container Template Metadata #350

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benblank
Copy link

@benblank benblank commented Dec 14, 2023

I've made an attempt at creating the JSON schema for Template metadata which I requested in #349.

Some notes:

  • It is patterned after devContainerFeature.schema.json, on the theory that Template metadata has more in common with Feature metadata than it does Development Container metadata.
  • The spec doesn't indicate which properties are required, so:
    • For Templates, between how they are described to be used and the fact that id, name, and version are required for Feature metadata, I've assumed those same three properties are required for Template metadata.
    • For options, based on how they are described to be used, I've assumed that all properties are required (with the exception of enum and proposals; see the points below).
  • Though the spec indicates that an option's default property may only be a string, I've assumed that it should instead be a boolean when the type property is "boolean".
  • With regards to the enum and proposals properties of options:
    • I've assumed that neither enum nor proposals are relevant to boolean options, so have forbidden them.
    • I've assumed that enum cannot be an empty array, on the theory that because free-form values are not allowed, it would make it impossible to set a valid value for that option.
      • I have not made the same assumption regarding proposals, as free-form values are allowed.
  • I haven't placed any restrictions (e.g. via patterns) on Template IDs, Template versions, or option IDs, as the Feature schema also doesn't. However, these could easily be added.

Apologies for the verbose description; I'm only getting started with Development Containers and want to make sure that any mistakes I've made are easy to spot. 😅

@benblank benblank requested a review from a team as a code owner December 14, 2023 06:48
@benblank
Copy link
Author

@microsoft-github-policy-service agree

Copy link

@TateGunning TateGunning left a comment

Choose a reason for hiding this comment

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

Looks fine :)

Choose a reason for hiding this comment

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

Thankful, please advise

},
"required": [
"id",
"name",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is name required? It might be, but we don't have it written down it seems. It is optional for features.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm. It isn't marked as required in devContainerFeature.schema.json now that I look at it, but it is on the implementors page. I'm not yet familiar enough with the spec to know which is accurate, but I suppose the other should be updated to match. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

@joshspicer Should we make name required in the features schema? Would a feature work if it had no name currently? If not, we would know that making it required in the schema won't break anyone. :)

Copy link
Member

@joshspicer joshspicer Jan 30, 2024

Choose a reason for hiding this comment

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

In the tooling name is not required. For example when we build the READMEs in devcontainers/action it will use id if name is not available.

That said, we should probably just make it required since it's useful as a display name in our tooling and it's provided by default in our devcontainers/feature-starter template.

One thing to note - if we add name as required in the schema for Features, this validation code when publishing with the devcontainers/action will fail if the Feature is missing name.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems safer to update the spec to make it optional then. We could break existing features as you point out.

One advantage of not making it required is that it is easier to write one-off features for prototyping or testing. We would still want features going into the index to have good names for the UI. 🤷‍♂️

Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks great overall, I have left a few comments for consideration / discussion.

We should then also update https://github.com/devcontainers/action to use this schema similarly to how it uses the schema for features.

We could add the features and templates schemas to VS Code like we do for the main schema here: https://github.com/microsoft/vscode/blob/75b1639461b53dc51380a61532aabdd502c4c718/extensions/configuration-editing/package.json#L142

schemas/devContainerTemplate.schema.json Show resolved Hide resolved
@Coinsintegrity
Copy link

Coinsintegrity commented Dec 21, 2023 via email

- The schema for features has been updated to use "oneOf" semantics instead of "anyOf" semantics for option types. (This should have no practical effect, but clarifies intent.)
- For templates, the schema no longer requires a string option to have "proposals" when "enum" is absent. (This matches the behavior of the features schema.)
@113ll3
Copy link

113ll3 commented Jan 30, 2024 via email

@@ -211,7 +216,7 @@
"type": "object"
},
"FeatureOption": {
"anyOf": [

Choose a reason for hiding this comment

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

Copy link

@TateGunning TateGunning left a comment

Choose a reason for hiding this comment

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

where is this one at @chrmarti

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.