extra_env_vars support fnmatch globs #21781
Conversation
lilatomic
force-pushed
the
feature/envvar_globs
branch
from
2327eef to
47dc38c
Compare
There was a problem hiding this comment.
Nice, I like it.
Can we expand the docs of the relevant options/fields to mention they support globs? Otherwise people coming in new (and/or not reading release notes in detail) won't be aware of this handy functionality.
(And/or, maybe there's a relevant .mdx doc file, although I couldn't find one from a quick skim just now.)
src/python/pants/engine/env_vars.py
Outdated
| @@ -62,6 +64,12 @@ def check_and_set(name: str, value: Optional[str]): | |||
|
|
|||
| return FrozenDict(env_var_subset) | |||
|
|
|||
| def get_or_match(self, name_or_pattern: str) -> dict[str, str]: | |||
There was a problem hiding this comment.
Totally minor, but given this is just used as a .items(), maybe it could return Iterator[tuple[str, str]], with the body like
if value := self.get(name_or_pattern):
yield name_or_pattern, value
for k, v in self.items():
if fnmatch.fnmatch(k, name_or_pattern):
yield k, vThere was a problem hiding this comment.
There's a subtlety here for not returning fnmatch if we have an exact match. Inserting return after the yield in the initial block works. Although fnmatch could handle direct matches (eg with VAR_[*]), it's about 2200 times slower (I think because of all the re.compile, even with caching). Feels reasonable for a slight edge case for people using envvars with "*" or "?" in their names.
| @@ -37,3 +37,20 @@ def test_invalid_variable() -> None: | |||
| "An invalid variable was requested via the --test-extra-env-var mechanism: 3INVALID" | |||
| in str(exc) | |||
| ) | |||
|
|
|||
|
|
|||
| def test_envvar_fnmatch() -> None: | |||
There was a problem hiding this comment.
Could we add a test of the exact-matching behaviour? E.g. include an env variable LETTER_P* and call get_subset with that exact name, to validate that LETTER_PI isn't included?
docs/notes/2.25.x.md
Outdated
| @@ -21,6 +21,7 @@ Thank you to [Klayvio](https://www.klaviyo.com/) and [Normal Computing](https:// | |||
| - Fixed a longstanding bug in the processing of [synthetic targets](https://www.pantsbuild.org/2.24/docs/writing-plugins/the-target-api/concepts#synthetic-targets-api). This fix has the side-effect of requiring immutability and hashability of scalar values in BUILD files, which was always assumed but not enforced. This may cause BUILD file parsing errors, if you have custom field types involving custom mutable data structures. See ([#21725](https://github.com/pantsbuild/pants/pull/21725)) for more. | |||
| - [Fixed](https://github.com/pantsbuild/pants/pull/21665) bug where `pants --export-resolve=<resolve> --export-py-generated-sources-in-resolve=<resolve>` fails (see [#21659](https://github.com/pantsbuild/pants/issues/21659) for more info). | |||
| - [Fixed](https://github.com/pantsbuild/pants/pull/21694) bug where an `archive` target is unable to produce a ZIP file with no extension (see [#21693](https://github.com/pantsbuild/pants/issues/21693) for more info). | |||
| - `[subprocess-environment].env_vars` and `extra_env_vars` (on many subsystems) now supports a generalised glob syntax using Python [fnmatch](https://docs.python.org/3/library/fnmatch.html) to construct patterns like `AWS_*`, `TF_*`, and `S2TESTS_*`. | |||
There was a problem hiding this comment.
I think many targets have extra_env_vars fields too, so maybe:
| - `[subprocess-environment].env_vars` and `extra_env_vars` (on many subsystems) now supports a generalised glob syntax using Python [fnmatch](https://docs.python.org/3/library/fnmatch.html) to construct patterns like `AWS_*`, `TF_*`, and `S2TESTS_*`. | |
| - `[subprocess-environment].env_vars` and `extra_env_vars` (on many subsystems and targets) now supports a generalised glob syntax using Python [fnmatch](https://docs.python.org/3/library/fnmatch.html) to construct patterns like `AWS_*`, `TF_*`, and `S2TESTS_*`. |
|
Oop, I completely skipped the docs. I've added to everywhere I could find use of. I don't see a good place to add this as a "general" thing, although I think that might be because it's not a general thing. |
- exact match for names with glob characters in them - case sensitivity (`fnmatch.fnmatch` normalises case on Windows)
lilatomic
force-pushed
the
feature/envvar_globs
branch
from
92c645e to
41ccb16
Compare
|
|
||
| from pants.util.frozendict import FrozenDict | ||
| from pants.util.ordered_set import FrozenOrderedSet | ||
|
|
||
| name_value_re = re.compile(r"([A-Za-z_]\w*)=(.*)") | ||
| shorthand_re = re.compile(r"([A-Za-z_]\w*)") | ||
|
|
||
| EXTRA_ENV_VARS_USAGE_HELP = """\ |
There was a problem hiding this comment.
Good idea to factor this repeated text out 👍
It first checks for exact matches, then tries an fnmatch.
closes #20291