devcontainers / cli

A reference implementation for the specification that can create and configure a dev container from a devcontainer.json.
https://containers.dev
MIT License
1.51k stars 215 forks source link

inconsistency in handling of environment variables defined within features #609

Closed rokroskar closed 1 year ago

rokroskar commented 1 year ago

First of all thanks for this project! It's a great tool and I'm currently working on integrating it more tightly into our workflows. That said, I'm not sure the issue I'm having is appropriate for this repo or if it would be better suited for the spec or features repos - feel free to move it if needed.

The problem I'm seeing is an inconsistency in the container environment depending on how I end up running the devcontainer. My devcontainer.json uses a feature that itself installs another feature via the nanolayer tool - this is a pattern that many of the contributed features use. This dependency feature sets some environment variables, notably it changes the PATH; if used directly (i.e. as part of the features set in devcontainer.json) the variables are set via the ENV directive in the generated Dockerfile. However, if the feature is used indirectly, the variables do not propagate to the Dockerfile, meaning that if I just run the generated image, the environment is wrong. If running the image via the devcontainer tooling, the CLI seems to inspect all the features (even the indirectly used ones) and "corrects" the environment at runtime, so when running via VSCode it's correct.

Here's a sketch of the situation:

devcontainer.json

...
    "features": {
      "<high-level-feature>: {}"
    }

Inside high-level-feature:

install.sh:

nanolayer install devcontainer-feature <dependency-feature>

Inside dependency-feature:

devcontainer-feature.json:

...
    "containerEnv": {
      "PATH": "/some/path:${PATH}"
    }

I'm wondering if this discrepancy is intentional? If the tool can inspect the environment of the dependency feature, why not inject it into the Dockerfile instead of reconciling at runtime? If this is intentional, what is the recommended best-practice in this case? I've seen that some features resort to injecting the environment variables into shell init scripts, but this is not generally a robust solution. As it stands, the generated images are in such cases only usable (without modification) through devcontainer tooling. Of course the high-level feature can also set those environment variables, but if multiple features are involved (or if the configuration changes) this becomes difficult to manage.

samruddhikhandale commented 1 year ago

Hi 👋

Thanks for opening the issue!

I'm wondering if this discrepancy is intentional? If the tool can inspect the environment of the dependency feature, why not inject it into the Dockerfile instead of reconciling at runtime?

Looping in @chrmarti @joshspicer to help provide their insights on this question.

what is the recommended best-practice in this case?

. My devcontainer.json uses a feature that itself installs another feature via the nanolayer tool - this is a pattern that many of the contributed features use.

@rokroskar Have you looked at the proposal for Feature dependencies, which adds the new dependsOn property?

A new property dependsOn can be optionally added to the devcontainer-feature.json. This property mirrors the features object in devcontainer.json. Adding Feature(s) to this property tells the orchestrating tool to install the Feature(s) (with the associated options, if provided) before installing the Feature that declares the dependency.

In the meanwhile, I wonder if the nanolayer tool can be replaced by the dependsOn property - which should ideally resolve the inconsistencies with the environment variables.

**Note: This new dependsOn property is added to the CLI with https://github.com/devcontainers/cli/pull/530 which is currently in preview.

rokroskar commented 1 year ago

Thanks for the quick follow up @samruddhikhandale, I missed this proposal and it's exactly what I was looking for. That will make feature writing much cleaner! I will close this now and test the preview version. Thanks!