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

Initial version of the spec. #33

Merged
merged 10 commits into from
May 24, 2022
Merged

Conversation

edgonmsft
Copy link
Contributor

This is an initial version of the specification.

cc @Chuxel, @chrmarti, @joshspicer

@edgonmsft edgonmsft requested a review from Chuxel May 23, 2022 17:28
Copy link
Member

@Chuxel Chuxel left a comment

Choose a reason for hiding this comment

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

A few comments on review

Comment on lines 15 to 17
A [codespace](https://docs.github.com/en/codespaces/overview) is a development environment that's hosted in the cloud. Codespaces run on a variety of VM-based compute options hosted by GitHub.com, which you can configure from 2 core machines up to 32 core machines. You can connect to your codespaces from the browser or locally using Visual Studio Code.

> **Tip:** If you've already built a codespace and connected to it, be sure to run **Codespaces: Rebuild Container** from the Command Palette (`kbstyle(F1)`) to pick up any changes you make.
Copy link
Member

Choose a reason for hiding this comment

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

The repository properties are now available publicly for Codespaces, correct? Shouldn't we should include those here assuming so. @edgonmsft @bamurtaugh ?

Comment on lines 44 to 49
Some properties are specific to VS Code. Please note that Codespaces supports the VS Code properties.

| Property | Type | Description |
|----------|------|-------------|
| `extensions` | array | An array of extension IDs that specify the extensions that should be installed inside the container when it is created. Defaults to `[]`. |
| `settings` | object | Adds default `settings.json` values into a container/machine specific settings file. Defaults to `{}`. |
Copy link
Member

Choose a reason for hiding this comment

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

We should update these to be under "customizaitons": { "vscode": {} }. It's not clear these are there from this content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the customizations property to the main definition and reorganized supporting-tools.md a little bit for this.

Copy link
Member

Choose a reason for hiding this comment

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

Was customizations added to the files in this PR? I'm trying to search for it, but I can't seem to find it.

Copy link
Member

Choose a reason for hiding this comment

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

Odd, now I see it (maybe it was an old cached version). LGTM!

Comment on lines 21 to 23
The current metadata schema for **development containers** is contained in a `devcontainer.json` file.

The `devcontainer.json` specification contains different configuration options related to the underlying orchestrator format. At the same time this specification leaves space for further development and implementation of other orchestrator mechanisms and file formats.
Copy link
Member

Choose a reason for hiding this comment

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

Given we had an item in this repo that dealt with where devcontainer.json can be found, I'd suggest including that logic here. e.g.,

While this metadata may be provided in different ways, when searching for a devcontainer.json file, products should expect to find a devcontainer.json file in one or more of the following locations (in order of precidence):

  • .devcontainer/devcontainer.json
  • .devcontainer.json
  • .devcontainer/**/devcontainer.json (where ** is a sub-folder)

It is valid that these files may exist in more than one location, so consider providing a mechanism for users to select one when appropriate.

@chrmarti @edgonmsft is the above the right order?

@Chuxel Chuxel requested review from chrmarti and bamurtaugh May 23, 2022 21:40
Copy link
Member

@bamurtaugh bamurtaugh left a comment

Choose a reason for hiding this comment

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

Thanks so much for sharing this, @edgonmsft! Left some comments and suggestions.

In chatting with @Chuxel and now reviewing this PR, I'll close #12 in favor of this PR, as this PR includes the devcontainer.json reference and supporting tools info.

Copy link
Member

@bamurtaugh bamurtaugh 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 incorporating all the feedback!

@edgonmsft
Copy link
Contributor Author

Thanks! Let's merge it and deal with comments in other PR's.

@edgonmsft edgonmsft merged commit b1e8184 into devcontainers:main May 24, 2022
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.

3 participants