Optionally inject environment variables into EnvironmentAware
#17013
Conversation
chrisjrn
added
the
category:internal
… subsystems at construct time. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…envvars # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
chrisjrn
force-pushed
the
14612-inject-environment-variables
branch
from
6324001
to
2c056b6
Compare
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| def rules(): | ||
| return [*collect_rules()] | ||
| @property | ||
| def environment_dict(self) -> dict[str, str]: |
There was a problem hiding this comment.
| def environment_dict(self) -> dict[str, str]: | |
| def environment_vars_dict(self) -> dict[str, str]: |
Maybe?
This somewhat conflates environments with environment variables... while environment variables can be overridden by environments via options, they don't have to be.
Unless we expect all access to environment variables to move to being via Subsystems (maybe...?), this results in two APIs for consuming environment variables.
There was a problem hiding this comment.
Well, not really -- the specific use case for this is for the cases where the subsystem's options are private, and the public API post-processes based on environment variables. This happens all the time, and the EnvironmentVarsRequest API is brittle -- you have to request the specific env vars ahead of time, and then pass them into the method that consumes them. This makes those subsystems very difficult to consume correctly. Pre-fetching the envvars makes that entirely non-brittle.
That said, the intention here is that EnvironmentAware.env_vars is meant to be used solely for those post-processing methods. Would it help if env_vars were called _env_vars instead?
There was a problem hiding this comment.
@stuhood to be clear, environment_dict was a property that existed on PythonNativeCodeSubsystem before today.
There was a problem hiding this comment.
to be clear, environment_dict was a property that existed on PythonNativeCodeSubsystem before today.
Ack. Renaming it would I think make things more clear, even though it's existing
There was a problem hiding this comment.
Sounds good. I don't feel strongly about it.
There was a problem hiding this comment.
@Eric-Arellano I'll do that now. Too many things named "environment" 🤣
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
This extends #17009 by making a generic mechanism to request environment variables when
EnvironmentAwaresubsystems are constructed.Generally speaking, this encapsulates both the requesting and consumption of environment variables into the subsystem itself, and this will save subsystem consumers from knowing which environment variables to request in order to call an option post-processing method. All option post-processing methods should safely be able to be converted to properties too. All very exciting.
Fixes #14612