Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Proposal for Features functionality and spec. #27
Proposal for Features functionality and spec. #27
Changes from all commits
a9061b9
eed5b79
35382b0
9498ed5
b0357e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an OSI boolean?
I will be faster to filter then regex on license remote files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main issue might be that we would have to get authors to maintain that flag. It might be easier to allow the value to be the name of an OSI approved license OR an URL to a custom license (maybe as an object literal that has an OSI flag for those who want/have to link to their copy of an OSI license).
A license name to show in the UI would also make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scope was related to having something machine readable so that we can quckly filter also in the UI.
How can we quickly filter OSS only elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edgonmsft Do I need to open a new ticket to track this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, lets create a new one. I think its a good idea the we should flesh out a little bit more. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep this declarative for several reasons:
One example was the creation of a user: Since the user is specified in the devcontainer.json as a top-level option, it makes more sense to deal with that in general instead of as part of a feature (the
common
feature?). It might be easiest if we create the user before installing any features if that user does not exist yet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there are two ways of dealing with this. One is the array format which makes it clear to the user in what order things are going to run and the user has total control of that. Not sure I like the idea of things changing from under me if there is an update somewhere upstream. Declaring dependencies could actually be done in
devcontainer.json
instead of as part of the feature itself. If we do that the tool can more easily parallelize, but it complicates the user experience in that now they have to declare the dependencies instead of just using an in order execution.Thoughts? @Chuxel @jkeech @joshspicer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: Dependencies would be declared by features themselves (not in the devcontainer.json), the user should not have to know about these. We don't have dependencies at the moment, but since we have already investigated adding them, I use them as one example to show that having
"features"
in the devcontainer.json determine an execution order will constrain us in the future. (Even more important is the usability argument above.)On the user example: The user is configured with the top-level
"remoteUser"
independently of features. That suggests that we should address it also independently of features. Since features then depend on that user, we would create it before we run the feature install scripts. (Having a feature create the"remoteUser"
doesn't seem to make sense since that user needs to exist also without any features for the configuration to be valid.)Another argument for keeping "features" declarative is that it will make adding auto-detection of features in the future simpler. (If order matters, we need logic to arrange that for the detected features.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I get what you mean but not sure if we should add that to this proposal till we have a better idea of how we want it to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main idea of making this changes is to allow users to control the order of execution, especially since we are not supporting dependencies yet and the changes to the OS applied by features might clash between themselves in unpredictable ways that a user might need to fix in this way.
Thoughts @Chuxel, @bamurtaugh, @jkeech, @joshspicer ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this would work, but is there a way a user could set a property if the order of their features matters? I think the concerns @chrmarti mentions make sense, but I can see the value in what @edgonmsft mentions about controlling the order of execution, so could we default to declarative but allow setting a property that ordering matters, in case users would like to override this declarative default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order that works best will depend on how the features are implemented and could change over time. The user does not have any insight into this. When we add dependencies we want to be in control of the order.
We should ensure the order of installation is always the same (e.g., sort by id), so the outcome is reproducible. If we have any implicit dependencies today, I suggest we take a closer look and find a way to deal with them short term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, I think the user being able to execute things in the order they choose is important for the following reasons:
pip install
andnpm install g
etc that need to run after their respective sdks are installed. This can also be custom user code that needs to explicitly run after certain features.We've already encountered the case where to build some complex images we need to add custom code at different points in time, code that only makes sense for that image and not as a reusable feature.
There are probably more reasons than this, thoughts? @chrmarti, @Chuxel, @joshspicer , @jkeech
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My biggest concern with obscuring the installation order of features (which are implemented as shell scripts with no standardization on how to author them), is that is will significantly hinder the ability of a community member to author their own set of features, and prevent the wider adoption and ecosystem of features.
We have not landed on a simple, complete mechanism for automated versioning/dependency management despite a fair bit of time and effort researching and discussing options. We've investigated building on top of existing package managers like
npm
. Our biggest detractor for such an idea is that we aren't just installing the feature, but the feature with the provided options. Our "blessed" set of features today already function quite differently given provided options - We would need strict authoring rules for the community, as well as a strictly defined set of metadata and listing of dependencies, to make the declarative features object feasible. We are talking about reimplementing a package manager like 'apt'.I think that is too complicated given the problem 'features' is trying to solve (An easy-to-use mechanism to get tool X available in your dev environment, that can auto-update on rebuilds if you want it to)
To the three arguments for implicit ordering:
I think the opposite is true. We provide the user with total control over dependencies (we hope there are few with features, but maybe there are 1 or 2). I think hiding the dependencies will cause more breaks for users.
This argument makes sense, we lose a bit of control over installation order if we aren't able to decide that ourselves.
This is what we are seeking with the array syntax, an option for users to add explicit dependencies. If a dependency changes significantly, it is on the author to create a new major version of the feature and for users to make the conscious decision on how they would like to proceed in their dev containers. I have already implemented a way to pin to a GitHub release or tgz URL, ranges are a next logical step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, we'll also need to take a dependency on the base image, which could be based on a yet another base image, both installing tools. While I think, in a perfect world, we could write tooling to figure everything out for a user, pragmatically I don't think we want to get into that business :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devcontianer-feature.json
that we could validate in some way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be easier to let a feature add options for installing multiple versions (maybe with some additional support for doing that). That would allow the feature to control installation of all requested versions in a single run instead of having it run multiple times. With that approach IntelliSense in the devcontainer.json would likely include a hint if a feature supports multiple versions (e.g., an
"additionalVersions"
property).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't want to block features from receiving multiple values and using them to loop, the idea is that we should also not disallow multiple runs.