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

extra_env_vars support fnmatch globs #21781

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/notes/2.25.x.md
Original file line number Diff line number Diff line change
Expand Up @@ -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_*`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think many targets have extra_env_vars fields too, so maybe:

Suggested change
- `[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_*`.


#### Remote Caching/Execution

Expand Down Expand Up @@ -56,7 +57,7 @@ Fixed an error which was caused when the same tool appeaed in both the `--docker
Strict adherence to the [schema of Helm OCI registry configuration](https://www.pantsbuild.org/2.25/reference/subsystems/helm#registries) is now required.
Previously we did ad-hoc coercion of some field values, so that, e.g., you could provide a "true"/"false" string as a boolean value. Now we require actual booleans.

The `helm_infer.external_docker_images` glob syntax has been generalized. In addition to `*`, you can now use Python [fnmatch](https://docs.python.org/3/library/fnmatch.html) to construct matterns like `quay.io/*`.
The `helm_infer.external_docker_images` glob syntax has been generalized. In addition to `*`, you can now use Python [fnmatch](https://docs.python.org/3/library/fnmatch.html) to construct patterns like `quay.io/*`.

#### Python

Expand Down
10 changes: 9 additions & 1 deletion src/python/pants/engine/env_vars.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from __future__ import annotations

import fnmatch
import re
from dataclasses import dataclass
from typing import Dict, Optional, Sequence
Expand Down Expand Up @@ -53,7 +54,8 @@ def check_and_set(name: str, value: Optional[str]):
if name_value_match:
check_and_set(name_value_match[1], name_value_match[2])
elif shorthand_re.match(env_var):
check_and_set(env_var, self.get(env_var))
for name, value in self.get_or_match(env_var).items():
check_and_set(name, value)
else:
raise ValueError(
f"An invalid variable was requested via the --test-extra-env-var "
Expand All @@ -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]:
Copy link
Contributor

@huonw huonw Dec 19, 2024

Choose a reason for hiding this comment

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

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, v

"""Get the value of an envvar if it has an exact match, otherwise all fnmatches."""
if name_or_pattern in self:
return {name_or_pattern: self.get(name_or_pattern)}
return {k: v for k, v in self.items() if fnmatch.fnmatch(k, name_or_pattern)}


@dataclass(frozen=True)
class EnvironmentVarsRequest:
Expand Down
17 changes: 17 additions & 0 deletions src/python/pants/engine/environment_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

@huonw huonw Dec 19, 2024

Choose a reason for hiding this comment

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

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?

"""Test fnmatch patterns correctly pull in all matching envvars."""

pants_env = CompleteEnvironmentVars(
{
"LETTER_C": "prefix_char_match",
"LETTER_PI": "prefix",
}
)

char_match = pants_env.get_subset(["LETTER_?"])
assert char_match == {"LETTER_C": "prefix_char_match"}

multichar_match = pants_env.get_subset(["LETTER_*"])
assert multichar_match == {"LETTER_C": "prefix_char_match", "LETTER_PI": "prefix"}
Loading