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

Jump to bottom

Allow using ${remoteUser} inside of features #525

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Allow using ${remoteUser} inside of features #525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c47cb27
feat: :sparkles: Support ${remoteUser} in feature definitions
max06 Jul 25, 2023
6bdccac
test: Add test for `image-with-local-feature`
max06 Jul 25, 2023
cf32d27
test: Improve test to use lifecycleHooks
max06 Jul 26, 2023
1afe943
Add additional tests
max06 Sep 5, 2023
92ca28c
Typos in comments
max06 Sep 5, 2023
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
4 changes: 4 additions & 0 deletions src/spec-common/variableSubstitution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export interface SubstitutionContext {
localWorkspaceFolder?: string;
containerWorkspaceFolder?: string;
env: NodeJS.ProcessEnv;
remoteUser?: string;
}

export function substitute<T extends object>(context: SubstitutionContext, value: T): T {
Expand Down Expand Up @@ -109,6 +110,9 @@ function replaceWithContext(isWindows: boolean, context: SubstitutionContext, ma
case 'containerWorkspaceFolderBasename':
return context.containerWorkspaceFolder !== undefined ? path.posix.basename(context.containerWorkspaceFolder) : match;

case 'remoteUser':
joshspicer marked this conversation as resolved.
Show resolved Hide resolved
return context.remoteUser !== undefined ? context.remoteUser : match;

default:
return match;
}
Expand Down
1 change: 1 addition & 0 deletions src/spec-node/configContainer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export async function readDevContainerConfigFile(cliHost: CLIHost, workspace: Wo
containerWorkspaceFolder: workspaceConfig.workspaceFolder,
configFile,
env: cliHost.env,
remoteUser: updated.remoteUser
Copy link
Contributor

Choose a reason for hiding this comment

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

updated.remoteUser could use other variables like ${localEnv:DEV_CONTAINER_USER}, would that work correctly? E.g.:

{
	"name": "Demo container",
 	"image": "mcr.microsoft.com/devcontainers/base:bullseye",
 	"features": {
 		"./test-feature": {}
 	},
 	"remoteUser": "${localEnv:DEV_CONTAINER_USER}",
 	"postCreateCommand": "echo ${remoteUser} > /tmp/container.variable-substitution.testMarker"
 }

Maybe there needs to be a second pass for substituting ${remoteUser} which can get its value from the devcontainer.json?

Another case to consider is when the remoteUser is set in the base image's metadata (e.g., mcr.microsoft.com/devcontainers/base:bullseye), in that case updated.remoteUser would be undefined.

What if the remoteUser is not set, but the dev container will run with the container user? Should ${remoteUser} be replaced with the container user? We also do that for the _REMOTE_USER variable.

Copy link
Member

Choose a reason for hiding this comment

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

Another case to consider is when the remoteUser is set in the base image's metadata (e.g., mcr.microsoft.com/devcontainers/base:bullseye), in that case updated.remoteUser would be undefined.

There's a lot of tests here that could help assert the behavior when remoteUser is set on as image metadata: https://github.com/devcontainers/cli/blob/main/src/test/imageMetadata.test.ts

(If you're curious what metadata is attached to an image, this is how I generally take a peek at it on the fly:)

$ docker pull mcr.microsoft.com/devcontainers/base:bullseye
bullseye: Pulling from devcontainers/base
Digest: sha256:cd9bbfbe4d380278d08fe22c39c5801667424a059c59368819a89ad22ff6936f
Status: Image is up to date for mcr.microsoft.com/devcontainers/base:bullseye
mcr.microsoft.com/devcontainers/base:bullseye

$ export IMAGE="mcr.microsoft.com/devcontainers/base:bullseye"

$ docker image inspect "$IMAGE" | jq '.[0].Config.Labels."devcontainer.metadata" | fromjson'
[
  {
    "id": "ghcr.io/devcontainers/features/common-utils:2"
  },
  {
    "id": "ghcr.io/devcontainers/features/git:1"
  },
  {
    "remoteUser": "vscode"
  }
]

What if the remoteUser is not set, but the dev container will run with the container user? Should ${remoteUser} be replaced with the container user? We also do that for the _REMOTE_USER variable.

That's a good point that I did not think of, perhaps it should align with those variable (info here https://containers.dev/implementors/features/#user-env-var)

Copy link
Author

Choose a reason for hiding this comment

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

Hello @chrmarti, hello @joshspicer,

I understood the assignment. Configuration options can come from many different sources. Relying on the devcontainer.json to contain them is not the proper way to go.

Usually I'm pretty good at reading foreign code and getting the gist of it. At least enough to be able to provide smaller bugfixes and features. This project here... doesn't work very well with my brain. I'm having a hard time following along the different workflows without seeing the red string showing a common structure. Most likely the lack of experience on my side, I'm more ops than dev.

I'll give it another shot next weekend, unless one of you already knows where to use the screwdriver. Feel free to do whatever needs to be done, I definitely won't complain.

}, value);
const config: DevContainerConfig = substitute0(updated);
if (typeof config.workspaceFolder === 'string') {
Expand Down
9 changes: 9 additions & 0 deletions src/test/container-features/configs/image-with-local-feature/.devcontainer/devcontainer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "Demo container",
"image": "mcr.microsoft.com/devcontainers/base:bullseye",
"features": {
"./test-feature": {}
},
"remoteUser": "vscode",
"postCreateCommand": "echo ${remoteUser} > /tmp/container.variable-substitution.testMarker"
}
15 changes: 15 additions & 0 deletions ...res/configs/image-with-local-feature/.devcontainer/test-feature/devcontainer-feature.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"$schema": "https://raw.githubusercontent.com/devcontainers/spec/main/schemas/devContainerFeature.schema.json",
"id": "test-feature",
"name": "test-feature",
"version": "0.0.1",
"description": "Testing a feature",
"mounts": [
{
"source": "test-${devcontainerId}",
"target": "/home/${remoteUser}",
joshspicer marked this conversation as resolved.
Show resolved Hide resolved
"type": "volume"
}
],
"postCreateCommand": "/usr/bin/whoami && echo ${remoteUser} > /tmp/feature.variable-substitution.testMarker"
}
3 changes: 3 additions & 0 deletions ...container-features/configs/image-with-local-feature/.devcontainer/test-feature/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/bin/bash

