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.

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

Make local env vars available in non-local environments #20122

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

Conversation

gauthamnair
Copy link
Contributor

@gauthamnair gauthamnair commented Oct 30, 2023

Fixes #18577

Background and Motivation

Users like to be able to transmit environment variables from their running shell into processes run by pants.
This has not worked when using docker_environment or remote execution. In both cases, the universe of environment variables passed to the process is restricted to the environment within the docker container or the remote execution environment.

Pants intentionally sets up the environment variables for the sandboxed processes it runs:

@dataclass(frozen=True)
class Process:
    argv: tuple[str, ...]
    ...
    env: FrozenDict[str, str]    # this dict will be the env experienced by the sandboxed command when it runs
    ...

The env dictionary is built in rule code. Values from the user shell or the execution environments can get in here from rules that read and intentionally subset CompleteEnvVars, which represents the environment outside of pants.

In main, CompleteEnvVars comes from one of the following sources, depending on the execution environment target:

  • local environment: grab the whole os.environ from the shell running pants.
  • docker_environment: run env to read and save the environment in the docker container
  • remote: same as above - run env and read the remote env.

As a result, environment variables set in the user's shell do not make it into non-local environments

Intended change

the docker and remote environment targets have a new field pass_through_env_vars taking a list of variable names to take from the local running shell and merge with the non-local environment's environment variables when building CompleteEnvVars. Pants can then use them when setting up the env for sandboxed processes.

Testing example

https://github.com/gauthamnair/example-python/tree/docker-env-vars

BUILD

python_tests(
  name="tests",
  environment=parametrize(d="docker", l="local")
)

local_environment(
  name="local_env",
)

docker_environment(
  name="docker_env",
  platform="linux_x86_64",
  image="python:3.11.6-bullseye",
  python_bootstrap_search_path=["<PATH>"],
  pass_through_env_vars=["CFG"],
)

pants.toml

[environments-preview.names]
local = "//:local_env"
docker = "//:docker_env"

[test]
extra_env_vars = [
  "CFG",
]

env_test.py constructed to fail but to reveal platform and environment:

from platform import platform
import os

def test_platform():
    assert ('hello1', 'b') == (platform(), os.getenv('CFG', '__'))

currently showing the env var getting passed all the way into the test in both local and docker environments:

$ CFG=some-cfg pants_from_sources test //env_test.py:tests
...

>       assert ('hello1', 'b') == (platform(), os.getenv('CFG', '__'))
E       AssertionError: assert ('hello1', 'b') == ('macOS-13.0-...', 'some-cfg')
E         At index 0 diff: 'hello1' != 'macOS-13.0-arm64-arm-64bit'
E         Use -v to get the full diff

...
E       AssertionError: assert ('hello1', 'b') == ('Linux-5.15....', 'some-cfg')
E         At index 0 diff: 'hello1' != 'Linux-5.15.49-linuxkit-pr-x86_64-with-glibc2.31'
E         Use -v to get the full diff

✕ //env_test.py:tests@environment=d failed in 1.32s (ran in docker environment `docker`).
✕ //env_test.py:tests@environment=l failed in 0.27s (ran in local environment `local`).

@gauthamnair gauthamnair changed the title Passing env vars from shell to a docker env Make local env vars available in non-local environments Nov 2, 2023
@gauthamnair gauthamnair marked this pull request as ready for review November 2, 2023 18:26
@gauthamnair gauthamnair requested review from huonw and kaos November 2, 2023 18:28
@@ -17,6 +17,9 @@
class CompleteEnvironmentVars(FrozenDict):
"""CompleteEnvironmentVars contains all environment variables from the current Pants process.

For non-local environments like docker or remote execution we also include the local environment
from the shell running pants.
Copy link
Member

@kaos kaos Nov 2, 2023

Choose a reason for hiding this comment

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

I think we should be careful with this. When executing in a remote environment, there is no guarantee that the local env will be compatible with the remote system.

Think we should require the user to be very explicit with which env vars they want forwarded to the remote system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 Do you think this could look like an extra field on the remote_environment and/or docker_environment target?

Something like
pass_through_env_vars=["ENV_VAR_1", "ENV_VAR_2"]
and would mean that those variables are safe to pass through from the local environment and made available in the non-local environment

Copy link
Member

Choose a reason for hiding this comment

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

something like that, yea. feels like a good bikeshed topic for slack, being a user facing thing.. ;)

Copy link
Member

@stuhood stuhood Nov 6, 2023

Choose a reason for hiding this comment

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

Yea, definitely a per-environment-target option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuhood @kaos , made the change. It is now controlled via a new field pass_through_env_vars

@cedric-fauth
Copy link

Hi, how is the progress on this one? Would be very nice to have this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support passing env vars into docker_environments from user's shell
4 participants