[pull] master from DataDog:master

ddev/changelog.d/24289.added

@@ -0,0 +1 @@
+Add support for the `--env` option to `ddev env agent` to pass environment variables.

ddev/src/ddev/cli/env/agent.py

@@ -12,14 +12,21 @@
     from ddev.e2e.agent.interface import AgentInterface
 
 
-def _invoke_check_with_retry(agent: AgentInterface, args: list[str], *, retries: int = 3, backoff: float = 0.5) -> None:
+def _invoke_check_with_retry(
+    agent: AgentInterface,
+    args: list[str],
+    *,
+    env_vars: dict[str, str] | None = None,
+    retries: int = 3,
+    backoff: float = 0.5,
+) -> None:
     """Invoke ``agent check`` with bounded retry to absorb transient autodiscovery-reload races."""
     import subprocess
     import time
 
     for attempt in range(retries + 1):
         try:
-            agent.invoke(args)
+            agent.invoke(args, env_vars=env_vars)
             return
         except subprocess.CalledProcessError:
             if attempt >= retries:
@@ -31,15 +38,42 @@ def _invoke_check_with_retry(agent: AgentInterface, args: list[str], *, retries:
             time.sleep(backoff)
 
 
+def _validate_env_vars(ctx: click.Context, param: click.Parameter, value: tuple[str, ...]) -> dict[str, str] | None:
+    env_vars: dict[str, str] = {}
+    for entry in value:
+        key, sep, env_value = entry.partition('=')
+        if not sep or not key:
+            raise click.BadParameter(f'`{entry}` is not in KEY=VALUE format', ctx=ctx, param=param)
+        env_vars[key] = env_value
+
+    return env_vars or None
+
+
 @click.command(
     short_help='Invoke the Agent', context_settings={'help_option_names': [], 'ignore_unknown_options': True}
 )
 @click.argument('intg_name', metavar='INTEGRATION')
 @click.argument('environment')
 @click.argument('args', required=True, nargs=-1)
 @click.option('--config-file', hidden=True)
+@click.option(
+    '--env',
+    'env_vars',
+    multiple=True,
+    metavar='KEY=VALUE',
+    callback=_validate_env_vars,
+    help='Set an environment variable for this invocation only (may be repeated)',
+)
 @click.pass_obj
-def agent(app: Application, *, intg_name: str, environment: str, args: tuple[str, ...], config_file: str | None):
+def agent(
+    app: Application,
+    *,
+    intg_name: str,
+    environment: str,
+    args: tuple[str, ...],
+    config_file: str | None,
+    env_vars: dict[str, str] | None,
+):
     """
     Invoke the Agent.
     """
@@ -75,11 +109,13 @@ def agent(app: Application, *, intg_name: str, environment: str, args: tuple[str
     if config_file is None or not trigger_run:
         try:
             if trigger_run:
-                _invoke_check_with_retry(agent, full_args)
+                _invoke_check_with_retry(agent, full_args, env_vars=env_vars)
             else:
-                agent.invoke(full_args)
+                agent.invoke(full_args, env_vars=env_vars)
         except subprocess.CalledProcessError as e:
             app.abort(code=e.returncode)
+        except NotImplementedError as e:
+            app.abort(str(e))
 
         return
 
@@ -90,14 +126,18 @@ def agent(app: Application, *, intg_name: str, environment: str, args: tuple[str
     if not env_data.config_file.is_file():
         try:
             env_data.write_config(config)
-            _invoke_check_with_retry(agent, full_args)
+            _invoke_check_with_retry(agent, full_args, env_vars=env_vars)
+        except NotImplementedError as e:
+            app.abort(str(e))
         finally:
             env_data.config_file.unlink()
     else:
         temp_config_file = env_data.config_file.parent / f'{env_data.config_file.name}.bak.example'
         env_data.config_file.replace(temp_config_file)
         try:
             env_data.write_config(config)
-            _invoke_check_with_retry(agent, full_args)
+            _invoke_check_with_retry(agent, full_args, env_vars=env_vars)
+        except NotImplementedError as e:
+            app.abort(str(e))
         finally:
             temp_config_file.replace(env_data.config_file)

ddev/src/ddev/e2e/agent/docker.py

@@ -121,10 +121,12 @@ def _python_path(self) -> str:
             else f'/opt/datadog-agent/embedded/bin/python{self.python_version[0]}'
         )
 
-    def _format_command(self, command: list[str]) -> list[str]:
+    def _format_command(self, command: list[str], *, env_vars: dict[str, str] | None = None) -> list[str]:
         cmd = ['docker', 'exec']
         if self._isatty:
             cmd.append('-it')
+        for key, value in (env_vars or {}).items():
+            cmd.extend(['-e', f'{key}={value}'])
         cmd.append(self._container_name)
 
         if command[0] == 'pip':
@@ -364,11 +366,11 @@ def restart(self) -> None:
                 f'Unable to restart Agent container `{self._container_name}`: {process.stdout.decode("utf-8")}'
             )
 
-    def invoke(self, args: list[str]) -> None:
-        self.run_command(['agent', *args])
+    def invoke(self, args: list[str], *, env_vars: dict[str, str] | None = None) -> None:
+        self.run_command(['agent', *args], env_vars=env_vars)
 
-    def run_command(self, args: list[str]) -> None:
-        self._run_command(self._format_command([*args]), check=True)
+    def run_command(self, args: list[str], *, env_vars: dict[str, str] | None = None) -> None:
+        self._run_command(self._format_command([*args], env_vars=env_vars), check=True)
 
     def enter_shell(self) -> None:
         self._run_command(self._format_command(['cmd' if self._is_windows_container else 'bash']), check=True)

ddev/src/ddev/e2e/agent/interface.py

@@ -73,7 +73,7 @@ def stop(self) -> None: ...
     def restart(self) -> None: ...
 
     @abstractmethod
-    def invoke(self, args: list[str]) -> None: ...
+    def invoke(self, args: list[str], *, env_vars: dict[str, str] | None = None) -> None: ...
 
     @abstractmethod
     def enter_shell(self) -> None: ...

ddev/src/ddev/e2e/agent/vagrant.py

@@ -116,7 +116,10 @@ def restart(self) -> None:
         self._run_commands(guest_cmds, "restart_agent_service")
         self.app.display_info("Datadog Agent service restart sequence completed.")
 
-    def invoke(self, args: list[str]) -> None:
+    def invoke(self, args: list[str], *, env_vars: dict[str, str] | None = None) -> None:
+        if env_vars:
+            raise NotImplementedError("Per-invocation env_vars are not supported for the Vagrant agent")
+
         agent_bin = LINUX_AGENT_BIN_PATH if not self._is_windows_vm else WINDOWS_AGENT_BIN_PATH
         guest_cmd_parts = ["sudo", agent_bin] + args if not self._is_windows_vm else [agent_bin] + args
         host_cmd = self._format_command(guest_cmd_parts)

ddev/tests/AGENTS.md

@@ -0,0 +1,70 @@
+# ddev Test Guidelines
+
+These conventions apply to the tests under `ddev/tests/`. They supplement the repository root
+`AGENTS.md`; where they overlap, the root file still applies.
+
+## Importing from `conftest.py`
+
+**Runtime rule: nothing imports from a `conftest.py` at runtime.** Pytest loads
+conftest files through its own plugin machinery; importing one as a regular
+module can create a second copy, double-registering fixtures and duplicating
+module state. It may work in today's layout; it breaks under
+`--import-mode=importlib`.
+
+**The one exception: fixture types.** A type that describes what a fixture
+returns (e.g. a Protocol for a factory fixture's callable) lives next to that
+fixture in `conftest.py`, so the fixture itself is typed where it is defined.
+Tests that need it for annotations import it under `if TYPE_CHECKING:` only:
+
+    # conftest.py
+    class ClientFactory(Protocol):
+        def __call__(self, transport: httpx.MockTransport) -> AsyncGitHubClient: ...
+
+    @pytest.fixture
+    def client_factory(...) -> ClientFactory: ...
+
+    # test_x.py
+    if TYPE_CHECKING:
+        from .conftest import ClientFactory
+
+    def test_y(client_factory: ClientFactory) -> None: ...
+
+The guard is what makes this safe: the import never runs, so no second module
+is created. A conftest import outside `if TYPE_CHECKING:` is a bug, and a
+`TYPE_CHECKING` import of anything other than types (helpers, constants,
+payload factories) is too — needing one of those at runtime means it belongs
+in a helpers module.
+
+## Shared test code: scopes and placement
+
+Helpers live at the narrowest scope that contains all their users. Three scopes
+exist, from narrowest to broadest:
+
+| Scope     | Location                                   | Who may use it                          |
+|-----------|--------------------------------------------|------------------------------------------|
+| Local     | `helpers.py` next to the tests             | Tests in that one package                |
+| Subtree   | `helpers/` package at a subtree root (e.g. `tests/x/mypackage/helpers/`) | Test packages anywhere under that subtree |
+| Global    | `tests/helpers/`                            | Any test suite                           |
+
+Module vs package is size, not meaning: a `helpers.py` that outgrows one file
+becomes a `helpers/` package with the public names re-exported from
+`__init__.py`, and its scope does not change by doing so.
+
+**Placement rules:**
+
+- A new helper starts local. When a second package under the same subtree needs
+  it, promote it to the subtree `helpers/`. When packages outside the subtree
+  need it, promote it to `tests/helpers/`. Promote, never copy.
+- Imports point upward only: tests and local helpers may import from their
+  subtree helpers and from `tests/helpers/`; subtree helpers may import from
+  `tests/helpers/`. Never import sideways from another package's local
+  `helpers.py` — a sideways import means the helper's scope is wrong; promote it.
+- `conftest.py` at any level may import from helpers at its level or above.
+  Helpers never import from any conftest (see the conftest section).
+- Helpers contain no fixtures. Anything decorated with `@pytest.fixture`
+  belongs in a `conftest.py`; helpers hold the plain functions, factories,
+  transports, registries, and types those fixtures and tests build on.
+
+Litmus test when adding a helper: list who uses it today, find their nearest
+common ancestor in the tree, and put it there. If you cannot name a second
+user, it is local.

ddev/tests/cli/env/test_agent.py

@@ -1,6 +1,8 @@
 # (C) Datadog, Inc. 2023-present
 # All rights reserved
 # Licensed under a 3-clause BSD style license (see LICENSE)
+import pytest
+
 from ddev.e2e.config import EnvDataStorage
 
 
@@ -35,7 +37,108 @@ def test_not_trigger_run(ddev, data_dir, mocker):
     assert result.exit_code == 0, result.output
     assert not result.output
 
-    invoke.assert_called_once_with(['status'])
+    invoke.assert_called_once_with(['status'], env_vars=None)
+
+
+def test_env_vars(ddev, data_dir, mocker):
+    invoke = mocker.patch('ddev.e2e.agent.docker.DockerAgent.invoke')
+
+    integration = 'postgres'
+    environment = 'py3.12'
+    env_data = EnvDataStorage(data_dir).get(integration, environment)
+    env_data.write_metadata({})
+
+    result = ddev('env', 'agent', integration, environment, '--env', 'FOO=bar', '--env', 'BAZ=qux', 'status')
+
+    assert result.exit_code == 0, result.output
+    assert not result.output
+
+    invoke.assert_called_once_with(['status'], env_vars={'FOO': 'bar', 'BAZ': 'qux'})
+
+
+def test_env_vars_trigger_run(ddev, data_dir, mocker):
+    invoke = mocker.patch('ddev.e2e.agent.docker.DockerAgent.invoke')
+
+    integration = 'postgres'
+    environment = 'py3.12'
+    env_data = EnvDataStorage(data_dir).get(integration, environment)
+    env_data.write_metadata({})
+
+    result = ddev('env', 'agent', integration, environment, '--env', 'FOO=bar', 'check', integration, '-l', 'debug')
+
+    assert result.exit_code == 0, result.output
+    assert not result.output
+
+    invoke.assert_called_once_with(['check', integration, '-l', 'debug'], env_vars={'FOO': 'bar'})
+
+
+@pytest.mark.parametrize('env_value', ['FOO', '=FOO'])
+def test_env_vars_malformed(ddev, helpers, data_dir, mocker, env_value):
+    invoke = mocker.patch('ddev.e2e.agent.docker.DockerAgent.invoke')
+
+    integration = 'postgres'
+    environment = 'py3.12'
+    env_data = EnvDataStorage(data_dir).get(integration, environment)
+    env_data.write_metadata({})
+
+    result = ddev('env', 'agent', integration, environment, '--env', env_value, 'status')
+
+    assert result.exit_code == 2, result.output
+    assert result.output == helpers.dedent(
+        f"""
+        Usage: ddev env agent [OPTIONS] INTEGRATION ENVIRONMENT ARGS...
+
+        Error: Invalid value for '--env': `{env_value}` is not in KEY=VALUE format
+        """
+    )
+
+    invoke.assert_not_called()
+
+
+def test_env_vars_not_supported(ddev, helpers, data_dir, mocker):
+    invoke = mocker.patch(
+        'ddev.e2e.agent.docker.DockerAgent.invoke',
+        side_effect=NotImplementedError('Per-invocation env_vars are not supported for the Vagrant agent'),
+    )
+
+    integration = 'postgres'
+    environment = 'py3.12'
+    env_data = EnvDataStorage(data_dir).get(integration, environment)
+    env_data.write_metadata({})
+
+    result = ddev('env', 'agent', integration, environment, '--env', 'FOO=bar', 'status')
+
+    assert result.exit_code == 1, result.output
+    assert result.output == helpers.dedent(
+        """
+        Per-invocation env_vars are not supported for the Vagrant agent
+        """
+    )
+
+    invoke.assert_called_once_with(['status'], env_vars={'FOO': 'bar'})
+
+
+def test_env_vars_not_supported_trigger_run(ddev, helpers, data_dir, mocker):
+    invoke = mocker.patch(
+        'ddev.e2e.agent.docker.DockerAgent.invoke',
+        side_effect=NotImplementedError('Per-invocation env_vars are not supported for the Vagrant agent'),
+    )
+
+    integration = 'postgres'
+    environment = 'py3.12'
+    env_data = EnvDataStorage(data_dir).get(integration, environment)
+    env_data.write_metadata({})
+
+    result = ddev('env', 'agent', integration, environment, '--env', 'FOO=bar', 'check', integration, '-l', 'debug')
+
+    assert result.exit_code == 1, result.output
+    assert result.output == helpers.dedent(
+        """
+        Per-invocation env_vars are not supported for the Vagrant agent
+        """
+    )
+
+    invoke.assert_called_once_with(['check', integration, '-l', 'debug'], env_vars={'FOO': 'bar'})
 
 
 def test_trigger_run(ddev, data_dir, mocker):
@@ -51,7 +154,7 @@ def test_trigger_run(ddev, data_dir, mocker):
     assert result.exit_code == 0, result.output
     assert not result.output
 
-    invoke.assert_called_once_with(['check', integration, '-l', 'debug'])
+    invoke.assert_called_once_with(['check', integration, '-l', 'debug'], env_vars=None)
 
 
 def test_trigger_run_inject_integration(ddev, data_dir, mocker):
@@ -67,4 +170,4 @@ def test_trigger_run_inject_integration(ddev, data_dir, mocker):
     assert result.exit_code == 0, result.output
     assert not result.output
 
-    invoke.assert_called_once_with(['check', integration, '-l', 'debug'])
+    invoke.assert_called_once_with(['check', integration, '-l', 'debug'], env_vars=None)

ddev/tests/e2e/agent/test_docker.py

@@ -1217,6 +1217,35 @@ def test_basic(self, app, get_integration, docker_path, mocker):
             ),
         ]
 
+    def test_with_env_vars(self, app, get_integration, docker_path, mocker):
+        run = mocker.patch('subprocess.run', return_value=mocker.MagicMock(returncode=0))
+
+        integration = 'postgres'
+        environment = 'py3.12'
+        metadata = {}
+
+        agent = DockerAgent(app, get_integration(integration), environment, metadata, Path('config.yaml'))
+        agent.invoke(['check', 'postgres'], env_vars={'FOO': 'bar', 'BAZ': 'qux'})
+
+        assert run.call_args_list == [
+            mocker.call(
+                [
+                    docker_path,
+                    'exec',
+                    '-e',
+                    'FOO=bar',
+                    '-e',
+                    'BAZ=qux',
+                    f'dd_{integration}_{environment}',
+                    'agent',
+                    'check',
+                    'postgres',
+                ],
+                shell=False,
+                check=True,
+            ),
+        ]
+
 
 class TestEnterShell:
     def test_linux_container(self, app, get_integration, docker_path, mocker):