Allow using `${remoteUser}` inside of features #525

max06 · 2023-05-17T15:03:00Z

This PR aims to (partially) solve devcontainers/spec/issues/220

The primary goal is to allow features access to the remoteUser-option set in the parent devcontainer.json.

The example container/feature included in this PR (located at src/test/container-features/configs/image-with-local-feature) demonstrates this by using ${remoteUser} in the feature configuration.

I am far away from being fluent in typescript or this codebase. There's very likely a better way to implement this change - in that case I'd be very happy about a hint how to improve this PR.

A PR containing the changes for the feature specs, the schema and the documentation pages will be opened by me right after this PR got merged and verified. Also I'll be very happy to publish my first feature making use of that change as well.

max06 · 2023-05-17T15:03:37Z

@microsoft-github-policy-service agree

max06 · 2023-07-25T22:04:21Z

In case there's no notification for marking a PR as ready: It's ready 🎉

joshspicer · 2023-07-25T22:06:32Z

src/test/configs/image-with-local-feature/.devcontainer/devcontainer.json

@@ -0,0 +1,8 @@
+{


Thanks for adding the test. It doesn't look like it's being executed anywhere?

Test added for image-with-local-feature. I'm afraid the resulting image doesn't contain any useful details about the mount from the local feature (or if the var substitution happened). I might have to test this using cli up instead if this test is not sufficient.

That would be great - we have plenty of examples of using up with Features.

Perhaps you could use the new ${remoteUser} variable in a Feature lifecycle hook to write that in a file (and assert it as you expect)?

If you decide to go that route, the tests in this file may be more helpful.

Gotcha! I thought about using it in a mount directive, starting the container and checking it with docker inspect if the mount is used properly. But your idea is better. I'll adapt it tomorrow.

Test adapted. Feature now writes ${remoteUser} into /tmp/variable-substitution.testMarker and asserts the content.

max06 · 2023-08-08T20:02:14Z

Anyone alive? 😄 @chrmarti maybe?

max06 · 2023-09-03T18:17:20Z

Folks... I'd really appreciate some activity in here.

Can I do something to speed this up a bit?

...atures/configs/image-with-local-feature/.devcontainer/test-feature/devcontainer-feature.json

src/test/container-features/lifecycleHooks.test.ts

src/spec-common/variableSubstitution.ts

joshspicer

Apologies for the delay in reviewing. Thank for adding the test, that's what I had in mind! I've asked @chrmarti to take a quick peek as he's very familiar with the variable substitution code. Thanks for following up again

This change allows using `${remoteUser}` inside of devcontainer-feature.json similar to the exising ${devcontainerId}.

max06 · 2023-09-05T21:00:36Z

We have 3 tests now:

Tests if substitution works inside of a features json
Tests if substitution works inside of the main devcontainer.json
Tests if a specified mount containing a substitution gets resolved and used properly.

Happy to add more! Thanks for your time!

joshspicer · 2023-09-08T15:32:32Z

Merging this in we'll also want to update the documentation on variables on the spec (https://github.com/devcontainers/spec/blob/main/docs/specs/devcontainerjson-reference.md#variables-in-devcontainerjson and https://github.com/devcontainers/spec/blob/main/docs/specs/devcontainer-features.md) and website.

chrmarti

Thanks for the PR! I have left a few questions.

chrmarti · 2023-09-12T06:35:39Z

src/spec-node/configContainer.ts

@@ -97,6 +97,7 @@ export async function readDevContainerConfigFile(cliHost: CLIHost, workspace: Wo
 		containerWorkspaceFolder: workspaceConfig.workspaceFolder,
 		configFile,
 		env: cliHost.env,
+		remoteUser: updated.remoteUser


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.

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)

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.

max06 force-pushed the add-substitute-remoteuser branch 2 times, most recently from 7446e48 to a56419c Compare July 25, 2023 21:53

max06 changed the title ~~Add substitution for remoteUser~~ Allow using ${remoteUser} inside of features Jul 25, 2023

max06 marked this pull request as ready for review July 25, 2023 22:03

max06 requested a review from a team as a code owner July 25, 2023 22:03

joshspicer reviewed Jul 25, 2023

View reviewed changes

joshspicer reviewed Sep 5, 2023

View reviewed changes

...atures/configs/image-with-local-feature/.devcontainer/test-feature/devcontainer-feature.json Show resolved Hide resolved

joshspicer reviewed Sep 5, 2023

View reviewed changes

src/test/container-features/lifecycleHooks.test.ts Outdated Show resolved Hide resolved

joshspicer reviewed Sep 5, 2023

View reviewed changes

src/spec-common/variableSubstitution.ts Show resolved Hide resolved

joshspicer reviewed Sep 5, 2023

View reviewed changes

max06 added 4 commits September 5, 2023 19:57

feat: ✨ Support ${remoteUser} in feature definitions

c47cb27

This change allows using `${remoteUser}` inside of devcontainer-feature.json similar to the exising ${devcontainerId}.

test: Add test for image-with-local-feature

6bdccac

test: Improve test to use lifecycleHooks

cf32d27

Add additional tests

1afe943

max06 force-pushed the add-substitute-remoteuser branch from a3c58da to 1afe943 Compare September 5, 2023 20:54

Typos in comments

92ca28c

joshspicer approved these changes Sep 8, 2023

View reviewed changes

joshspicer requested a review from chrmarti September 8, 2023 15:32

chrmarti requested changes Sep 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using `${remoteUser}` inside of features #525

Allow using `${remoteUser}` inside of features #525

max06 commented May 17, 2023 •

edited

Loading

max06 commented May 17, 2023

max06 commented Jul 25, 2023

joshspicer Jul 25, 2023

max06 Jul 25, 2023 •

edited

Loading

joshspicer Jul 25, 2023

joshspicer Jul 25, 2023

max06 Jul 26, 2023

max06 Jul 26, 2023

max06 commented Aug 8, 2023

max06 commented Sep 3, 2023

joshspicer left a comment

max06 commented Sep 5, 2023

joshspicer commented Sep 8, 2023

chrmarti left a comment

chrmarti Sep 12, 2023

joshspicer Sep 12, 2023

max06 Sep 12, 2023

Allow using ${remoteUser} inside of features #525

Are you sure you want to change the base?

Allow using ${remoteUser} inside of features #525

Conversation

max06 commented May 17, 2023 • edited Loading

max06 commented May 17, 2023

max06 commented Jul 25, 2023

Choose a reason for hiding this comment

max06 Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max06 commented Aug 8, 2023

max06 commented Sep 3, 2023

joshspicer left a comment

Choose a reason for hiding this comment

max06 commented Sep 5, 2023

joshspicer commented Sep 8, 2023

chrmarti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Allow using `${remoteUser}` inside of features #525

Allow using `${remoteUser}` inside of features #525

max06 commented May 17, 2023 •

edited

Loading

max06 Jul 25, 2023 •

edited

Loading