Skip to content

Add JSON schema for Development Container Template Metadata#350

Open
benblank wants to merge 3 commits intodevcontainers:mainfrom
benblank:templates-schema
Open

Add JSON schema for Development Container Template Metadata#350
benblank wants to merge 3 commits intodevcontainers:mainfrom
benblank:templates-schema

Conversation

@benblank
Copy link
Copy Markdown

@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
Copy Markdown
Author

@microsoft-github-policy-service agree

Copy link
Copy Markdown

@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 :)

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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. 🤷‍♂️

Comment thread schemas/devContainerTemplate.schema.json
Copy link
Copy Markdown
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

Comment thread schemas/devContainerTemplate.schema.json
@Coinsintegrity
Copy link
Copy Markdown

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.)
"type": "object"
},
"FeatureOption": {
"anyOf": [
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

@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.

10 participants