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

Updating features proposal after latest discussions. #48

Merged
merged 7 commits into from
Jun 29, 2022

Conversation

edgonmsft
Copy link
Contributor

@edgonmsft edgonmsft commented Jun 21, 2022

proposals/devcontainer-features.md Outdated Show resolved Hide resolved
proposals/devcontainer-features.md Outdated Show resolved Hide resolved
@joshspicer
Copy link
Member

does it make sense to add an example to installation order, like how I did in the issue? Or too in the weeds? #43 (comment)

Co-authored-by: Josh Spicer <josh@joshspicer.com>
Co-authored-by: Samruddhi Khandale <skhandale@microsoft.com>
proposals/devcontainer-features.md Outdated Show resolved Hide resolved
proposals/devcontainer-features.md Outdated Show resolved Hide resolved
proposals/devcontainer-features.md Outdated Show resolved Hide resolved
proposals/devcontainer-features.md Show resolved Hide resolved
proposals/devcontainer-features.md Outdated Show resolved Hide resolved
proposals/devcontainer-features.md Show resolved Hide resolved
proposals/devcontainer-features.md Show resolved Hide resolved
proposals/devcontainer-features.md Show resolved Hide resolved
proposals/devcontainer-features.md Outdated Show resolved Hide resolved
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.

Can we move install.app and install.file to a separate proposal? I think this requires more discussion. (Previously discussed in #27 (review).)

proposals/devcontainer-features.md Outdated Show resolved Hide resolved
proposals/devcontainer-features.md Outdated Show resolved Hide resolved
Co-authored-by: Brigit Murtaugh <brigit.murtaugh@microsoft.com>
@Chuxel
Copy link
Member

Chuxel commented Jun 27, 2022

Already mentioned elsewhere, but adding here for tracking. I think we should call out customizations being in the schema and the expected behavior for merging properties under it. (For example, each "namespace" under customizations is treated as a separate set of properties. For each set, the object is parsed and leaf values are replaced while leaf arrays are a union).

I wouldn't want the spec to have to update as new tools come in, for example. Right now there's "extensions" and "settings" at the wrong level per #1.

@Chuxel
Copy link
Member

Chuxel commented Jun 29, 2022

@edgonmsft @chrmarti @joshspicer @craiglpeters One other thought. I think at one point we'd discussed the idea that a feature could declare the operating systems and architectures it supports. Given the number of hiccups we've seen with arm64 installs with existing features and PRs like microsoft/vscode-dev-containers#1513 (for Gentoo) driving things into different base OS distros, I think it may be a good idea to include this in the spec from the beginning.

Didn't we have that included at one point?

We could have normalized architecture values and then support for /etc/os-release detection based on ID or ID_LIKE for distribution. We can also detect the version in a similar way. Most of the features have some code in them to detect and block based on these values. Moving it into the core spec just makes things easier for feature authors and we can provide UX hints if a feature isn't expected to work given the current environment.

e.g., something like this might work

"architectures": [ "x86_64", "arm64"],
"os": "linux",
"version": {
   "debian": ["10", "11"],
   "ubuntu": ["20.04", "18.04"],
   "alpine": true
}

...where "true" means any version.

@edgonmsft
Copy link
Contributor Author

Made changes for the comments here. We can use this issue to track changes for architecture.

#58

@edgonmsft
Copy link
Contributor Author

Also: #59

@edgonmsft edgonmsft merged commit 7ff8d1f into devcontainers:main Jun 29, 2022
Copy link

@113ll3 113ll3 left a comment

Choose a reason for hiding this comment

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

Review

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.

Feature installation order
7 participants