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.

Sign up for GitHub

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

[proposal] Update the devcontainer.json schema to be able to represent a 'mergedConfiguration' #206

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

[proposal] Update the devcontainer.json schema to be able to represent a 'mergedConfiguration' #206

Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c1224a4
goal of consolidate-merged-lifecycle-hook-schema
joshspicer Feb 28, 2023
3a3e262
init consolidate-merged-lifecycle-hook-schema
joshspicer Feb 28, 2023
4ab251e
proposal
joshspicer Feb 28, 2023
9e26125
Update consolidate-merged-lifecycle-hook-schema.md
joshspicer Feb 28, 2023
3a62b33
cover all properties
joshspicer Mar 9, 2023
a778863
indent;
joshspicer Mar 9, 2023
aa048eb
proofreading
joshspicer Mar 9, 2023
465381d
customizations array variant
joshspicer Mar 16, 2023
56b107c
Apply suggestions from code review
joshspicer Mar 16, 2023
1dc72c9
italics
joshspicer Mar 16, 2023
452b918
code review edits
joshspicer Jun 27, 2023
b19219f
clarify $entrypoints
joshspicer Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions proposals/consolidate-merged-lifecycle-hook-schema.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Goal

Expand the lifecycle hook `devcontainer.json` schema to make it possible to represent a merged configuration within a `devcontainer.json`. This will allow a configuration comprised of lifecycle hooks comprised of [Feature contributed lifecycle scripts](./features-contribute-lifecycle-scripts.md) or otherwise merged onto the image to not need a separate 'mergedConfiguration' schema to represent the config.

## Motivation

As seen below, the current generated 'mergedConfiguration' returned by the `read-configuration` CLI command does not return a `devcontainer.json` as outlined in this repo's specification, but rather an array of hooks. [(source)](https://github.com/devcontainers/cli/pull/390#issuecomment-1430190326)

![img-1](https://user-images.githubusercontent.com/23246594/218825633-cf037d97-db05-4d0d-9157-66287cd47073.png)

## Proposal

Update the schema of lifecycle hooks to mirror the merged configuration illustrated above.

Propose to change the interface to:

```typescript

export type LifecycleCommand = string | string[]
export type LifecycleCommandParallel = { [key: string]: LifecycleCommand };

interface DevContainerConfig {
// ...other devcontainer.json properties...
onCreateCommand?: LifecycleCommand | LifecycleCommandParallel | LifecycleCommandParallel[]
Copy link

Choose a reason for hiding this comment

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

I think that this means that a devcontainer-feature.json could specify something like:

{"onCreateCommand": [{
  "foo": "script/foo",
  "$origin": "foo"
}, {
  "foo": ["date > /tmp/last-ran"]
}]

Should it be considered an error if a devcontainer-feature.json specifies a list of objects? Should tools ignore an $origin property when reading from a json file directly (versus from image metadata)?

Copy link

Choose a reason for hiding this comment

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

Oh, this change specifically affects the schema for devcontainer.json, I see. I hadn't realized that devcontainer-feature.json has a distinct json schema.

Copy link
Member Author

@joshspicer joshspicer Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I don't think we'd want to update the devcontainer-feature.json schema, which is very close to a devcontainer.json but distinct. The goal of the $ properties as members of the devcontainer.json is so that a command such as devcontainer read-configuration can output a json object that is also a devcontainer.json. That's not the case today without these changes.

Copy link

@avidal avidal Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The goal of the $ properties as members of the devcontainer.json is so that a command such as devcontainer read-configuration can output a json object that is also a devcontainer.json. That's not the case today without these changes.

I've been thinking more about this, and I'm not sure that's a good thing to have in the spec itself...it seems like an implementation detail for the specific tool in question. The way some of the other comments read indicates that it'd be "valid" in a devcontainer.json but not actually used (re: a comment above about $entrypoints)...which kind of implies that the output of read-configuration is not actually a devcontainer.json.

That is, it seems like a "proper" implementation would use this schema to encode/decode metadata stored in the image label, and use it internally as the result of merging all of the various metadata sources, but it's not actually valid devcontainer.json config. Effectively splitting out merged metadata into a separate schema and leaving the existing devcontainer.json and devcontainer-feature.json alone.

The way I've been thinking about it lately is that a *.json contains two sets of data: config and metadata. I parse either devcontainer.json or devcontainer-feature.json into Config or FeatureConfig respectively, plus a Metadata entry. I parse the image label into a MergedMetadata entry. Internally I store the config and the merged metadata to drive the build process. I use the merged metadata to render into an image label. I print the config and merged metadata during debug output as two separate entities.

edit: The main reason I'm bringing this up is because it seems to me that this change may require significant explanation in the docs to reduce confusion for implementors vs declaring it as a distinct schema and indicating that it represents the results of the "merge logic table" and should be used as the entry for the devcontainer.metadata image label.

updateContentCommand?: LifecycleCommand | LifecycleCommandParallel | LifecycleCommandParallel[]
postCreateCommand?: LifecycleCommand | LifecycleCommandParallel | LifecycleCommandParallel[]
postStartCommand?: LifecycleCommand | LifecycleCommandParallel | LifecycleCommandParallel[]
postAttachCommand?: LifecycleCommand | LifecycleCommandParallel | LifecycleCommandParallel[]
}
```

The new addition is the final unioned value `LifecycleCommandParallel[]`. Every existing lifecycle command and merged command can be represented within that type.

Using this property, the 'mergedConfiguration' returned by `read-configuration` can directly return a `devcontainer.json` without needing non-spec properties (ie: plural `postCreateCommands`).