echo "I am the install script and I do nothing."
49 changes: 49 additions & 0 deletions src/test/container-features/lifecycleHooks.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,55 @@ describe('Feature lifecycle hooks', function () {
});
});

describe('lifecycle-hooks-with-variable-substitution', () => {
describe('devcontainer up', () => {
let containerId: string | null = null;
let containerUpStandardError: string;
const testFolder = `${__dirname}/configs/image-with-local-feature`;

before(async () => {
await shellExec(`rm -f ${testFolder}/*.testMarker`, undefined, undefined, true);
const res = await devContainerUp(cli, testFolder, { 'logLevel': 'trace' });
containerId = res.containerId;
containerUpStandardError = res.stderr;
});

after(async () => {
await devContainerDown({ containerId });
await shellExec(`rm -f ${testFolder}/*.testMarker`, undefined, undefined, true);
});

it('executes lifecycle hooks with variable substitution', async () => {
// substitution in feature
const res1 = await shellExec(`${cli} exec --workspace-folder ${testFolder} cat /tmp/feature.variable-substitution.testMarker`);
assert.strictEqual(res1.error, null);

const outputOfExecCommand1 = res1.stdout;
console.log(outputOfExecCommand1);

assert.strictEqual(outputOfExecCommand1, 'vscode\n');
assert.match(containerUpStandardError, /Running the postCreateCommand from Feature '.\/test-feature/);

// substitution in main devcontainer.json
const res2 = await shellExec(`${cli} exec --workspace-folder ${testFolder} cat /tmp/container.variable-substitution.testMarker`);
assert.strictEqual(res2.error, null);

const outputOfExecCommand2 = res2.stdout;
console.log(outputOfExecCommand2);

assert.strictEqual(outputOfExecCommand2, 'vscode\n');
assert.match(containerUpStandardError, /Running the postCreateCommand from devcontainer.json/);

// Check if substituted mount path is in use
const res3 = await shellExec(`docker inspect ${containerId} --format '{{json .Mounts}}'`);
assert.strictEqual(res3.error, null);

const mounts = JSON.parse(res3.stdout);
assert.exists(mounts.find((item: { Type: string; Destination: string }) => item.Type === 'volume' && item.Destination === '/home/vscode'));
});
});
});

describe('lifecycle-hooks-advanced', () => {

describe(`devcontainer up`, () => {
Expand Down
Loading