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.
[proposal] Composite Features #208
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] Composite Features #208
Changes from all commits
149035b
9347bd2
ce50238
5ad1c0d
7b82815
e3cd09d
5419937
f441a0b
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.
Does the correct installation order depend on each of the install script's requirements?
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'd want to peek at each Feature's
installsAfter
, i think. That was the primary reason that I didn't want to (for now) enable nesting composite Features. This spec does NOT let you nest one composite feature in another, making this process easier to resolve (we can "flatten" the installation from composite Features into one install order.This is something to discuss and finalize in the spec, but we could have installation follow
installsAfter
order with precedence within a composite Feature first, and then some algorithm for ordering composite Features based on that? Perhaps the other way around?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.
Since we are already thinking about composite features depending on each other: Wouldn't it be simpler to avoid the additional artifact type of a "composite feature" and instead allow features to have hard dependencies in addition to the soft dependencies supported with "installAfter"? We already compute the dependency graph including cycle detection for "installAfter". (Other package management systems also seem to have only one package type that can carry code/binaries and dependencies. Do we have an example that keeps these apart?)
We could add "dependencies" to the feature metadata as an object like "features" in the devcontainer.json, so installation order is decided by the same dependency graph algorithm. A dependency would implicitly go into the "installAfter" list of the feature depending on it.
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.
What if a different version is configured in the devcontainer.json?
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 think this brings us back to similar discussion over at #44.
Maybe it would be helpful to define behavior for installing a Feature more than once. I think a goal of Features should be that they are idempotent - perhaps this would be a good time to also define some "Feature best practices" to guide authors toward the behavior we want Features to exhibit?
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.
Agreed. We already have that when you use a base image with a feature installed and you then install that feature again in a second devcontainer.json.
If the version and options objects are equal, we can optimize by running the feature only once.
Maybe a detail: The install order algorithm currently uses the feature ids to sort features that do not require a specific install order to arrive at a stable install order. With the same feature being allowed multiple times in the same install pass, we will need to include also the feature version and options object in this sorting to ensure a stable install order.
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.
Does "remaining install steps will be skipped" mean, the remaining features listed in the array will be skipped?
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 will clarify this - I meant that each Feature will have a
detect
that will cause the further installation and merging of dev container config for that Feature. Remaining Features in the array will still be executed (and their detect phase run, too).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.
What are the examples we want to cover with this? Would this run after all previous features have detected & installed, immediately before this feature's install script would run?
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.
How are these merged when the same feature is installed by the devcontainer.json?
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.
Similar to #208 (comment)
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.
Do we have good examples for that?
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.
This could be omitted in the first pass if we find it to be unnecessary or without justification yet. I'll leave for discussion
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 like that we are discussing various options. We should aim for as much simplicity as we can while still covering the important cases we have. It will be easier to later add to a simple design than a complex one.