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.

Sign up for GitHub

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

Merged
merged 33 commits into from
Jun 28, 2022
Merged

Changes for Features V2 #6

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
eebacc5
First changes for features
edgonmsft May 9, 2022
e825692
Missing reference.
edgonmsft May 9, 2022
1047e01
Fix test
edgonmsft May 9, 2022
40aa793
Fixes.
edgonmsft May 9, 2022
83afa1b
Run config
edgonmsft May 9, 2022
39cff54
Comment out test
edgonmsft May 9, 2022
12d39a2
Fixes for absolute paths
edgonmsft May 10, 2022
e769e66
Comments on pr.
edgonmsft May 10, 2022
b35174c
Fix for github features.
edgonmsft May 12, 2022
6b59032
Merge remote-tracking branch 'origin/main' into edgon/features-1
edgonmsft May 23, 2022
1e9a5bb
Fixes for locally cacched features.
edgonmsft May 23, 2022
9b98c33
Merge remote-tracking branch 'origin' into edgon/features-1
edgonmsft May 24, 2022
5923ef7
Fix old features.
edgonmsft May 24, 2022
23e87ff
Other fixes.
edgonmsft May 24, 2022
9bce15f
Some comments and formating.
edgonmsft May 24, 2022
91edf38
Test fix
edgonmsft May 24, 2022
971bd56
Fix
edgonmsft May 24, 2022
4e0fc7e
Fix test.
edgonmsft May 24, 2022
4842ec5
Refactor changes and read devcontainer collections.
edgonmsft May 25, 2022
ebbbc4e
Fix test.
edgonmsft May 25, 2022
26f3f35
Fix test.
edgonmsft May 25, 2022
11fb36c
Fix test 3
edgonmsft May 25, 2022
934ae4a
fix test 4
edgonmsft May 25, 2022
63a5ed2
fix test 5
edgonmsft May 25, 2022
b304f90
Comments from PR.
edgonmsft May 25, 2022
5e29707
Fix cleanup
edgonmsft May 25, 2022
ec88721
Remove prefix for environment variables in features v2
edgonmsft Jun 2, 2022
267ef67
support fetching v2 features from github repo (#48)
joshspicer Jun 3, 2022
df7575b
clean up tracing and formatting
joshspicer Jun 8, 2022
1b42a63
Merge remote-tracking branch 'origin/main' into edgon/features-1
edgonmsft Jun 14, 2022
68e68b1
Changes from the last spec proposal changes.
edgonmsft Jun 24, 2022
39a1b22
Comments and fixes from PR.
edgonmsft Jun 27, 2022
e92dd24
Unit test fixes.
edgonmsft Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
// Use IntelliSense to learn about possible attributes.
// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
"configurations": [
{
"type": "node",
"request": "launch",
"name": "Launch cli - up",
"program": "${workspaceFolder}/src/spec-node/devContainersSpecCLI.ts",
"cwd": "${workspaceFolder}",
"args": ["up", "--workspace-folder", "../features-playground", "--log-level", "debug", ],
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can add in a very simple feature set/collection as one of the tests, and use that here? That way we can always F5 into something useful.

"console": "integratedTerminal",
}
]
}
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@
"vinyl-fs": "^3.0.3"
},
"dependencies": {
"@types/ncp": "^2.0.5",
edgonmsft marked this conversation as resolved.
Show resolved Hide resolved
"ncp": "^2.0.0",
"follow-redirects": "^1.14.8",
"js-yaml": "^4.1.0",
"jsonc-parser": "^3.0.0",
Expand Down
7 changes: 6 additions & 1 deletion src/spec-common/injectHeadless.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,18 @@ export type DevContainerConfigCommand = 'initializeCommand' | 'onCreateCommand'

const defaultWaitFor: DevContainerConfigCommand = 'updateContentCommand';

export interface DevContainerFeature{
edgonmsft marked this conversation as resolved.
Show resolved Hide resolved
id: string;
options: boolean | string | Record<string, boolean | string | undefined>;
}

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>>;
Copy link
Contributor

@chrmarti chrmarti Jun 3, 2022 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor

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

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.

onCreateCommand?: string | string[];
updateContentCommand?: string | string[];
postCreateCommand?: string | string[];
Expand Down
11 changes: 8 additions & 3 deletions src/spec-configuration/configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ export interface HostRequirements {
storage?: string;
}

export interface DevContainerFeature{
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
export interface DevContainerFeature{
export interface DevContainerFeature {

id: string;
options: boolean | string | Record<string, boolean | string | undefined>;
}

export interface DevContainerFromImageConfig {
configFilePath: URI;
image: string;
Expand Down Expand Up @@ -54,7 +59,7 @@ export interface DevContainerFromImageConfig {
remoteUser?: string;
updateRemoteUserUID?: boolean;
userEnvProbe?: UserEnvProbe;
features?: Record<string, string | boolean | Record<string, string | boolean>>;
features?: DevContainerFeature[] | Record<string, string | boolean | Record<string, string | boolean>>;
hostRequirements?: HostRequirements;
}

Expand Down Expand Up @@ -85,7 +90,7 @@ export type DevContainerFromDockerfileConfig = {
remoteUser?: string;
updateRemoteUserUID?: boolean;
userEnvProbe?: UserEnvProbe;
features?: Record<string, string | boolean | Record<string, string | boolean>>;
features?: DevContainerFeature[] | Record<string, string | boolean | Record<string, string | boolean>>;
hostRequirements?: HostRequirements;
} & (
{
Expand Down Expand Up @@ -132,7 +137,7 @@ export interface DevContainerFromDockerComposeConfig {
remoteUser?: string;
updateRemoteUserUID?: boolean;
userEnvProbe?: UserEnvProbe;
features?: Record<string, string | boolean | Record<string, string | boolean>>;
features?: DevContainerFeature[] | Record<string, string | boolean | Record<string, string | boolean>>;
hostRequirements?: HostRequirements;
}

Expand Down
Loading