-
Notifications
You must be signed in to change notification settings - Fork 238
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
Changes for Features V2 #6
Conversation
}); | ||
return result; | ||
} | ||
|
||
// Used for features version 2 |
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.
Would it perhaps make sense to try and divide the internalVersion==1
and internalVersion==2
implementations into separate files?
This file was already a bit massive :)
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've been thinking about it, I was thinking of extracting functions for the things that are different and we can move those to separate files.
return featuresConfig; | ||
} | ||
|
||
const getUniqueFeatureId = (id: string, srcInfo: SourceInformation) => `${id}-${getSourceInfoString(srcInfo)}`; | ||
export function parseFeatureIdentifier(output: Log, userFeature: DevContainerFeature) : FeatureSet | undefined { |
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.
You have this returning a FeatureSet now. Note that the vscode extension side of things uses this to add diagnostics (the little squigglys) underneath the features in vscode, so that will need to be fixed too.
Seems like the role of this function changed quite a bit? Why is 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.
It was one of the complicated parts of supporting both versions at the same time. I also wanted to split the parsing from fetching the features themselves. I think we can fix the diagnostics if we don't get a featureSet back?
return; | ||
} | ||
|
||
if(!localFeatures) |
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'd suggest running "prettier" on this file from vscode to format
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 a preferred configuration for prettier? If I run it it changes everything so I would do it in another pr.
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.
Good question. I'm looking now and looks like I just use the built-in vscode formatter. Christof might have better answer?
@@ -192,17 +212,48 @@ USER $_DEV_CONTAINERS_IMAGE_USER | |||
|
|||
export function getFeatureLayers(featuresConfig: FeaturesConfig) { | |||
let result = ''; | |||
const folders = (featuresConfig.featureSets || []).map(x => getSourceInfoString(x.sourceInformation)); | |||
|
|||
// Features version 1 FIX |
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 can cause issues since it breaks the ordering of how features are executed, adding features v1 first and then features v2.
There is no easy way to fix it since some feature sets might need to be called together at the same time and that already breaks the convention.
One option would be to execute v2 first and ensure that we include a cache of v2 at the same time but its still an issue for user written 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.
Features shouldn't depend on the order they are executed. Those that do we should take a look at.
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 only one so far is the Jupyter feature that requires some sort of Python installed.
if (fe.infoString) | ||
{ | ||
fe.internalVersion = '2'; | ||
const envPath = cliHost.path.join(fe.infoString, 'devcontainer-features.env'); |
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.
Should we move 'devcontainer-features.env'
out to be a global variable at the top of this file?
There are two identical definitions of cli/src/spec-configuration/configuration.ts Lines 29 to 32 in b304f90
cli/src/spec-common/injectHeadless.ts Lines 108 to 111 in b304f90
|
src/spec-common/injectHeadless.ts
Outdated
export interface CommonDevContainerConfig { | ||
configFilePath?: URI; | ||
remoteEnv?: Record<string, string | null>; | ||
forwardPorts?: (number | string)[]; | ||
portsAttributes?: Record<string, PortAttributes>; | ||
otherPortsAttributes?: PortAttributes; | ||
features?: Record<string, string | boolean | Record<string, string | boolean>>; | ||
features?: DevContainerFeature[] | Record<string, string | boolean | Record<string, string | boolean>>; |
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.
Note that we don't want to support the array variant. Please see https://github.com/devcontainers/spec/pull/27/files#r869018039.
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.
Made a comment on that thread.
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 have shared some thoughts as well
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.
Dropping my thoughts here since the spec PR has already been merged for a couple weeks. I strongly believe we need to support the array syntax, and I would go a step further and say that we should only document the array syntax going forward, leaving the object syntax in place only for backwards compatibility (object can be easily mapped to the array form at runtime in a deterministic way).
There are several scenarios we've encountered from users who need to better control feature execution order as a consumer of the feature in a way that the feature author wouldn't know about in advance. Most of these involve custom logic that the consumer wants to run after a feature is installed, but are things the user wants baked into the image itself and not later on in a lifecycle hook. A few of the examples:
- Running a tool from an earlier feature to install other tools in a later feature
- A user wants to install a global npm package (
npm install -g
) after Node/npm are installed - A user wants to install some Python packages through pip after installing python/pip
- A user wants to install an Azure CLI extension after installing the Azure CLI
- A user wants to install a GH CLI extension after installing the GH CLI
- A user wants to install a global npm package (
I understand the argument that the tooling should be able to figure out the right order if dependencies are defined, but I don't think we have a solution for dependency declaration within each feature that is robust enough to handle all of the cases. Ultimately, giving users the ability to force a specific ordering puts them in control and allows them to make adjustments as needed instead of being at the mercy of the tooling to figure out the perfect install order.
For most users, they won't need to worry about the installation order of different features, and in that case, both syntaxes work just fine. But in the case where users do need the control for the ordering, the array syntax makes that possible, whereas the object syntax does not.
* support fetching v2 features from github repo * update test
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.
As the automatic ordering (and accompanying overrides) get more complicated, I think it would be great to add in tests that run against the final Dockerfile
and assert the order of statements, etc..
Can leave that for another PR 🚀
@samruddhikhandale have you had a chance to play around with these changes for the images? |
Yes! devcontainers/images#31 (Didn't make changes to codespaces due to need of multiple excecution of features) |
This includes the changes outlined in the following proposal:
devcontainers/spec#27
Still has a couple of things in progress but can be commented on.
Fixes other issues:
https://github.com/github/codespaces/issues/7101