From 6add6b38b5d062efa1dd54c9b337f8d3047a713e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 00:32:23 +0000 Subject: [PATCH 1/4] Read sift-cli profiles in SiftClient and the pytest plugin sift-cli keeps per-environment credentials in a sift.toml and selects between them with --profile. Nothing else could read that file, so every SDK, script, and service re-derived credentials its own way and switching environments meant switching it in several places. Add a resolver that reads the same file and wire it into the two Python consumers. SiftClient() now resolves credentials when no connection_config is given, so it connects where sift-cli does with no arguments. SiftClient(profile=) and SiftClient.from_profile() select a named profile. The explicit-argument and connection_config paths are unchanged. credential_sources and profile report which layer supplied each value. The pytest plugin gains --sift-profile, the sift_profile ini key, and SIFT_PROFILE, as one PLUGIN_OPTIONS entry. The plugin's existing surfaces still outrank the profile, which fills only what they leave unset, so a key injected by CI is never overridden by a profile on the runner. This is the one place the precedence differs from SiftClient, and both are documented. Resolution order for SiftClient, highest first: inline arguments, the fields of a profile named in code, the per-field environment variables, the fields of the profile named by SIFT_PROFILE, then the config file's default table. Naming a profile is explicit so it beats ambient environment variables; SIFT_PROFILE is ambient so it does not, which keeps the CI case of profile endpoints plus an injected key working. At most one profile table is ever read, and a named profile does not inherit from the default table, matching sift-cli. Transport security now follows the gRPC URL's scheme. sift_py strips the scheme and decides plaintext vs TLS from use_ssl alone, so a profile's http://localhost:50051 would otherwise be dialed over TLS. This changes behavior only for http:// URLs, which could not work before. The config directory is hand-rolled to match Rust's dirs::config_dir(), which is what sift-cli uses. The current working directory is not searched, so a sift.toml in a cloned repository cannot supply an API key. Deferred deliberately: apikey_env and apikey_command belong in the shared schema alongside the Rust reader, and adding them here alone would recreate the drift this change exists to remove. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0151sUrwsupXb4c2vdAQBcuV --- python/CHANGELOG.md | 16 + python/docs/guides/credentials.md | 150 +++++++ python/docs/guides/index.md | 3 + .../guides/pytest_plugin/configuration.md | 14 +- .../lib/sift_client/_internal/credentials.py | 374 ++++++++++++++++++ .../_internal/pytest_plugin/options.py | 20 +- .../_tests/pytest_plugin/conftest.py | 23 +- .../_tests/pytest_plugin/test_credentials.py | 147 +++++++ .../sift_client/_tests/test_credentials.py | 254 ++++++++++++ python/lib/sift_client/client.py | 83 +++- python/lib/sift_client/credentials.py | 29 ++ python/lib/sift_client/errors.py | 8 + python/lib/sift_client/pytest_plugin.py | 75 +++- python/mkdocs.yml | 1 + 14 files changed, 1161 insertions(+), 36 deletions(-) create mode 100644 python/docs/guides/credentials.md create mode 100644 python/lib/sift_client/_internal/credentials.py create mode 100644 python/lib/sift_client/_tests/test_credentials.py create mode 100644 python/lib/sift_client/credentials.py diff --git a/python/CHANGELOG.md b/python/CHANGELOG.md index c2fcd53af6..d65a3d87f9 100644 --- a/python/CHANGELOG.md +++ b/python/CHANGELOG.md @@ -7,6 +7,22 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### What's New +#### Credentials from sift-cli profiles + +`SiftClient` now reads the same `sift.toml` profiles that `sift-cli --profile` uses, so an environment configured once for the CLI works from Python with no arguments. + +```python +client = SiftClient() # the CLI's default profile +client = SiftClient(profile="staging") # a named profile +client = SiftClient.from_profile("staging") +``` + +Arguments still win, then a profile named in code, then `SIFT_API_KEY` / `SIFT_GRPC_URI` / `SIFT_REST_URI` / `SIFT_APP_URL`, then the profile named by `SIFT_PROFILE`, then the config file's default profile. `client.credential_sources` reports which layer supplied each value. See [Credentials & Profiles](guides/credentials.md). + +The pytest plugin gains `--sift-profile`, the `sift_profile` ini key, and `SIFT_PROFILE`. There, the plugin's existing surfaces still outrank the profile, which fills in whatever they leave unset, so CI-injected values stay authoritative. + +Passing an `http://` URL now connects without TLS instead of failing: transport security follows the gRPC URL's scheme. `https://` and bare host names are unaffected. + #### List and get data imports New in `client.data_import`: `list_` and `get` (plus `find`), and a `run.data_imports` property. diff --git a/python/docs/guides/credentials.md b/python/docs/guides/credentials.md new file mode 100644 index 0000000000..0a46370be3 --- /dev/null +++ b/python/docs/guides/credentials.md @@ -0,0 +1,150 @@ +# Credentials and profiles + +`SiftClient` finds its credentials the same way `sift-cli` does. If you already +run `sift-cli`, the Python client works with no arguments: + +```python +from sift_client import SiftClient + +client = SiftClient() +``` + +To use a named environment, give it a profile name, the same one you pass to +`sift-cli --profile`: + +```python +client = SiftClient(profile="staging") + +# Equivalent, and easier to find in the docs: +client = SiftClient.from_profile("staging") +``` + +## The config file + +`sift-cli` keeps one or more profiles in a `sift.toml` under your user config +directory. Create and edit it with the CLI rather than by hand: + +```bash +sift-cli config create +sift-cli config update --profile staging +sift-cli config where # prints the path +``` + +The file looks like this. The top-level table is the default profile; each +named table is a profile: + +```toml +grpc_uri = "https://api.siftstack.com" +rest_uri = "https://api.siftstack.com" +app_uri = "https://app.siftstack.com" +apikey = "..." + +[staging] +grpc_uri = "https://api.staging.siftstack.com" +rest_uri = "https://api.staging.siftstack.com" +app_uri = "https://app.staging.siftstack.com" +apikey = "..." + +[localdev] +grpc_uri = "http://localhost:50051" +rest_uri = "http://localhost:8080" +apikey = "local" +``` + +A profile does not inherit from the default profile. If `[staging]` has no +`apikey`, that is an error rather than a silent fall back to the default +profile's key, which would otherwise point your tests at one environment using +another environment's credentials. + +Use an `http://` scheme for a plaintext endpoint, as `[localdev]` does above. +The client reads the scheme to decide whether to use TLS. + +## Resolution order + +Highest precedence first: + +1. Arguments you pass to `SiftClient`, per field. +2. The fields of a profile named by `profile=`. +3. The environment variables `SIFT_API_KEY`, `SIFT_GRPC_URI`, `SIFT_REST_URI`, + and `SIFT_APP_URL`. +4. The fields of the profile named by the `SIFT_PROFILE` environment variable. +5. The default (top-level) table of the config file. + +Naming a profile in code outranks the environment, so `SiftClient(profile="prod")` +still reaches production in a shell that was pointed somewhere else. +`SIFT_PROFILE` does not, so CI can select a profile for its endpoints and still +inject the API key through `SIFT_API_KEY`: + +```bash +export SIFT_PROFILE=staging # endpoints from the staging profile +export SIFT_API_KEY="$CI_SECRET" # key from the secret store +pytest +``` + +Only one profile is ever read. If both `profile=` and `SIFT_PROFILE` are set, +the argument wins and the other profile is ignored entirely. + +## Where the file is looked for + +1. `SIFT_CONFIG_FILE`, when set, is used directly. +2. Otherwise the user config directory: `$XDG_CONFIG_HOME/sift.toml` (or + `~/.config/sift.toml`) on Linux, `~/Library/Application Support/sift.toml` + on macOS, and `%APPDATA%\sift.toml` on Windows. + +The current working directory is not searched, so a `sift.toml` committed to a +repository you cloned cannot supply an API key. + +## Checking what a client resolved + +`credential_sources` reports which layer supplied each value, which is usually +faster than re-deriving the precedence by hand: + +```python +client = SiftClient(profile="staging") +client.profile # 'staging' +client.credential_sources # {'grpc_url': 'profile:staging', 'api_key': 'env', ...} +``` + +Each value is `arg`, `profile:`, `env`, `default`, or `unset`. Both are +`None` when the client was built from an explicit `connection_config`, which +bypasses resolution entirely. + +## Passing credentials directly + +Explicit arguments and `connection_config` work exactly as before. Use them +when credentials come from somewhere the resolver does not know about, such as +a secrets manager: + +```python +client = SiftClient( + api_key="...", + grpc_url="https://api.siftstack.com", + rest_url="https://api.siftstack.com", +) +``` + +## Errors + +When the API key or either URL cannot be resolved, `SiftClient` raises +`SiftCredentialsError`, which subclasses `ValueError`. The message names the +missing variables, the file and profile it looked in, the profiles that file +defines, and the `sift-cli` command that sets them. + +## Use with pytest + +The pytest plugin reads the same profiles. See +[Configuration & Defaults](pytest_plugin/configuration.md) for the plugin's own +settings, and note one difference: in the plugin, the plugin's existing +surfaces (environment variables, `--sift-*` flags, and the +`sift_grpc_uri` / `sift_rest_uri` ini keys) all outrank the profile, which +fills in whatever they leave unset. That keeps CI-injected values authoritative. + +```bash +pytest --sift-profile staging +``` + +```toml +# pyproject.toml +[tool.pytest.ini_options] +sift_profile = "staging" +``` diff --git a/python/docs/guides/index.md b/python/docs/guides/index.md index 105f0bb252..a932c93716 100644 --- a/python/docs/guides/index.md +++ b/python/docs/guides/index.md @@ -6,6 +6,9 @@ works and how to configure it. For runnable, end-to-end walkthroughs see the ## Available guides +- [Credentials & Profiles](credentials.md): how `SiftClient` resolves its API key + and endpoints from arguments, environment variables, and the `sift.toml` + profiles that `sift-cli` manages. - [Pytest Plugin](pytest_plugin/index.md): turn a pytest run into a `TestReport` in Sift. Each test becomes a `TestStep`, measurements are recorded as rows, and failures propagate up through nested substeps to the report. diff --git a/python/docs/guides/pytest_plugin/configuration.md b/python/docs/guides/pytest_plugin/configuration.md index c949b597d8..6da4963e10 100644 --- a/python/docs/guides/pytest_plugin/configuration.md +++ b/python/docs/guides/pytest_plugin/configuration.md @@ -161,12 +161,13 @@ suggestion, so typos like `SIFT_REPORT_SERIALNUM` surface immediately. ### Connection -| Setting | Ini (`[tool.pytest.ini_options]`) | Env var | -|---|---|---| -| Sift API key (secret, env-only). | — | `SIFT_API_KEY` | -| Sift gRPC endpoint URI. | `sift_grpc_uri` | `SIFT_GRPC_URI` | -| Sift REST endpoint URI. | `sift_rest_uri` | `SIFT_REST_URI` | -| Sift web-app origin for the report link in the terminal footer (e.g. https://app.siftstack.com). When unset, the link is derived from the REST URI for known Sift hosts. | `sift_app_url` | `SIFT_APP_URL` | +| Setting | CLI flag | Ini (`[tool.pytest.ini_options]`) | Env var | +|---|---|---|---| +| Named sift.toml profile to draw credentials from, as used by `sift-cli --profile`. | `--sift-profile` | `sift_profile` | `SIFT_PROFILE` | +| Sift API key (secret, env-only). | — | — | `SIFT_API_KEY` | +| Sift gRPC endpoint URI. | — | `sift_grpc_uri` | `SIFT_GRPC_URI` | +| Sift REST endpoint URI. | — | `sift_rest_uri` | `SIFT_REST_URI` | +| Sift web-app origin for the report link in the terminal footer (e.g. https://app.siftstack.com). When unset, the link is derived from the REST URI for known Sift hosts. | — | `sift_app_url` | `SIFT_APP_URL` | ### Report content @@ -179,6 +180,7 @@ suggestion, so typos like `SIFT_REPORT_SERIALNUM` surface immediately. | Serial number of the unit under test. | `[tool.sift.pytest.report] serial_number` | `SIFT_REPORT_SERIAL_NUMBER` | | Part number of the unit under test. | `[tool.sift.pytest.report] part_number` | `SIFT_REPORT_PART_NUMBER` | | Free-form report metadata, as a TOML table of scalar values. For dynamic per-run keys, override the sift_report_metadata fixture in conftest. | `[tool.sift.pytest.report.metadata]` (table) | — | + ### Quick-start examples diff --git a/python/lib/sift_client/_internal/credentials.py b/python/lib/sift_client/_internal/credentials.py new file mode 100644 index 0000000000..ad70319bd4 --- /dev/null +++ b/python/lib/sift_client/_internal/credentials.py @@ -0,0 +1,374 @@ +"""Resolution of Sift credentials from arguments, environment, and ``sift.toml``. + +``sift-cli`` stores one or more named profiles in a ``sift.toml`` under the +user's config directory, and selects between them with ``--profile``. This +module lets the Python client read that same file, so a developer who already +runs ``sift-cli --profile staging`` gets the same endpoints from +``SiftClient(profile="staging")`` without restating them. + +The resolution order, highest precedence first: + +1. Inline arguments (``SiftClient(api_key=...)``), per field. +2. The fields of a profile named explicitly via ``profile=``. +3. Per-field environment variables (``SIFT_API_KEY``, ``SIFT_GRPC_URI``, + ``SIFT_REST_URI``, ``SIFT_APP_URL``). +4. The fields of the profile named by ``SIFT_PROFILE``. +5. The default (top-level) table of the config file. + +Naming a profile explicitly outranks the ambient environment variables so that +an argument beats a shell that was pointed somewhere else; ``SIFT_PROFILE`` +does not, so per-field environment overrides still work in CI. + +At most one profile table is ever consulted. A named profile does not inherit +missing fields from the top-level table, matching ``sift-cli``: a profile that +omits ``apikey`` is an error rather than a silent fall back to the default +profile's key. +""" + +from __future__ import annotations + +import os +import sys +import warnings +from dataclasses import dataclass +from pathlib import Path +from typing import Any, Mapping +from urllib.parse import urlparse + +# ``tomllib`` landed in 3.11; ``tomli`` is the same parser packaged for older +# interpreters and is declared as a conditional install dep on 3.8-3.10. +try: + import tomllib # type: ignore[import-not-found,import-untyped,unused-ignore] +except ImportError: # pragma: no cover - exercised on 3.8-3.10 only + import tomli as tomllib # type: ignore[no-redef,import-not-found,unused-ignore] + +from sift_client.errors import SiftCredentialsError, SiftWarning + +CONFIG_FILE_NAME = "sift.toml" + +ENV_PROFILE = "SIFT_PROFILE" +ENV_CONFIG_FILE = "SIFT_CONFIG_FILE" + +# TOML key -> (public field name, environment variable). The TOML spellings are +# the ones ``sift-cli`` writes; the env spellings are the ones the pytest plugin +# already ships. They differ for the app URL and both are kept. +_FIELDS = ( + ("grpc_uri", "grpc_url", "SIFT_GRPC_URI"), + ("rest_uri", "rest_url", "SIFT_REST_URI"), + ("app_uri", "app_url", "SIFT_APP_URL"), + ("apikey", "api_key", "SIFT_API_KEY"), +) + +_REQUIRED = ("grpc_url", "rest_url", "api_key") + +_CLI_NAME = "sift-cli" + + +@dataclass(frozen=True) +class ResolvedCredentials: + """Credentials resolved from arguments, environment, and config file. + + ``sources`` maps each field name to the layer that supplied it: ``"arg"``, + ``"profile:"``, ``"env"``, ``"default"``, or ``"unset"``. It answers + "which environment am I actually talking to" without re-deriving the + precedence by hand. + """ + + api_key: str + grpc_url: str + rest_url: str + app_url: str | None + use_ssl: bool + profile: str | None + config_path: str | None + sources: Mapping[str, str] + + +def user_config_dir(env: Mapping[str, str] | None = None) -> Path | None: + """The directory ``sift-cli`` stores ``sift.toml`` in. + + Mirrors Rust's ``dirs::config_dir()``, which is what ``sift-cli`` uses: + ``%APPDATA%`` on Windows, ``~/Library/Application Support`` on macOS, and + ``$XDG_CONFIG_HOME`` (when absolute) or ``~/.config`` elsewhere. Returns + ``None`` when the home directory cannot be determined. + + This is deliberately hand-rolled rather than delegated to ``platformdirs``, + whose default app-name suffix would put the file somewhere ``sift-cli`` + never looks. + """ + environ = os.environ if env is None else env + + if sys.platform == "win32": + appdata = environ.get("APPDATA") + return Path(appdata) if appdata else None + + if sys.platform == "darwin": + home = _home(environ) + return home / "Library" / "Application Support" if home else None + + xdg = environ.get("XDG_CONFIG_HOME") + if xdg and os.path.isabs(xdg): + return Path(xdg) + home = _home(environ) + return home / ".config" if home else None + + +def _home(environ: Mapping[str, str]) -> Path | None: + home = environ.get("HOME") or environ.get("USERPROFILE") + if home: + return Path(home) + try: + return Path.home() + except (RuntimeError, OSError): + return None + + +def config_file_path( + config_path: str | None = None, + env: Mapping[str, str] | None = None, +) -> Path | None: + """Where to look for ``sift.toml``. + + An explicit path wins, then ``SIFT_CONFIG_FILE``, then the user config + directory. There is deliberately no search of the current working + directory: a ``sift.toml`` inside a checkout would let a cloned repository + supply an API key, which needs its own decision before it ships. + """ + if config_path is not None: + return Path(config_path) + environ = os.environ if env is None else env + from_env = environ.get(ENV_CONFIG_FILE) + if from_env: + return Path(from_env) + base = user_config_dir(environ) + return base / CONFIG_FILE_NAME if base else None + + +def _load_config(path: Path | None) -> dict[str, Any]: + """Parse the config file, or return ``{}`` when there isn't one. + + A missing file is not an error on its own, since arguments or environment + variables may supply everything. A file that exists but cannot be read or + parsed does raise: silently ignoring it would surface later as a confusing + "credentials missing" rather than the syntax error it is. + """ + if path is None: + return {} + try: + with path.open("rb") as fh: + return tomllib.load(fh) + except FileNotFoundError: + return {} + except OSError as exc: + raise SiftCredentialsError(f"Failed to read Sift config file '{path}': {exc}") from exc + except tomllib.TOMLDecodeError as exc: + raise SiftCredentialsError( + f"Sift config file '{path}' is not valid TOML: {exc}. " + f"Run `{_CLI_NAME} config show` to inspect it." + ) from exc + + +def _profile_names(config: Mapping[str, Any]) -> list[str]: + return sorted(key for key, value in config.items() if isinstance(value, dict)) + + +def _profile_table( + config: Mapping[str, Any], + name: str, + path: Path | None, +) -> Mapping[str, Any]: + table = config.get(name) + if isinstance(table, dict): + return table + + location = f"'{path}'" if path else "the Sift config file" + if not config: + raise SiftCredentialsError( + f"Profile '{name}' was requested but no Sift config file was found at {location}. " + f"Create one with `{_CLI_NAME} config create`, then " + f"`{_CLI_NAME} config update --profile {name}`." + ) + available = _profile_names(config) + known = ", ".join(available) if available else "none" + raise SiftCredentialsError( + f"Profile '{name}' was not found in {location}. Profiles defined there: {known}. " + f"Add it with `{_CLI_NAME} config update --profile {name}`." + ) + + +def _str_or_none(value: Any) -> str | None: + """Coerce a layer's value, treating empty and non-string values as absent.""" + if isinstance(value, str) and value: + return value + return None + + +def _derive_use_ssl(grpc_url: str, rest_url: str) -> bool: + """Infer transport security from the gRPC URL's scheme. + + ``sift_py`` strips the scheme off the URI and decides plaintext vs TLS from + ``use_ssl`` alone, so a profile's ``http://localhost:50051`` would otherwise + be dialed over TLS and fail. A bare host with no scheme keeps the TLS + default. + """ + grpc_scheme = urlparse(grpc_url).scheme + rest_scheme = urlparse(rest_url).scheme + use_ssl = grpc_scheme != "http" + + if grpc_scheme and rest_scheme and grpc_scheme != rest_scheme: + warnings.warn( + f"Sift gRPC URL uses '{grpc_scheme}://' but the REST URL uses " + f"'{rest_scheme}://'. Both connections will use " + f"{'TLS' if use_ssl else 'plaintext'}, following the gRPC URL.", + SiftWarning, + stacklevel=3, + ) + return use_ssl + + +def _select_profile( + profile: str | None, + environ: Mapping[str, str], +) -> tuple[str | None, str | None]: + """The profile to read and where its name came from (``"arg"`` or ``"env"``).""" + if profile: + return profile, "arg" + from_env = environ.get(ENV_PROFILE) + if from_env: + return from_env, "env" + return None, None + + +def resolve_credentials( + api_key: str | None = None, + grpc_url: str | None = None, + rest_url: str | None = None, + app_url: str | None = None, + profile: str | None = None, + config_path: str | None = None, + env: Mapping[str, str] | None = None, + require: bool = True, +) -> ResolvedCredentials: + """Resolve Sift credentials across arguments, environment, and ``sift.toml``. + + Args: + api_key: Explicit API key, overriding every other layer. + grpc_url: Explicit gRPC endpoint, overriding every other layer. + rest_url: Explicit REST endpoint, overriding every other layer. + app_url: Explicit Sift web-app origin, overriding every other layer. + profile: Name of a profile in the config file. Outranks the per-field + environment variables; see the module docstring. + config_path: Path to a specific config file, bypassing discovery. + env: Environment mapping to read, defaulting to ``os.environ``. + require: When ``True``, raise if the API key or either URL is still + missing. Pass ``False`` to resolve as much as is available and + leave the rest empty, as the pytest plugin's offline mode does. + + Returns: + The resolved credentials, including which layer supplied each field. + + Raises: + SiftCredentialsError: The config file is unreadable or malformed, the + named profile does not exist, or (when ``require``) a required + field could not be resolved. + """ + environ = os.environ if env is None else env + profile_name, profile_origin = _select_profile(profile, environ) + + path = config_file_path(config_path, environ) + config = _load_config(path) + + if profile_name is not None: + file_layer: Mapping[str, Any] = _profile_table(config, profile_name, path) + file_source = f"profile:{profile_name}" + else: + file_layer = config + file_source = "default" + + args = {"grpc_url": grpc_url, "rest_url": rest_url, "app_url": app_url, "api_key": api_key} + + # Ordered highest precedence first. The file layer appears exactly once: + # above the environment when its profile was named explicitly, below it + # otherwise. + layers: list[tuple[str, Mapping[str, Any]]] = [("arg", args)] + if profile_origin == "arg": + layers.append((file_source, file_layer)) + layers.append(("env", environ)) + if profile_origin != "arg": + layers.append((file_source, file_layer)) + + resolved: dict[str, str] = {} + sources: dict[str, str] = {} + for toml_key, field_name, env_key in _FIELDS: + for source, layer in layers: + if source == "arg": + value = _str_or_none(layer.get(field_name)) + elif source == "env": + value = _str_or_none(layer.get(env_key)) + else: + value = _str_or_none(layer.get(toml_key)) + if value is not None: + resolved[field_name] = value + sources[field_name] = source + break + else: + resolved[field_name] = "" + sources[field_name] = "unset" + + if require: + missing = [name for name in _REQUIRED if not resolved[name]] + if missing: + raise SiftCredentialsError( + _missing_message(missing, profile_name, profile_origin, path, config) + ) + + return ResolvedCredentials( + api_key=resolved["api_key"], + grpc_url=resolved["grpc_url"], + rest_url=resolved["rest_url"], + app_url=resolved["app_url"] or None, + use_ssl=_derive_use_ssl(resolved["grpc_url"], resolved["rest_url"]), + profile=profile_name, + config_path=str(path) if path else None, + sources=sources, + ) + + +def _missing_message( + missing: list[str], + profile_name: str | None, + profile_origin: str | None, + path: Path | None, + config: Mapping[str, Any], +) -> str: + """Explain what is missing, where it was looked for, and how to supply it.""" + env_names = {field_name: env_key for _, field_name, env_key in _FIELDS} + wanted = ", ".join(env_names[name] for name in missing) + + if profile_name is not None: + origin = "--profile/profile=" if profile_origin == "arg" else ENV_PROFILE + looked = f"profile '{profile_name}' (from {origin}) in '{path}'" + fix = f"`{_CLI_NAME} config update --profile {profile_name}`" + elif path is not None: + looked = f"the default profile in '{path}'" + fix = f"`{_CLI_NAME} config update`" + else: + looked = "the environment (no config file location could be determined)" + fix = f"`{_CLI_NAME} config create`" + + lines = [ + f"Sift credentials incomplete. Missing: {wanted}.", + f"Looked in: {looked}, then the environment.", + ] + if profile_name is None and config: + available = _profile_names(config) + if available: + lines.append( + f"Named profiles in that file: {', '.join(available)}. " + "Select one with profile= or SIFT_PROFILE=." + ) + lines.append( + f"Set them with {fix}, export {wanted}, " + "or pass api_key/grpc_url/rest_url to SiftClient directly." + ) + return " ".join(lines) diff --git a/python/lib/sift_client/_internal/pytest_plugin/options.py b/python/lib/sift_client/_internal/pytest_plugin/options.py index 461c0c93b6..c8368c3819 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/options.py +++ b/python/lib/sift_client/_internal/pytest_plugin/options.py @@ -113,8 +113,10 @@ def __post_init__(self) -> None: def resolve(self, config: pytest.Config | None) -> Any: """First set value from declared surfaces; ``None`` when unset everywhere. - Walk order is env > cli > ini > toml. No current option declares both - env and cli, so the chain isn't ambiguous in practice. + Walk order is env > cli > ini > toml. ``profile`` is the only option + declaring both env and cli, and env-before-cli is the wrong order for + it, so the ``sift_client`` fixture reads its CLI flag first rather than + calling this; see ``_resolve_profile``. ``getini`` returns the typed default for unset bool/list keys, so this only returns ini values for booleans (always meaningful), non-empty strings, and non-empty lists. @@ -335,7 +337,17 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: ini_default=True, ) -# Credentials. The API key is env-only; the URIs accept env + ini. +# Credentials. The API key is env-only; the URIs accept env + ini. A profile +# supplies whatever the other three leave unset, from the same sift.toml that +# `sift-cli --profile` reads. +PROFILE_OPTION = Option( + name="profile", + category=CAT_CONNECTION, + help="Named sift.toml profile to draw credentials from, as used by `sift-cli --profile`.", + cli="--sift-profile", + env="SIFT_PROFILE", + ini="sift_profile", +) API_KEY_OPTION = Option( name="api_key", category=CAT_CONNECTION, @@ -432,6 +444,7 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: MODULE_STEP_OPTION, CLASS_STEP_OPTION, PARAMETRIZE_NESTING_OPTION, + PROFILE_OPTION, API_KEY_OPTION, GRPC_URI_OPTION, REST_URI_OPTION, @@ -524,6 +537,7 @@ def _env_cell(opt: Option) -> str: ("Ini (`[tool.pytest.ini_options]`)", _ini_cell), ], CAT_CONNECTION: [ + ("CLI flag", _cli_cell), ("Ini (`[tool.pytest.ini_options]`)", _ini_cell), ("Env var", _env_cell), ], diff --git a/python/lib/sift_client/_tests/pytest_plugin/conftest.py b/python/lib/sift_client/_tests/pytest_plugin/conftest.py index 62569d0c45..df805980ee 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/conftest.py +++ b/python/lib/sift_client/_tests/pytest_plugin/conftest.py @@ -29,7 +29,15 @@ import pytest -_SIFT_ENV_VARS = ("SIFT_API_KEY", "SIFT_GRPC_URI", "SIFT_REST_URI", "SIFT_DISABLED", "SIFT_APP_URL") +_SIFT_ENV_VARS = ( + "SIFT_API_KEY", + "SIFT_GRPC_URI", + "SIFT_REST_URI", + "SIFT_DISABLED", + "SIFT_APP_URL", + "SIFT_PROFILE", + "SIFT_CONFIG_FILE", +) def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: @@ -45,6 +53,19 @@ def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: item.add_marker("plugin_compat") +@pytest.fixture(autouse=True) +def _isolate_sift_config_file(monkeypatch: pytest.MonkeyPatch, tmp_path_factory) -> None: + """Point credential resolution at a config file that does not exist. + + Inner sessions inherit this environment, so without it a developer's real + ``~/.config/sift.toml`` would supply credentials the test never set and the + missing-credential assertions would pass locally but not in CI. + """ + absent = tmp_path_factory.mktemp("sift-config") / "absent.toml" + monkeypatch.setenv("SIFT_CONFIG_FILE", str(absent)) + monkeypatch.delenv("SIFT_PROFILE", raising=False) + + @pytest.fixture def clear_sift_env(monkeypatch: pytest.MonkeyPatch) -> None: """Unset all ``SIFT_*`` environment variables for the duration of the test.""" diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py b/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py index 3f6d22a6e7..2099df7b1c 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py @@ -115,3 +115,150 @@ def test_missing_credentials_named_in_error( combined = "\n".join(result.outlines + result.errlines) for name in ("SIFT_API_KEY", "SIFT_GRPC_URI", "SIFT_REST_URI"): assert name in combined, combined + + +_PROFILE_CONFIG = """\ +grpc_uri = "https://grpc.default.example" +rest_uri = "https://rest.default.example" +apikey = "default-key" + +[staging] +grpc_uri = "https://grpc.staging.example" +rest_uri = "https://rest.staging.example" +apikey = "staging-key" + +[other] +grpc_uri = "https://grpc.other.example" +rest_uri = "https://rest.other.example" +apikey = "other-key" +""" + + +class TestProfiles: + """The fixture's use of ``sift.toml`` profiles via ``--sift-profile``.""" + + @staticmethod + def _write_config(pytester: pytest.Pytester, monkeypatch: pytest.MonkeyPatch) -> None: + config = pytester.path / "sift.toml" + config.write_text(_PROFILE_CONFIG) + monkeypatch.setenv("SIFT_CONFIG_FILE", str(config)) + for name in ("SIFT_API_KEY", "SIFT_GRPC_URI", "SIFT_REST_URI", "SIFT_PROFILE"): + monkeypatch.delenv(name, raising=False) + + def test_profile_supplies_credentials( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_plugin_conftest: Callable[[], None], + ) -> None: + """`--sift-profile` fills credentials no env var or ini key supplied.""" + self._write_config(pytester, monkeypatch) + write_plugin_conftest() + pytester.makepyfile( + """ + def test_from_profile(sift_client): + cfg = sift_client.grpc_client._config + assert cfg.api_key == "staging-key" + assert "grpc.staging.example" in cfg.uri + """ + ) + result = pytester.runpytest_subprocess("--sift-profile", "staging", "--sift-offline") + result.assert_outcomes(passed=1) + + def test_default_profile_used_without_a_name( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_plugin_conftest: Callable[[], None], + ) -> None: + """With no profile named, the config file's top-level table is used.""" + self._write_config(pytester, monkeypatch) + write_plugin_conftest() + pytester.makepyfile( + """ + def test_from_default(sift_client): + assert sift_client.grpc_client._config.api_key == "default-key" + """ + ) + result = pytester.runpytest_subprocess("--sift-offline") + result.assert_outcomes(passed=1) + + def test_env_var_overrides_profile( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_plugin_conftest: Callable[[], None], + ) -> None: + """In the plugin, an injected env var still wins so CI secrets stay authoritative.""" + self._write_config(pytester, monkeypatch) + monkeypatch.setenv("SIFT_API_KEY", "ci-key") + write_plugin_conftest() + pytester.makepyfile( + """ + def test_env_wins(sift_client): + cfg = sift_client.grpc_client._config + assert cfg.api_key == "ci-key" + assert "grpc.staging.example" in cfg.uri + """ + ) + result = pytester.runpytest_subprocess("--sift-profile", "staging", "--sift-offline") + result.assert_outcomes(passed=1) + + def test_profile_from_ini_key( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_plugin_conftest: Callable[[], None], + ) -> None: + """`sift_profile` in pyproject selects the profile without a CLI flag.""" + self._write_config(pytester, monkeypatch) + write_plugin_conftest() + pytester.makepyprojecttoml( + """ + [tool.pytest.ini_options] + sift_profile = "staging" + sift_offline = true + """ + ) + pytester.makepyfile( + """ + def test_from_ini_profile(sift_client): + assert sift_client.grpc_client._config.api_key == "staging-key" + """ + ) + result = pytester.runpytest_subprocess() + result.assert_outcomes(passed=1) + + def test_cli_flag_beats_env_profile( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_plugin_conftest: Callable[[], None], + ) -> None: + """Typing --sift-profile beats a SIFT_PROFILE left over in the shell.""" + self._write_config(pytester, monkeypatch) + monkeypatch.setenv("SIFT_PROFILE", "staging") + write_plugin_conftest() + pytester.makepyfile( + """ + def test_cli_wins(sift_client): + assert sift_client.grpc_client._config.api_key == "other-key" + """ + ) + result = pytester.runpytest_subprocess("--sift-profile", "other", "--sift-offline") + result.assert_outcomes(passed=1) + + def test_unknown_profile_is_a_usage_error( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_plugin_conftest: Callable[[], None], + ) -> None: + """A named profile that isn't in the file aborts and lists the ones that are.""" + self._write_config(pytester, monkeypatch) + write_plugin_conftest() + pytester.makepyfile("def test_should_not_run(): pass") + result = pytester.runpytest_subprocess("--sift-profile", "nope", "--sift-offline") + assert result.ret != 0 + combined = "\n".join(result.outlines + result.errlines) + assert "staging" in combined, combined diff --git a/python/lib/sift_client/_tests/test_credentials.py b/python/lib/sift_client/_tests/test_credentials.py new file mode 100644 index 0000000000..2c446c3f52 --- /dev/null +++ b/python/lib/sift_client/_tests/test_credentials.py @@ -0,0 +1,254 @@ +"""Tests for credential resolution across arguments, environment, and sift.toml.""" + +from __future__ import annotations + +import sys + +import pytest + +from sift_client._internal.credentials import ( + config_file_path, + resolve_credentials, + user_config_dir, +) +from sift_client.errors import SiftCredentialsError + +CONFIG = """\ +grpc_uri = "https://grpc.default.example" +rest_uri = "https://rest.default.example" +app_uri = "https://app.default.example" +apikey = "default-key" + +[staging] +grpc_uri = "https://grpc.staging.example" +rest_uri = "https://rest.staging.example" +app_uri = "https://app.staging.example" +apikey = "staging-key" + +[localdev] +grpc_uri = "http://localhost:50051" +rest_uri = "http://localhost:8080" +apikey = "local-key" + +[partial] +grpc_uri = "https://grpc.partial.example" +""" + + +@pytest.fixture +def config_file(tmp_path): + path = tmp_path / "sift.toml" + path.write_text(CONFIG) + return str(path) + + +def resolve(config_file=None, env=None, **kwargs): + """Resolve against an isolated environment so the host's config never leaks in.""" + return resolve_credentials(config_path=config_file, env=env or {}, **kwargs) + + +class TestProfileSelection: + def test_default_profile_is_top_level_table(self, config_file): + creds = resolve(config_file) + assert creds.grpc_url == "https://grpc.default.example" + assert creds.api_key == "default-key" + assert creds.profile is None + assert creds.sources["api_key"] == "default" + + def test_named_profile(self, config_file): + creds = resolve(config_file, profile="staging") + assert creds.grpc_url == "https://grpc.staging.example" + assert creds.api_key == "staging-key" + assert creds.profile == "staging" + assert creds.sources["api_key"] == "profile:staging" + + def test_profile_from_env(self, config_file): + creds = resolve(config_file, env={"SIFT_PROFILE": "staging"}) + assert creds.api_key == "staging-key" + assert creds.profile == "staging" + + def test_profile_argument_beats_env_profile(self, config_file): + creds = resolve(config_file, profile="localdev", env={"SIFT_PROFILE": "staging"}) + assert creds.profile == "localdev" + assert creds.api_key == "local-key" + + def test_named_profile_does_not_inherit_from_default(self, config_file): + """A profile missing a key is an error, not a silent fall back to the default's.""" + with pytest.raises(SiftCredentialsError) as exc: + resolve(config_file, profile="partial") + assert "SIFT_REST_URI" in str(exc.value) + assert "SIFT_API_KEY" in str(exc.value) + + def test_unknown_profile_lists_the_known_ones(self, config_file): + with pytest.raises(SiftCredentialsError) as exc: + resolve(config_file, profile="nope") + message = str(exc.value) + assert "'nope' was not found" in message + assert "localdev, partial, staging" in message + + def test_unknown_profile_without_config_file_says_so(self, tmp_path): + with pytest.raises(SiftCredentialsError) as exc: + resolve(str(tmp_path / "absent.toml"), profile="staging") + assert "no Sift config file was found" in str(exc.value) + + +class TestPrecedence: + def test_arguments_win_over_everything(self, config_file): + creds = resolve( + config_file, + profile="staging", + env={"SIFT_API_KEY": "env-key"}, + api_key="arg-key", + ) + assert creds.api_key == "arg-key" + assert creds.sources["api_key"] == "arg" + + def test_named_profile_beats_environment(self, config_file): + """Naming a profile is explicit, so it outranks an ambient env var.""" + creds = resolve( + config_file, + profile="staging", + env={"SIFT_GRPC_URI": "https://grpc.env.example"}, + ) + assert creds.grpc_url == "https://grpc.staging.example" + assert creds.sources["grpc_url"] == "profile:staging" + + def test_environment_beats_env_named_profile(self, config_file): + """SIFT_PROFILE is ambient too, so per-field env vars still override it.""" + creds = resolve( + config_file, + env={"SIFT_PROFILE": "staging", "SIFT_GRPC_URI": "https://grpc.env.example"}, + ) + assert creds.grpc_url == "https://grpc.env.example" + assert creds.sources["grpc_url"] == "env" + assert creds.api_key == "staging-key" + assert creds.sources["api_key"] == "profile:staging" + + def test_environment_beats_default_profile(self, config_file): + creds = resolve(config_file, env={"SIFT_API_KEY": "env-key"}) + assert creds.api_key == "env-key" + assert creds.sources["api_key"] == "env" + assert creds.grpc_url == "https://grpc.default.example" + + def test_partial_environment_override_keeps_profile_fields(self, config_file): + """The CI case: profile endpoints, key injected from a secret store.""" + creds = resolve(config_file, profile="staging", env={"SIFT_API_KEY": "ci-key"}) + assert creds.grpc_url == "https://grpc.staging.example" + assert creds.api_key == "staging-key" + + creds = resolve(config_file, env={"SIFT_PROFILE": "staging", "SIFT_API_KEY": "ci-key"}) + assert creds.grpc_url == "https://grpc.staging.example" + assert creds.api_key == "ci-key" + + def test_app_url_environment_name_differs_from_toml_key(self, config_file): + creds = resolve(config_file, env={"SIFT_APP_URL": "https://app.env.example"}) + assert creds.app_url == "https://app.env.example" + creds = resolve(config_file) + assert creds.app_url == "https://app.default.example" + + def test_empty_values_are_treated_as_absent(self, config_file): + creds = resolve(config_file, api_key="", env={"SIFT_API_KEY": ""}) + assert creds.api_key == "default-key" + assert creds.sources["api_key"] == "default" + + +class TestUseSsl: + def test_https_profile_uses_tls(self, config_file): + assert resolve(config_file, profile="staging").use_ssl is True + + def test_http_profile_disables_tls(self, config_file): + """Without this the transport would dial a plaintext port over TLS.""" + creds = resolve(config_file, profile="localdev") + assert creds.use_ssl is False + + def test_bare_host_keeps_the_tls_default(self, config_file): + creds = resolve(config_file, grpc_url="grpc.example:443", rest_url="rest.example") + assert creds.use_ssl is True + + def test_mismatched_schemes_warn_and_follow_grpc(self, config_file): + from sift_client.errors import SiftWarning + + with pytest.warns(SiftWarning, match="REST URL"): + creds = resolve( + config_file, + grpc_url="http://localhost:50051", + rest_url="https://rest.example", + ) + assert creds.use_ssl is False + + +class TestMissingAndMalformed: + def test_missing_everything_names_the_variables_and_the_fix(self, tmp_path): + with pytest.raises(SiftCredentialsError) as exc: + resolve(str(tmp_path / "absent.toml")) + message = str(exc.value) + assert "SIFT_GRPC_URI" in message + assert "SIFT_REST_URI" in message + assert "SIFT_API_KEY" in message + assert "sift-cli config update" in message + + def test_missing_message_lists_available_profiles(self, config_file, tmp_path): + stripped = tmp_path / "profiles-only.toml" + stripped.write_text('[staging]\ngrpc_uri = "https://g.example"\n') + with pytest.raises(SiftCredentialsError) as exc: + resolve(str(stripped)) + assert "Named profiles in that file: staging" in str(exc.value) + + def test_require_false_leaves_fields_empty(self, tmp_path): + creds = resolve(str(tmp_path / "absent.toml"), require=False) + assert creds.api_key == "" + assert creds.grpc_url == "" + assert creds.sources["api_key"] == "unset" + + def test_malformed_toml_raises_rather_than_falling_through(self, tmp_path): + bad = tmp_path / "sift.toml" + bad.write_text("grpc_uri = \nnot valid") + with pytest.raises(SiftCredentialsError, match="not valid TOML"): + resolve(str(bad), env={"SIFT_API_KEY": "k"}) + + def test_missing_file_is_not_an_error_when_env_supplies_everything(self, tmp_path): + creds = resolve( + str(tmp_path / "absent.toml"), + env={ + "SIFT_API_KEY": "k", + "SIFT_GRPC_URI": "https://g.example", + "SIFT_REST_URI": "https://r.example", + }, + ) + assert creds.api_key == "k" + assert creds.sources["grpc_url"] == "env" + + +class TestConfigDiscovery: + def test_explicit_path_wins(self, config_file): + assert str(config_file_path(config_file, {})) == config_file + + def test_env_var_overrides_the_config_directory(self, tmp_path): + target = tmp_path / "elsewhere.toml" + found = config_file_path(None, {"SIFT_CONFIG_FILE": str(target)}) + assert found == target + + def test_cwd_is_not_searched(self, tmp_path, monkeypatch): + """A sift.toml in a checkout must not be able to supply an API key.""" + monkeypatch.chdir(tmp_path) + (tmp_path / "sift.toml").write_text('apikey = "from-cwd"\n') + found = config_file_path(None, {"HOME": str(tmp_path / "home")}) + assert found != tmp_path / "sift.toml" + + @pytest.mark.skipif(sys.platform != "linux", reason="XDG layout is Linux-only") + def test_linux_uses_xdg_config_home_when_absolute(self, tmp_path): + assert user_config_dir({"XDG_CONFIG_HOME": str(tmp_path)}) == tmp_path + + @pytest.mark.skipif(sys.platform != "linux", reason="XDG layout is Linux-only") + def test_linux_ignores_relative_xdg_config_home(self, tmp_path): + found = user_config_dir({"XDG_CONFIG_HOME": "relative/path", "HOME": str(tmp_path)}) + assert found == tmp_path / ".config" + + @pytest.mark.skipif(sys.platform != "darwin", reason="macOS layout") + def test_macos_uses_application_support(self, tmp_path): + found = user_config_dir({"HOME": str(tmp_path)}) + assert found == tmp_path / "Library" / "Application Support" + + @pytest.mark.skipif(sys.platform != "win32", reason="Windows layout") + def test_windows_uses_appdata(self, tmp_path): + assert user_config_dir({"APPDATA": str(tmp_path)}) == tmp_path diff --git a/python/lib/sift_client/client.py b/python/lib/sift_client/client.py index b7c68b7a28..c509472404 100644 --- a/python/lib/sift_client/client.py +++ b/python/lib/sift_client/client.py @@ -2,8 +2,9 @@ import logging import warnings -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Mapping +from sift_client._internal.credentials import ResolvedCredentials, resolve_credentials from sift_client._internal.disk_cache_config import DiskCacheConfig from sift_client._internal.urls import frontend_origin_for_api from sift_client.errors import SiftWarning @@ -73,6 +74,12 @@ class SiftClient( from sift_client import SiftClient from datetime import datetime + # Use the same credentials sift-cli uses, from its default profile + client = SiftClient() + + # Or a named profile from sift.toml, like `sift-cli --profile staging` + client = SiftClient(profile="staging") + # Initialize with individual parameters client = SiftClient( api_key="your-api-key", @@ -154,35 +161,59 @@ def __init__( rest_url: str | None = None, connection_config: SiftConnectionConfig | None = None, app_url: str | None = None, + profile: str | None = None, ): """Initialize the SiftClient with specific connection parameters or a connection_config. + Any argument left unset is resolved from the environment and from the + ``sift.toml`` profiles that ``sift-cli`` manages, so ``SiftClient()`` + connects to the same place as ``sift-cli`` with no arguments at all. + See :func:`sift_client.credentials.resolve_credentials` for the full + precedence order. + Args: api_key: The Sift API key for authentication. grpc_url: The Sift gRPC API URL. rest_url: The Sift REST API URL. connection_config: A SiftConnectionConfig object to configure the connection behavior of the SiftClient. + When given, it is used as-is and no credential resolution happens. app_url: The Sift web-app origin (e.g. ``https://app.siftstack.com``). Set this for on-prem or custom deployments whose API host can't be mapped to a frontend automatically; see the ``app_url`` property. A value here takes precedence over ``connection_config.app_url``. + profile: Name of a ``sift.toml`` profile to draw credentials from, + equivalent to ``sift-cli --profile``. Ignored when + ``connection_config`` is given. + + Raises: + SiftCredentialsError: No ``connection_config`` was given and the API + key or either URL could not be resolved. """ - if not (api_key and grpc_url and rest_url) and not connection_config: - raise ValueError( - "Either api_key, grpc_url and rest_url or connection_config must be provided to establish a connection." - ) + self._credentials: ResolvedCredentials | None = None if connection_config: grpc_client = GrpcClient(connection_config.get_grpc_config()) rest_client = RestClient(connection_config.get_rest_config()) - elif api_key and grpc_url and rest_url: - grpc_client = GrpcClient(GrpcConfig(grpc_url, api_key)) - rest_client = RestClient(RestConfig(rest_url, api_key)) else: - raise ValueError( - "Invalid connection configuration. Please provide api_key, grpc_uri and rest_uri or a connection_config." + creds = resolve_credentials( + api_key=api_key, + grpc_url=grpc_url, + rest_url=rest_url, + app_url=app_url, + profile=profile, ) + self._credentials = creds + # ``use_ssl`` comes from the gRPC URL's scheme: the transport strips + # the scheme off and would otherwise dial an ``http://`` endpoint + # over TLS. + grpc_client = GrpcClient( + GrpcConfig(creds.grpc_url, creds.api_key, use_ssl=creds.use_ssl) + ) + rest_client = RestClient( + RestConfig(creds.rest_url, creds.api_key, use_ssl=creds.use_ssl) + ) + app_url = creds.app_url WithGrpcClient.__init__(self, grpc_client=grpc_client) WithRestClient.__init__(self, rest_client=rest_client) @@ -248,6 +279,38 @@ def __init__( data_import=DataImportAPIAsync(self), ) + @classmethod + def from_profile(cls, profile: str, **kwargs) -> SiftClient: + """Build a client from a named ``sift.toml`` profile. + + Equivalent to ``SiftClient(profile=...)``; keyword arguments are passed + through and still take precedence over the profile's values. + + Args: + profile: Profile name, as used by ``sift-cli --profile``. + **kwargs: Any other :class:`SiftClient` argument. + + Returns: + A client connected to the endpoints that profile names. + """ + return cls(profile=profile, **kwargs) + + @property + def credential_sources(self) -> Mapping[str, str] | None: + """Which layer supplied each credential, for diagnosing connections. + + Maps ``api_key`` / ``grpc_url`` / ``rest_url`` / ``app_url`` to + ``"arg"``, ``"profile:"``, ``"env"``, ``"default"``, or + ``"unset"``. ``None`` when the client was built from an explicit + ``connection_config``, which bypasses resolution. + """ + return self._credentials.sources if self._credentials else None + + @property + def profile(self) -> str | None: + """The ``sift.toml`` profile this client resolved its credentials from.""" + return self._credentials.profile if self._credentials else None + @property def grpc_client(self) -> GrpcClient: """The gRPC client used by the SiftClient for making gRPC API calls.""" diff --git a/python/lib/sift_client/credentials.py b/python/lib/sift_client/credentials.py new file mode 100644 index 0000000000..f813535428 --- /dev/null +++ b/python/lib/sift_client/credentials.py @@ -0,0 +1,29 @@ +"""Credential resolution shared by ``SiftClient`` and the pytest plugin. + +Reads the same ``sift.toml`` profiles that ``sift-cli --profile`` uses, so an +environment configured once for the CLI is available to the Python client +without restating its endpoints. See :func:`resolve_credentials` for the +precedence order. +""" + +from __future__ import annotations + +from sift_client._internal.credentials import ( + CONFIG_FILE_NAME, + ENV_CONFIG_FILE, + ENV_PROFILE, + ResolvedCredentials, + config_file_path, + resolve_credentials, + user_config_dir, +) + +__all__ = [ + "CONFIG_FILE_NAME", + "ENV_CONFIG_FILE", + "ENV_PROFILE", + "ResolvedCredentials", + "config_file_path", + "resolve_credentials", + "user_config_dir", +] diff --git a/python/lib/sift_client/errors.py b/python/lib/sift_client/errors.py index 34ffb66776..657c6a9488 100644 --- a/python/lib/sift_client/errors.py +++ b/python/lib/sift_client/errors.py @@ -11,6 +11,14 @@ class SiftExperimentalWarning(SiftWarning): """Warning for experimental features.""" +class SiftCredentialsError(ValueError): + """Raised when Sift credentials cannot be resolved. + + Subclasses ``ValueError`` because that is what ``SiftClient`` raised for + unusable connection arguments before credential resolution existed. + """ + + def _sift_stream_bindings_import_error(original_error: ImportError) -> NoReturn: # Returns NoReturn to satisfy pyright raise ImportError( diff --git a/python/lib/sift_client/pytest_plugin.py b/python/lib/sift_client/pytest_plugin.py index 3e2eda6d6e..0a9efebc27 100644 --- a/python/lib/sift_client/pytest_plugin.py +++ b/python/lib/sift_client/pytest_plugin.py @@ -23,6 +23,7 @@ import pytest from sift_client import SiftClient, SiftConnectionConfig +from sift_client._internal.credentials import resolve_credentials from sift_client._internal.pytest_plugin.audit_log import ( _make_session_dir, configure_audit_logging, @@ -46,6 +47,7 @@ LOG_FILE_OPTION, OPEN_OPTION, OUTPUT_DIR_OPTION, + PROFILE_OPTION, REST_URI_OPTION, register_options, resolved_settings, @@ -80,7 +82,7 @@ write_disabled_summary, write_report_summary, ) -from sift_client.errors import SiftWarning +from sift_client.errors import SiftCredentialsError, SiftWarning from sift_client.sift_types.test_report import TestStatus from sift_client.util.test_results import ReportContext from sift_client.util.test_results.context_manager import NewStep @@ -183,7 +185,7 @@ def abort(reason: str, returncode: int | None = None) -> NoReturn: @pytest.fixture(scope="session") def sift_client(pytestconfig: pytest.Config) -> SiftClient: - """Default ``SiftClient`` resolved from environment variables and ini keys. + """Default ``SiftClient`` resolved from env vars, ini keys, and sift.toml profiles. Each credential is read from its environment variable first. The URIs (``SIFT_GRPC_URI``, ``SIFT_REST_URI``) also fall back to the @@ -192,6 +194,13 @@ def sift_client(pytestconfig: pytest.Config) -> SiftClient: env-only; use ``pytest-dotenv`` (already a project dependency) to load it from a ``.env`` file kept out of version control. + Anything those surfaces leave unset is filled from a ``sift.toml`` profile, + the same file ``sift-cli --profile`` reads. Name one with ``--sift-profile``, + the ``sift_profile`` ini key, or ``SIFT_PROFILE``; with none named, the + file's default profile is used. Unlike :class:`~sift_client.SiftClient`, + here the profile sits *below* the env vars rather than above them, so a key + injected by CI is never overridden by a profile on the runner. + Projects that need custom construction (TLS toggles, custom timeouts, etc.) can override this fixture by defining their own ``sift_client`` in their ``conftest.py``; pytest fixture resolution prefers the local @@ -206,13 +215,33 @@ def sift_client(pytestconfig: pytest.Config) -> SiftClient: """ if is_disabled(pytestconfig): return build_disabled_client() + + offline = is_offline(pytestconfig) + # Everything the plugin's own surfaces resolved is passed as an explicit + # argument, so it outranks the profile; the profile fills only what they + # left unset. That keeps CI-injected env vars authoritative. + try: + creds = resolve_credentials( + api_key=API_KEY_OPTION.resolve(pytestconfig), + grpc_url=GRPC_URI_OPTION.resolve(pytestconfig), + rest_url=REST_URI_OPTION.resolve(pytestconfig), + app_url=APP_URL_OPTION.resolve(pytestconfig), + profile=_resolve_profile(pytestconfig), + require=False, + ) + except SiftCredentialsError as exc: + # A named profile that doesn't exist, or an unreadable config file, is a + # usage error even offline: the run asked for something specific. + log_event(logger, logging.ERROR, "credentials", error=type(exc).__name__) + raise pytest.UsageError(str(exc)) from exc + resolved = { - "SIFT_API_KEY": API_KEY_OPTION.resolve(pytestconfig), - "SIFT_GRPC_URI": GRPC_URI_OPTION.resolve(pytestconfig), - "SIFT_REST_URI": REST_URI_OPTION.resolve(pytestconfig), + "SIFT_API_KEY": creds.api_key, + "SIFT_GRPC_URI": creds.grpc_url, + "SIFT_REST_URI": creds.rest_url, } missing = [env for env, value in resolved.items() if not value] - if missing and not is_offline(pytestconfig): + if missing and not offline: log_event(logger, logging.ERROR, "credentials", missing=",".join(missing)) raise pytest.UsageError( "Sift credentials missing: " @@ -220,25 +249,39 @@ def sift_client(pytestconfig: pytest.Config) -> SiftClient: + ". Set the environment variable(s) (pytest-dotenv loads them " "from a `.env` file automatically), or set the URIs under " "`sift_grpc_uri` / `sift_rest_uri` in `[tool.pytest.ini_options]` " - "in pyproject.toml, or override the sift_client fixture in your " - "conftest.py, or pass --sift-offline / --sift-disabled to run " - "without contacting Sift." + "in pyproject.toml, or name a sift.toml profile with " + "`--sift-profile` / `sift_profile` / SIFT_PROFILE, or override the " + "sift_client fixture in your conftest.py, or pass --sift-offline / " + "--sift-disabled to run without contacting Sift." ) for env in missing: resolved[env] = OFFLINE_DEFAULTS[env] - # Web-app origin for the report link: the SIFT_APP_URL env var wins, then the - # sift_app_url ini key, else host-based derivation in SiftClient.app_url. - app_url = APP_URL_OPTION.resolve(pytestconfig) + return SiftClient( connection_config=SiftConnectionConfig( - api_key=resolved["SIFT_API_KEY"] or "", - grpc_url=resolved["SIFT_GRPC_URI"] or "", - rest_url=resolved["SIFT_REST_URI"] or "", - app_url=app_url or None, + api_key=resolved["SIFT_API_KEY"], + grpc_url=resolved["SIFT_GRPC_URI"], + rest_url=resolved["SIFT_REST_URI"], + app_url=creds.app_url, + use_ssl=creds.use_ssl, ) ) +def _resolve_profile(pytestconfig: pytest.Config) -> str | None: + """The sift.toml profile for this run, preferring the CLI flag over SIFT_PROFILE. + + ``Option.resolve`` walks env before cli, which is right for the credential + options but wrong for a profile: typing ``--sift-profile staging`` should + beat a ``SIFT_PROFILE`` left over in the shell. Only this option declares + both surfaces, so the reordering stays local. + """ + from_cli = pytestconfig.getoption(PROFILE_OPTION.cli_dest, default=None) + if from_cli: + return str(from_cli) + return PROFILE_OPTION.resolve(pytestconfig) + + @pytest.fixture(scope="session") def client_has_connection(pytestconfig: pytest.Config, request: pytest.FixtureRequest) -> bool: """Verify the ``SiftClient`` can reach Sift via ``/ping``. diff --git a/python/mkdocs.yml b/python/mkdocs.yml index 32699b635e..728d92aa81 100644 --- a/python/mkdocs.yml +++ b/python/mkdocs.yml @@ -66,6 +66,7 @@ nav: - Pytest Plugin Quickstart: examples/pytest_plugin_quickstart.md - Guides: - guides/index.md + - Credentials & Profiles: guides/credentials.md - Pytest Plugin: - Overview: guides/pytest_plugin/index.md - Configuration & Defaults: guides/pytest_plugin/configuration.md From 6acb257c7d82c36f3d53f9f050b6d6dfb3a88478 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 00:51:43 +0000 Subject: [PATCH 2/4] Clean up credential resolution after review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registry: Option gains a `surfaces` field declaring per-option precedence, and resolve_with_source walks it instead of a hardcoded env > cli > ini > toml. PROFILE_OPTION declares cli first, so the _resolve_profile workaround in pytest_plugin.py deletes. This also fixes a real disagreement: the audit log's settings snapshot goes through resolve_with_source, so with SIFT_PROFILE=staging in the shell and --sift-profile other on the command line, the run used `other` while the snapshot recorded `staging (env)`. The audit log is what people read when a run hits the wrong environment. Env var names now live once, in _internal/credentials.py, and options.py imports them. warn_on_unknown_env_vars unions CREDENTIAL_ENV_VARS with the registry, so SIFT_CONFIG_FILE no longer warns that a variable the run obeyed was "ignored" — it was reported as a typo on every plugin test, which is most of the drop in the suite's warning count. Resolver: Layers are normalized to field-keyed dicts before the precedence walk, so the loop no longer re-derives which key spelling to read from a string tag. The two possible layer orders are stated directly rather than built by conditional appends. _select_profile returns a bool instead of a string tag that encoded that bool. Dropped ResolvedCredentials.config_path, which nothing read. Reused the tomllib/tomli shim from pyproject_config rather than copying it. Client: The resolved branch now builds a SiftConnectionConfig and falls through to the shared GrpcClient/RestClient construction, instead of hand-building GrpcConfig and RestConfig a second time. Public surface: sift_client/credentials.py is trimmed to the two documented names, and client.py and pytest_plugin.py import through it, so there is one import path rather than two. Tests: The config-file isolation fixture moves to the package-wide conftest; any test constructing SiftClient without a connection_config would otherwise read the developer's real ~/.config/sift.toml. Four near-identical pytester tests collapse into one parametrized case. Added a regression test that SIFT_CONFIG_FILE is not reported as an unknown variable. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0151sUrwsupXb4c2vdAQBcuV --- .../lib/sift_client/_internal/credentials.py | 94 ++++++++-------- .../_internal/pytest_plugin/options.py | 94 ++++++++++------ python/lib/sift_client/_tests/conftest.py | 15 +++ .../_tests/pytest_plugin/conftest.py | 13 --- .../_tests/pytest_plugin/test_credentials.py | 104 ++++++------------ .../pytest_plugin/test_typo_detector.py | 20 ++++ .../sift_client/_tests/test_credentials.py | 7 +- python/lib/sift_client/client.py | 28 ++--- python/lib/sift_client/credentials.py | 21 +--- python/lib/sift_client/pytest_plugin.py | 18 +-- 10 files changed, 196 insertions(+), 218 deletions(-) diff --git a/python/lib/sift_client/_internal/credentials.py b/python/lib/sift_client/_internal/credentials.py index ad70319bd4..b48bd20844 100644 --- a/python/lib/sift_client/_internal/credentials.py +++ b/python/lib/sift_client/_internal/credentials.py @@ -35,30 +35,35 @@ from typing import Any, Mapping from urllib.parse import urlparse -# ``tomllib`` landed in 3.11; ``tomli`` is the same parser packaged for older -# interpreters and is declared as a conditional install dep on 3.8-3.10. -try: - import tomllib # type: ignore[import-not-found,import-untyped,unused-ignore] -except ImportError: # pragma: no cover - exercised on 3.8-3.10 only - import tomli as tomllib # type: ignore[no-redef,import-not-found,unused-ignore] - +# Shared with the ``[tool.sift]`` loader so the 3.8-3.10 ``tomli`` fallback is +# declared once. +from sift_client._internal.pyproject_config import tomllib from sift_client.errors import SiftCredentialsError, SiftWarning CONFIG_FILE_NAME = "sift.toml" ENV_PROFILE = "SIFT_PROFILE" ENV_CONFIG_FILE = "SIFT_CONFIG_FILE" +ENV_API_KEY = "SIFT_API_KEY" +ENV_GRPC_URI = "SIFT_GRPC_URI" +ENV_REST_URI = "SIFT_REST_URI" +ENV_APP_URL = "SIFT_APP_URL" # TOML key -> (public field name, environment variable). The TOML spellings are # the ones ``sift-cli`` writes; the env spellings are the ones the pytest plugin # already ships. They differ for the app URL and both are kept. _FIELDS = ( - ("grpc_uri", "grpc_url", "SIFT_GRPC_URI"), - ("rest_uri", "rest_url", "SIFT_REST_URI"), - ("app_uri", "app_url", "SIFT_APP_URL"), - ("apikey", "api_key", "SIFT_API_KEY"), + ("grpc_uri", "grpc_url", ENV_GRPC_URI), + ("rest_uri", "rest_url", ENV_REST_URI), + ("app_uri", "app_url", ENV_APP_URL), + ("apikey", "api_key", ENV_API_KEY), ) +#: Every ``SIFT_*`` variable this module reads. The pytest plugin unions this +#: with its own registry so its unknown-variable warning doesn't flag one of +#: these as a typo. +CREDENTIAL_ENV_VARS = (ENV_PROFILE, ENV_CONFIG_FILE, *(env for _, _, env in _FIELDS)) + _REQUIRED = ("grpc_url", "rest_url", "api_key") _CLI_NAME = "sift-cli" @@ -80,7 +85,6 @@ class ResolvedCredentials: app_url: str | None use_ssl: bool profile: str | None - config_path: str | None sources: Mapping[str, str] @@ -229,14 +233,11 @@ def _derive_use_ssl(grpc_url: str, rest_url: str) -> bool: def _select_profile( profile: str | None, environ: Mapping[str, str], -) -> tuple[str | None, str | None]: - """The profile to read and where its name came from (``"arg"`` or ``"env"``).""" +) -> tuple[str | None, bool]: + """The profile to read, and whether it was named explicitly rather than by env.""" if profile: - return profile, "arg" - from_env = environ.get(ENV_PROFILE) - if from_env: - return from_env, "env" - return None, None + return profile, True + return environ.get(ENV_PROFILE) or None, False def resolve_credentials( @@ -273,40 +274,36 @@ def resolve_credentials( field could not be resolved. """ environ = os.environ if env is None else env - profile_name, profile_origin = _select_profile(profile, environ) + profile_name, profile_is_explicit = _select_profile(profile, environ) path = config_file_path(config_path, environ) config = _load_config(path) if profile_name is not None: - file_layer: Mapping[str, Any] = _profile_table(config, profile_name, path) + table: Mapping[str, Any] = _profile_table(config, profile_name, path) file_source = f"profile:{profile_name}" else: - file_layer = config + table = config file_source = "default" - args = {"grpc_url": grpc_url, "rest_url": rest_url, "app_url": app_url, "api_key": api_key} + # Every layer is keyed by field name, so picking a value never depends on + # which layer it came from. + arg_layer = {"grpc_url": grpc_url, "rest_url": rest_url, "app_url": app_url, "api_key": api_key} + env_layer = {field: environ.get(env_key) for _, field, env_key in _FIELDS} + file_layer = {field: table.get(toml_key) for toml_key, field, _ in _FIELDS} - # Ordered highest precedence first. The file layer appears exactly once: - # above the environment when its profile was named explicitly, below it - # otherwise. - layers: list[tuple[str, Mapping[str, Any]]] = [("arg", args)] - if profile_origin == "arg": - layers.append((file_source, file_layer)) - layers.append(("env", environ)) - if profile_origin != "arg": - layers.append((file_source, file_layer)) + # Highest precedence first. A profile named explicitly outranks the ambient + # environment; one named by SIFT_PROFILE does not. + if profile_is_explicit: + layers = [("arg", arg_layer), (file_source, file_layer), ("env", env_layer)] + else: + layers = [("arg", arg_layer), ("env", env_layer), (file_source, file_layer)] resolved: dict[str, str] = {} sources: dict[str, str] = {} - for toml_key, field_name, env_key in _FIELDS: + for _, field_name, _ in _FIELDS: for source, layer in layers: - if source == "arg": - value = _str_or_none(layer.get(field_name)) - elif source == "env": - value = _str_or_none(layer.get(env_key)) - else: - value = _str_or_none(layer.get(toml_key)) + value = _str_or_none(layer.get(field_name)) if value is not None: resolved[field_name] = value sources[field_name] = source @@ -319,7 +316,7 @@ def resolve_credentials( missing = [name for name in _REQUIRED if not resolved[name]] if missing: raise SiftCredentialsError( - _missing_message(missing, profile_name, profile_origin, path, config) + _missing_message(missing, profile_name, profile_is_explicit, path, config) ) return ResolvedCredentials( @@ -329,7 +326,6 @@ def resolve_credentials( app_url=resolved["app_url"] or None, use_ssl=_derive_use_ssl(resolved["grpc_url"], resolved["rest_url"]), profile=profile_name, - config_path=str(path) if path else None, sources=sources, ) @@ -337,7 +333,7 @@ def resolve_credentials( def _missing_message( missing: list[str], profile_name: str | None, - profile_origin: str | None, + profile_is_explicit: bool, path: Path | None, config: Mapping[str, Any], ) -> str: @@ -346,7 +342,7 @@ def _missing_message( wanted = ", ".join(env_names[name] for name in missing) if profile_name is not None: - origin = "--profile/profile=" if profile_origin == "arg" else ENV_PROFILE + origin = "--profile/profile=" if profile_is_explicit else ENV_PROFILE looked = f"profile '{profile_name}' (from {origin}) in '{path}'" fix = f"`{_CLI_NAME} config update --profile {profile_name}`" elif path is not None: @@ -360,13 +356,11 @@ def _missing_message( f"Sift credentials incomplete. Missing: {wanted}.", f"Looked in: {looked}, then the environment.", ] - if profile_name is None and config: - available = _profile_names(config) - if available: - lines.append( - f"Named profiles in that file: {', '.join(available)}. " - "Select one with profile= or SIFT_PROFILE=." - ) + if profile_name is None and (available := _profile_names(config)): + lines.append( + f"Named profiles in that file: {', '.join(available)}. " + "Select one with profile= or SIFT_PROFILE=." + ) lines.append( f"Set them with {fix}, export {wanted}, " "or pass api_key/grpc_url/rest_url to SiftClient directly." diff --git a/python/lib/sift_client/_internal/pytest_plugin/options.py b/python/lib/sift_client/_internal/pytest_plugin/options.py index c8368c3819..57da77553c 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/options.py +++ b/python/lib/sift_client/_internal/pytest_plugin/options.py @@ -20,6 +20,14 @@ logger = logging.getLogger(__name__) +from sift_client._internal.credentials import ( + CREDENTIAL_ENV_VARS, + ENV_API_KEY, + ENV_APP_URL, + ENV_GRPC_URI, + ENV_PROFILE, + ENV_REST_URI, +) from sift_client._internal.pyproject_config import load_tool_sift # Settings-reference categories. Each maps to a docs subsection and, in the @@ -55,7 +63,7 @@ class Option: A setting may come from an env var, a CLI flag, a pytest ini key, or a ``[tool.sift...]`` TOML path. :meth:`resolve` walks the declared surfaces in - env > cli > ini > toml order; ``metadata`` (``merge=True``) is the one + ``surfaces`` order; ``metadata`` (``merge=True``) is the one free-form table, resolved by :meth:`resolve_merged`. The single ``PLUGIN_OPTIONS`` registry of these drives ``pytest_addoption``, the resolvers, the docs settings-reference table, and the typo detector. @@ -67,6 +75,8 @@ class Option: - ``toml``: tuple path under ``[tool.sift...]``, e.g. ``("pytest", "report", "name")`` -> ``tool.sift.pytest.report.name``. - ``env``: full env var name, e.g. ``"SIFT_API_KEY"``. + - ``surfaces``: precedence order, defaulting to env before cli. Override it + where a typed flag should beat an ambient env var, as ``profile`` does. ``category`` groups the option in the docs reference (one of ``CATEGORIES``). """ @@ -82,6 +92,7 @@ class Option: toml: tuple[str, ...] | None = None env: str | None = None merge: bool = False + surfaces: tuple[str, ...] = ("env", "cli", "ini", "toml") @property def cli_dest(self) -> str: @@ -109,14 +120,16 @@ def __post_init__(self) -> None: raise ValueError(f"Option({self.name!r}): declares no surfaces") if self.category not in CATEGORIES: raise ValueError(f"Option({self.name!r}): category must be one of {CATEGORIES}") + if set(self.surfaces) != {"env", "cli", "ini", "toml"}: + raise ValueError( + f"Option({self.name!r}): surfaces must be a permutation of " + "('env', 'cli', 'ini', 'toml')" + ) def resolve(self, config: pytest.Config | None) -> Any: """First set value from declared surfaces; ``None`` when unset everywhere. - Walk order is env > cli > ini > toml. ``profile`` is the only option - declaring both env and cli, and env-before-cli is the wrong order for - it, so the ``sift_client`` fixture reads its CLI flag first rather than - calling this; see ``_resolve_profile``. + Walk order is :attr:`surfaces`, env before cli by default. ``getini`` returns the typed default for unset bool/list keys, so this only returns ini values for booleans (always meaningful), non-empty strings, and non-empty lists. @@ -128,34 +141,42 @@ def resolve_with_source(self, config: pytest.Config | None) -> tuple[Any, str]: Returns ``(value, source)`` where ``source`` is one of ``env``/``cli``/``ini``/``toml``, or ``default`` when nothing set it - (``value`` is then ``None``). Used by the audit log's settings snapshot. + (``value`` is then ``None``). Used by the audit log's settings snapshot, + which therefore always reports the surface the run actually used. """ - if self.env: + for surface in self.surfaces: + value = self._read_surface(surface, config) + if value is not None: + return value, surface + return None, "default" + + def _read_surface(self, surface: str, config: pytest.Config | None) -> Any: + """This option's value from one surface, or ``None`` when unset there.""" + if surface == "env": + if not self.env: + return None env_value = os.getenv(self.env) - if env_value not in (None, ""): - return env_value, "env" + return env_value if env_value else None if config is None: - return None, "default" - if self.cli: - cli_value = config.getoption(self.cli_dest, default=None) - if cli_value is not None: - return cli_value, "cli" - if self.ini: + return None + if surface == "cli": + return config.getoption(self.cli_dest, default=None) if self.cli else None + if surface == "ini": + if not self.ini: + return None try: ini_value = config.getini(self.ini) except (KeyError, ValueError): - ini_value = None + return None if isinstance(ini_value, bool): - return ini_value, "ini" - if isinstance(ini_value, str) and ini_value: - return ini_value, "ini" - if isinstance(ini_value, list) and ini_value: - return ini_value, "ini" - if self.toml: - toml_value = _walk_toml(tool_sift(config), self.toml) - if toml_value not in (None, ""): - return toml_value, "toml" - return None, "default" + return ini_value + if isinstance(ini_value, (str, list)) and ini_value: + return ini_value + return None + if not self.toml: + return None + toml_value = _walk_toml(tool_sift(config), self.toml) + return toml_value if toml_value not in (None, "") else None def resolve_merged(self, config: pytest.Config | None) -> dict[str, str | float | bool]: """For ``merge=True`` dict-shape settings: the free-form TOML table. @@ -345,27 +366,28 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: category=CAT_CONNECTION, help="Named sift.toml profile to draw credentials from, as used by `sift-cli --profile`.", cli="--sift-profile", - env="SIFT_PROFILE", + env=ENV_PROFILE, ini="sift_profile", + surfaces=("cli", "env", "ini", "toml"), ) API_KEY_OPTION = Option( name="api_key", category=CAT_CONNECTION, help="Sift API key (secret, env-only).", - env="SIFT_API_KEY", + env=ENV_API_KEY, ) GRPC_URI_OPTION = Option( name="grpc_uri", category=CAT_CONNECTION, help="Sift gRPC endpoint URI.", - env="SIFT_GRPC_URI", + env=ENV_GRPC_URI, ini="sift_grpc_uri", ) REST_URI_OPTION = Option( name="rest_uri", category=CAT_CONNECTION, help="Sift REST endpoint URI.", - env="SIFT_REST_URI", + env=ENV_REST_URI, ini="sift_rest_uri", ) APP_URL_OPTION = Option( @@ -374,7 +396,7 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: help="Sift web-app origin for the report link in the terminal footer (e.g. " "https://app.siftstack.com). When unset, the link is derived from the REST URI " "for known Sift hosts.", - env="SIFT_APP_URL", + env=ENV_APP_URL, ini="sift_app_url", ) @@ -573,16 +595,18 @@ def _escape(cell: str) -> str: def warn_on_unknown_env_vars() -> None: - """Emit a warning for any ``SIFT_*`` env var not declared in the registry. + """Emit a warning for any ``SIFT_*`` env var this plugin doesn't read. - The registry declares each env var by its full name (``opt.env``); a - ``SIFT_*`` var that matches none of them is almost always a typo. + Known names are the registry's (``opt.env``) plus the credential resolver's + ``CREDENTIAL_ENV_VARS``, which includes variables like ``SIFT_CONFIG_FILE`` + that the resolver honors without the registry declaring them. A ``SIFT_*`` + var matching neither is almost always a typo. """ import difflib from sift_client.pytest_plugin import SiftPytestPluginWarning - known_full = {opt.env for opt in PLUGIN_OPTIONS if opt.env} + known_full = {opt.env for opt in PLUGIN_OPTIONS if opt.env} | set(CREDENTIAL_ENV_VARS) suggestion_pool = sorted(known_full) for name in sorted(os.environ): if not name.startswith("SIFT_"): diff --git a/python/lib/sift_client/_tests/conftest.py b/python/lib/sift_client/_tests/conftest.py index 2272cc2998..ecf94b6b24 100644 --- a/python/lib/sift_client/_tests/conftest.py +++ b/python/lib/sift_client/_tests/conftest.py @@ -33,6 +33,21 @@ def _isolate_default_disk_cache_path(monkeypatch, tmp_path): ) +@pytest.fixture(autouse=True) +def _isolate_sift_config_file(monkeypatch, tmp_path_factory): + """Point credential resolution at a config file that does not exist. + + ``SiftClient()`` without a ``connection_config`` reads ``sift.toml`` from + the user config directory, so without this a developer's real + ``~/.config/sift.toml`` would supply credentials a test never set — and + could point a test at a live backend. The plugin suite relies on it too: + its inner sessions inherit this environment through ``runpytest_subprocess``. + """ + absent = tmp_path_factory.mktemp("sift-config") / "absent.toml" + monkeypatch.setenv("SIFT_CONFIG_FILE", str(absent)) + monkeypatch.delenv("SIFT_PROFILE", raising=False) + + @pytest.fixture(scope="session") def sift_client() -> SiftClient: """Create a SiftClient instance for testing. diff --git a/python/lib/sift_client/_tests/pytest_plugin/conftest.py b/python/lib/sift_client/_tests/pytest_plugin/conftest.py index df805980ee..96d3966345 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/conftest.py +++ b/python/lib/sift_client/_tests/pytest_plugin/conftest.py @@ -53,19 +53,6 @@ def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: item.add_marker("plugin_compat") -@pytest.fixture(autouse=True) -def _isolate_sift_config_file(monkeypatch: pytest.MonkeyPatch, tmp_path_factory) -> None: - """Point credential resolution at a config file that does not exist. - - Inner sessions inherit this environment, so without it a developer's real - ``~/.config/sift.toml`` would supply credentials the test never set and the - missing-credential assertions would pass locally but not in CI. - """ - absent = tmp_path_factory.mktemp("sift-config") / "absent.toml" - monkeypatch.setenv("SIFT_CONFIG_FILE", str(absent)) - monkeypatch.delenv("SIFT_PROFILE", raising=False) - - @pytest.fixture def clear_sift_env(monkeypatch: pytest.MonkeyPatch) -> None: """Unset all ``SIFT_*`` environment variables for the duration of the test.""" diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py b/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py index 2099df7b1c..6ac2e3757e 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py @@ -6,10 +6,9 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Callable +from typing import Callable -if TYPE_CHECKING: - import pytest +import pytest class TestCredentials: @@ -145,63 +144,51 @@ def _write_config(pytester: pytest.Pytester, monkeypatch: pytest.MonkeyPatch) -> for name in ("SIFT_API_KEY", "SIFT_GRPC_URI", "SIFT_REST_URI", "SIFT_PROFILE"): monkeypatch.delenv(name, raising=False) - def test_profile_supplies_credentials( + @pytest.mark.parametrize( + ("extra_env", "cli_args", "expected_key"), + [ + pytest.param({}, ("--sift-profile", "staging"), "staging-key", id="named-profile"), + pytest.param({}, (), "default-key", id="default-profile"), + pytest.param( + {"SIFT_API_KEY": "ci-key"}, + ("--sift-profile", "staging"), + "ci-key", + id="env-beats-profile", + ), + pytest.param( + {"SIFT_PROFILE": "staging"}, + ("--sift-profile", "other"), + "other-key", + id="cli-beats-env-profile", + ), + ], + ) + def test_profile_selection( self, pytester: pytest.Pytester, monkeypatch: pytest.MonkeyPatch, write_plugin_conftest: Callable[[], None], + extra_env: dict[str, str], + cli_args: tuple[str, ...], + expected_key: str, ) -> None: - """`--sift-profile` fills credentials no env var or ini key supplied.""" - self._write_config(pytester, monkeypatch) - write_plugin_conftest() - pytester.makepyfile( - """ - def test_from_profile(sift_client): - cfg = sift_client.grpc_client._config - assert cfg.api_key == "staging-key" - assert "grpc.staging.example" in cfg.uri - """ - ) - result = pytester.runpytest_subprocess("--sift-profile", "staging", "--sift-offline") - result.assert_outcomes(passed=1) - - def test_default_profile_used_without_a_name( - self, - pytester: pytest.Pytester, - monkeypatch: pytest.MonkeyPatch, - write_plugin_conftest: Callable[[], None], - ) -> None: - """With no profile named, the config file's top-level table is used.""" - self._write_config(pytester, monkeypatch) - write_plugin_conftest() - pytester.makepyfile( - """ - def test_from_default(sift_client): - assert sift_client.grpc_client._config.api_key == "default-key" - """ - ) - result = pytester.runpytest_subprocess("--sift-offline") - result.assert_outcomes(passed=1) + """Which profile supplies the API key, across the surfaces that can name one. - def test_env_var_overrides_profile( - self, - pytester: pytest.Pytester, - monkeypatch: pytest.MonkeyPatch, - write_plugin_conftest: Callable[[], None], - ) -> None: - """In the plugin, an injected env var still wins so CI secrets stay authoritative.""" + ``env-beats-profile`` is the plugin's deliberate difference from + ``SiftClient``: a key injected by CI outranks the profile, so a profile + on the runner can never silently replace it. + """ self._write_config(pytester, monkeypatch) - monkeypatch.setenv("SIFT_API_KEY", "ci-key") + for name, value in extra_env.items(): + monkeypatch.setenv(name, value) write_plugin_conftest() pytester.makepyfile( - """ - def test_env_wins(sift_client): - cfg = sift_client.grpc_client._config - assert cfg.api_key == "ci-key" - assert "grpc.staging.example" in cfg.uri + f""" + def test_key(sift_client): + assert sift_client.grpc_client._config.api_key == {expected_key!r} """ ) - result = pytester.runpytest_subprocess("--sift-profile", "staging", "--sift-offline") + result = pytester.runpytest_subprocess(*cli_args, "--sift-offline") result.assert_outcomes(passed=1) def test_profile_from_ini_key( @@ -229,25 +216,6 @@ def test_from_ini_profile(sift_client): result = pytester.runpytest_subprocess() result.assert_outcomes(passed=1) - def test_cli_flag_beats_env_profile( - self, - pytester: pytest.Pytester, - monkeypatch: pytest.MonkeyPatch, - write_plugin_conftest: Callable[[], None], - ) -> None: - """Typing --sift-profile beats a SIFT_PROFILE left over in the shell.""" - self._write_config(pytester, monkeypatch) - monkeypatch.setenv("SIFT_PROFILE", "staging") - write_plugin_conftest() - pytester.makepyfile( - """ - def test_cli_wins(sift_client): - assert sift_client.grpc_client._config.api_key == "other-key" - """ - ) - result = pytester.runpytest_subprocess("--sift-profile", "other", "--sift-offline") - result.assert_outcomes(passed=1) - def test_unknown_profile_is_a_usage_error( self, pytester: pytest.Pytester, diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_typo_detector.py b/python/lib/sift_client/_tests/pytest_plugin/test_typo_detector.py index 435170ed51..aea9d04719 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_typo_detector.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_typo_detector.py @@ -111,3 +111,23 @@ def test_metadata_subtree_keys_are_user_defined( result = pytester.runpytest_subprocess("--sift-disabled") combined = "\n".join(result.outlines + result.errlines) assert "Unknown sift config key" not in combined, combined + + def test_credential_resolver_env_vars_are_known( + self, + pytester: pytest.Pytester, + clear_sift_env: None, + monkeypatch: pytest.MonkeyPatch, + write_plugin_conftest: Callable[[], None], + ) -> None: + """``SIFT_CONFIG_FILE`` is honored by the resolver, so it must not read as a typo. + + The registry does not declare it; the known set unions the credential + resolver's own variables so the warning can't tell a user that a + variable the run obeyed was ignored. + """ + monkeypatch.setenv("SIFT_CONFIG_FILE", str(pytester.path / "absent.toml")) + write_plugin_conftest() + pytester.makepyfile("def test_runs(): pass") + result = pytester.runpytest_subprocess("--sift-disabled") + combined = "\n".join(result.outlines + result.errlines) + assert "SIFT_CONFIG_FILE" not in combined, combined diff --git a/python/lib/sift_client/_tests/test_credentials.py b/python/lib/sift_client/_tests/test_credentials.py index 2c446c3f52..247e217719 100644 --- a/python/lib/sift_client/_tests/test_credentials.py +++ b/python/lib/sift_client/_tests/test_credentials.py @@ -6,11 +6,8 @@ import pytest -from sift_client._internal.credentials import ( - config_file_path, - resolve_credentials, - user_config_dir, -) +from sift_client._internal.credentials import config_file_path, user_config_dir +from sift_client.credentials import resolve_credentials from sift_client.errors import SiftCredentialsError CONFIG = """\ diff --git a/python/lib/sift_client/client.py b/python/lib/sift_client/client.py index c509472404..c5d1c18e05 100644 --- a/python/lib/sift_client/client.py +++ b/python/lib/sift_client/client.py @@ -4,9 +4,9 @@ import warnings from typing import TYPE_CHECKING, Mapping -from sift_client._internal.credentials import ResolvedCredentials, resolve_credentials from sift_client._internal.disk_cache_config import DiskCacheConfig from sift_client._internal.urls import frontend_origin_for_api +from sift_client.credentials import ResolvedCredentials, resolve_credentials from sift_client.errors import SiftWarning from sift_client.resources import ( AssetsAPI, @@ -46,9 +46,7 @@ from sift_client.resources.access_control import AccessControlAPI, AccessControlAPIAsync from sift_client.transport import ( GrpcClient, - GrpcConfig, RestClient, - RestConfig, SiftConnectionConfig, WithGrpcClient, WithRestClient, @@ -192,10 +190,7 @@ def __init__( """ self._credentials: ResolvedCredentials | None = None - if connection_config: - grpc_client = GrpcClient(connection_config.get_grpc_config()) - rest_client = RestClient(connection_config.get_rest_config()) - else: + if connection_config is None: creds = resolve_credentials( api_key=api_key, grpc_url=grpc_url, @@ -207,22 +202,23 @@ def __init__( # ``use_ssl`` comes from the gRPC URL's scheme: the transport strips # the scheme off and would otherwise dial an ``http://`` endpoint # over TLS. - grpc_client = GrpcClient( - GrpcConfig(creds.grpc_url, creds.api_key, use_ssl=creds.use_ssl) + connection_config = SiftConnectionConfig( + grpc_url=creds.grpc_url, + rest_url=creds.rest_url, + api_key=creds.api_key, + use_ssl=creds.use_ssl, + app_url=creds.app_url, ) - rest_client = RestClient( - RestConfig(creds.rest_url, creds.api_key, use_ssl=creds.use_ssl) - ) - app_url = creds.app_url + + grpc_client = GrpcClient(connection_config.get_grpc_config()) + rest_client = RestClient(connection_config.get_rest_config()) WithGrpcClient.__init__(self, grpc_client=grpc_client) WithRestClient.__init__(self, rest_client=rest_client) # Explicit web-app origin override; falls back to the connection config's # value, then to host-based derivation in the ``app_url`` property. - self._app_url: str | None = app_url or ( - connection_config.app_url if connection_config else None - ) + self._app_url: str | None = app_url or connection_config.app_url # When set, test-results writes return synthesized responses without # contacting Sift. Read by `TestResultsAPIAsync._simulate`. Used by the diff --git a/python/lib/sift_client/credentials.py b/python/lib/sift_client/credentials.py index f813535428..da0cf632e9 100644 --- a/python/lib/sift_client/credentials.py +++ b/python/lib/sift_client/credentials.py @@ -3,27 +3,18 @@ Reads the same ``sift.toml`` profiles that ``sift-cli --profile`` uses, so an environment configured once for the CLI is available to the Python client without restating its endpoints. See :func:`resolve_credentials` for the -precedence order. +precedence order, and the Credentials & Profiles guide for the config file's +shape. + +This is the public surface; the implementation lives in +``sift_client._internal.credentials``. """ from __future__ import annotations -from sift_client._internal.credentials import ( - CONFIG_FILE_NAME, - ENV_CONFIG_FILE, - ENV_PROFILE, - ResolvedCredentials, - config_file_path, - resolve_credentials, - user_config_dir, -) +from sift_client._internal.credentials import ResolvedCredentials, resolve_credentials __all__ = [ - "CONFIG_FILE_NAME", - "ENV_CONFIG_FILE", - "ENV_PROFILE", "ResolvedCredentials", - "config_file_path", "resolve_credentials", - "user_config_dir", ] diff --git a/python/lib/sift_client/pytest_plugin.py b/python/lib/sift_client/pytest_plugin.py index 0a9efebc27..aa7d6bdd2f 100644 --- a/python/lib/sift_client/pytest_plugin.py +++ b/python/lib/sift_client/pytest_plugin.py @@ -23,7 +23,6 @@ import pytest from sift_client import SiftClient, SiftConnectionConfig -from sift_client._internal.credentials import resolve_credentials from sift_client._internal.pytest_plugin.audit_log import ( _make_session_dir, configure_audit_logging, @@ -82,6 +81,7 @@ write_disabled_summary, write_report_summary, ) +from sift_client.credentials import resolve_credentials from sift_client.errors import SiftCredentialsError, SiftWarning from sift_client.sift_types.test_report import TestStatus from sift_client.util.test_results import ReportContext @@ -226,7 +226,7 @@ def sift_client(pytestconfig: pytest.Config) -> SiftClient: grpc_url=GRPC_URI_OPTION.resolve(pytestconfig), rest_url=REST_URI_OPTION.resolve(pytestconfig), app_url=APP_URL_OPTION.resolve(pytestconfig), - profile=_resolve_profile(pytestconfig), + profile=PROFILE_OPTION.resolve(pytestconfig), require=False, ) except SiftCredentialsError as exc: @@ -268,20 +268,6 @@ def sift_client(pytestconfig: pytest.Config) -> SiftClient: ) -def _resolve_profile(pytestconfig: pytest.Config) -> str | None: - """The sift.toml profile for this run, preferring the CLI flag over SIFT_PROFILE. - - ``Option.resolve`` walks env before cli, which is right for the credential - options but wrong for a profile: typing ``--sift-profile staging`` should - beat a ``SIFT_PROFILE`` left over in the shell. Only this option declares - both surfaces, so the reordering stays local. - """ - from_cli = pytestconfig.getoption(PROFILE_OPTION.cli_dest, default=None) - if from_cli: - return str(from_cli) - return PROFILE_OPTION.resolve(pytestconfig) - - @pytest.fixture(scope="session") def client_has_connection(pytestconfig: pytest.Config, request: pytest.FixtureRequest) -> bool: """Verify the ``SiftClient`` can reach Sift via ``/ping``. From 9bd0d68ff775f5b3fe645cf033002db6c3940618 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 16:54:56 +0000 Subject: [PATCH 3/4] Make api_key the canonical config key, keep apikey as an alias The Sift API spells it api_key throughout: api_keys.proto, api_key_id, ApiKey, /api/v2/api-keys. The one-word apikey exists only in the hand-written channel-config structs of the three first-generation SDKs (sift_py, Go, sift_connect), and sift.toml inherited it from sift_connect, which defined the file format. sift_client and the C++ SDK went back to the proto spelling, which is what made the inconsistency visible. Reconcile on api_key, and accept apikey permanently: real users have files on disk with the old key and should never have to migrate. sift_cli reads either spelling, canonical first, and reports the canonical one when the key is missing. `config update` writes api_key and removes a legacy apikey from the profile it touches, so no profile ends up carrying both. Other profiles are left alone. sift_connect accepts either spelling. Its two profile branches, which duplicated the same lookups, collapse into one table lookup plus a shared helper, so the alias is expressed once rather than four times. The Python resolver's TOML-key map already existed, so each field now carries its accepted spellings with the canonical one first. One behavior change in sift_connect: an empty-string uri or api_key is now treated as absent and reported as missing, rather than accepted and failing later at connect time. This matches what sift_cli already did. Not changed: the `uri` key, which needs restructuring into grpc_uri and rest_uri rather than a rename, and the Rust `Credentials::Config` struct field, which is public API. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0151sUrwsupXb4c2vdAQBcuV --- python/CHANGELOG.md | 2 + python/docs/guides/credentials.md | 13 ++- .../lib/sift_client/_internal/credentials.py | 28 ++++-- .../sift_client/_tests/test_credentials.py | 33 +++++++ rust/crates/sift_cli/src/cmd/config/mod.rs | 6 +- rust/crates/sift_cli/src/cmd/config/tests.rs | 54 ++++++++++++ rust/crates/sift_cli/src/cmd/mod.rs | 50 +++++++++-- rust/crates/sift_connect/README.md | 4 +- rust/crates/sift_connect/src/grpc/config.rs | 86 ++++++++++--------- rust/crates/sift_connect/src/lib.rs | 4 +- 10 files changed, 215 insertions(+), 65 deletions(-) diff --git a/python/CHANGELOG.md b/python/CHANGELOG.md index d65a3d87f9..446adad2e0 100644 --- a/python/CHANGELOG.md +++ b/python/CHANGELOG.md @@ -23,6 +23,8 @@ The pytest plugin gains `--sift-profile`, the `sift_profile` ini key, and `SIFT_ Passing an `http://` URL now connects without TLS instead of failing: transport security follows the gRPC URL's scheme. `https://` and bare host names are unaffected. +In the config file the API key is spelled `api_key`, matching the rest of Sift's API. The older `apikey` is still accepted everywhere and needs no migration; `api_key` wins if a profile carries both. + #### List and get data imports New in `client.data_import`: `list_` and `get` (plus `find`), and a `run.data_imports` property. diff --git a/python/docs/guides/credentials.md b/python/docs/guides/credentials.md index 0a46370be3..69b4bd05bd 100644 --- a/python/docs/guides/credentials.md +++ b/python/docs/guides/credentials.md @@ -37,28 +37,33 @@ named table is a profile: grpc_uri = "https://api.siftstack.com" rest_uri = "https://api.siftstack.com" app_uri = "https://app.siftstack.com" -apikey = "..." +api_key = "..." [staging] grpc_uri = "https://api.staging.siftstack.com" rest_uri = "https://api.staging.siftstack.com" app_uri = "https://app.staging.siftstack.com" -apikey = "..." +api_key = "..." [localdev] grpc_uri = "http://localhost:50051" rest_uri = "http://localhost:8080" -apikey = "local" +api_key = "local" ``` A profile does not inherit from the default profile. If `[staging]` has no -`apikey`, that is an error rather than a silent fall back to the default +`api_key`, that is an error rather than a silent fall back to the default profile's key, which would otherwise point your tests at one environment using another environment's credentials. Use an `http://` scheme for a plaintext endpoint, as `[localdev]` does above. The client reads the scheme to decide whether to use TLS. +Older files spell the key `apikey`, which is still accepted everywhere and needs +no migration. `api_key` is canonical, matches the rest of Sift's API, and is what +`sift-cli config update` writes; when a profile somehow carries both, `api_key` +wins. + ## Resolution order Highest precedence first: diff --git a/python/lib/sift_client/_internal/credentials.py b/python/lib/sift_client/_internal/credentials.py index b48bd20844..47cf1250e5 100644 --- a/python/lib/sift_client/_internal/credentials.py +++ b/python/lib/sift_client/_internal/credentials.py @@ -21,7 +21,7 @@ At most one profile table is ever consulted. A named profile does not inherit missing fields from the top-level table, matching ``sift-cli``: a profile that -omits ``apikey`` is an error rather than a silent fall back to the default +omits ``api_key`` is an error rather than a silent fall back to the default profile's key. """ @@ -49,14 +49,15 @@ ENV_REST_URI = "SIFT_REST_URI" ENV_APP_URL = "SIFT_APP_URL" -# TOML key -> (public field name, environment variable). The TOML spellings are -# the ones ``sift-cli`` writes; the env spellings are the ones the pytest plugin -# already ships. They differ for the app URL and both are kept. +# Accepted TOML keys -> (public field name, environment variable). The first +# TOML key is canonical, the rest are accepted spellings kept so files written by +# earlier releases keep working. The env spellings are the ones the pytest plugin +# already ships; they differ from the TOML keys for the app URL and both stay. _FIELDS = ( - ("grpc_uri", "grpc_url", ENV_GRPC_URI), - ("rest_uri", "rest_url", ENV_REST_URI), - ("app_uri", "app_url", ENV_APP_URL), - ("apikey", "api_key", ENV_API_KEY), + (("grpc_uri",), "grpc_url", ENV_GRPC_URI), + (("rest_uri",), "rest_url", ENV_REST_URI), + (("app_uri",), "app_url", ENV_APP_URL), + (("api_key", "apikey"), "api_key", ENV_API_KEY), ) #: Every ``SIFT_*`` variable this module reads. The pytest plugin unions this @@ -200,6 +201,15 @@ def _profile_table( ) +def _first_present(table: Mapping[str, Any], keys: tuple[str, ...]) -> Any: + """The value of the first of ``keys`` set in ``table``, canonical spelling first.""" + for key in keys: + value = table.get(key) + if _str_or_none(value) is not None: + return value + return None + + def _str_or_none(value: Any) -> str | None: """Coerce a layer's value, treating empty and non-string values as absent.""" if isinstance(value, str) and value: @@ -290,7 +300,7 @@ def resolve_credentials( # which layer it came from. arg_layer = {"grpc_url": grpc_url, "rest_url": rest_url, "app_url": app_url, "api_key": api_key} env_layer = {field: environ.get(env_key) for _, field, env_key in _FIELDS} - file_layer = {field: table.get(toml_key) for toml_key, field, _ in _FIELDS} + file_layer = {field: _first_present(table, toml_keys) for toml_keys, field, _ in _FIELDS} # Highest precedence first. A profile named explicitly outranks the ambient # environment; one named by SIFT_PROFILE does not. diff --git a/python/lib/sift_client/_tests/test_credentials.py b/python/lib/sift_client/_tests/test_credentials.py index 247e217719..c9dca11862 100644 --- a/python/lib/sift_client/_tests/test_credentials.py +++ b/python/lib/sift_client/_tests/test_credentials.py @@ -149,6 +149,39 @@ def test_empty_values_are_treated_as_absent(self, config_file): assert creds.sources["api_key"] == "default" +class TestApiKeySpelling: + """``api_key`` is canonical; ``apikey`` stays accepted for existing files.""" + + def test_canonical_api_key(self, tmp_path): + path = tmp_path / "sift.toml" + path.write_text( + 'grpc_uri = "https://g.example"\n' + 'rest_uri = "https://r.example"\n' + 'api_key = "canonical"\n' + ) + assert resolve(str(path)).api_key == "canonical" + + def test_legacy_apikey_still_accepted(self, config_file): + """The shared fixture uses `apikey`, so this is the migration path.""" + assert resolve(config_file).api_key == "default-key" + + def test_canonical_wins_when_a_file_carries_both(self, tmp_path): + """`sift-cli config update` drops the legacy key, but a hand-edited + file can hold both; the canonical spelling decides. + """ + path = tmp_path / "sift.toml" + path.write_text( + 'grpc_uri = "https://g.example"\n' + 'rest_uri = "https://r.example"\n' + 'api_key = "canonical"\n' + 'apikey = "legacy"\n' + ) + assert resolve(str(path)).api_key == "canonical" + + def test_legacy_key_in_a_named_profile(self, config_file): + assert resolve(config_file, profile="staging").api_key == "staging-key" + + class TestUseSsl: def test_https_profile_uses_tls(self, config_file): assert resolve(config_file, profile="staging").use_ssl is True diff --git a/rust/crates/sift_cli/src/cmd/config/mod.rs b/rust/crates/sift_cli/src/cmd/config/mod.rs index 616b8fdff0..fd8f60906f 100644 --- a/rust/crates/sift_cli/src/cmd/config/mod.rs +++ b/rust/crates/sift_cli/src/cmd/config/mod.rs @@ -14,6 +14,7 @@ use toml::{Table, Value}; use crate::{ cli::ConfigUpdateArgs, + cmd::{API_KEY_KEY, API_KEY_KEY_LEGACY}, util::{ app_uri::{infer_app_uri, normalize_app_uri}, tty::{Output, PromptUser}, @@ -245,7 +246,10 @@ fn apply_profile_updates( target.insert(String::from("rest_uri"), Value::String(uri)); } if let Some(token) = api_key { - target.insert(String::from("apikey"), Value::String(token)); + // Write the canonical spelling and drop the legacy one, so a profile + // never carries both and readers never have to pick a winner. + target.remove(API_KEY_KEY_LEGACY); + target.insert(String::from(API_KEY_KEY), Value::String(token)); } if let Some(uri) = app_uri.as_deref().and_then(normalize_app_uri) { target.insert(String::from("app_uri"), Value::String(uri.to_string())); diff --git a/rust/crates/sift_cli/src/cmd/config/tests.rs b/rust/crates/sift_cli/src/cmd/config/tests.rs index 6bd8d2ae82..120ab812ae 100644 --- a/rust/crates/sift_cli/src/cmd/config/tests.rs +++ b/rust/crates/sift_cli/src/cmd/config/tests.rs @@ -195,3 +195,57 @@ app_uri = "https://sift.example.net" ); } } + +mod api_key_key { + use super::super::apply_profile_updates; + use crate::cmd::{API_KEY_KEY, API_KEY_KEY_LEGACY}; + use toml::Table; + + fn config(input: &str) -> Table { + input.parse().unwrap() + } + + fn update_key(config: &mut Table, profile: Option<&str>, key: &str) { + apply_profile_updates( + config, + profile.map(String::from), + None, + None, + Some(key.to_string()), + None, + ) + .unwrap(); + } + + #[test] + fn writes_the_canonical_key() { + let mut config = config(""); + update_key(&mut config, None, "fresh"); + assert_eq!(config[API_KEY_KEY].as_str(), Some("fresh")); + assert!(!config.contains_key(API_KEY_KEY_LEGACY)); + } + + #[test] + fn migrates_a_legacy_key_instead_of_leaving_both() { + let mut config = config("apikey = \"old\"\n"); + update_key(&mut config, None, "new"); + assert_eq!(config[API_KEY_KEY].as_str(), Some("new")); + assert!( + !config.contains_key(API_KEY_KEY_LEGACY), + "the legacy key must be removed so readers never see two spellings" + ); + } + + #[test] + fn migrates_within_a_named_profile_only() { + let mut config = config("apikey = \"top\"\n\n[mission]\napikey = \"old\"\n"); + update_key(&mut config, Some("mission"), "new"); + + let mission = config["mission"].as_table().unwrap(); + assert_eq!(mission[API_KEY_KEY].as_str(), Some("new")); + assert!(!mission.contains_key(API_KEY_KEY_LEGACY)); + + // The default profile is untouched, legacy spelling included. + assert_eq!(config[API_KEY_KEY_LEGACY].as_str(), Some("top")); + } +} diff --git a/rust/crates/sift_cli/src/cmd/mod.rs b/rust/crates/sift_cli/src/cmd/mod.rs index c897e2ebad..9284c32efa 100644 --- a/rust/crates/sift_cli/src/cmd/mod.rs +++ b/rust/crates/sift_cli/src/cmd/mod.rs @@ -25,6 +25,22 @@ pub struct Context { pub app_uri: Option, } +/// The canonical TOML key for the API key, matching the `api_key` spelling the +/// Sift API itself uses. +pub(super) const API_KEY_KEY: &str = "api_key"; + +/// The original spelling, still accepted so configs written by older releases +/// keep working. Never written back. +pub(super) const API_KEY_KEY_LEGACY: &str = "apikey"; + +/// The profile's API key under either spelling, canonical first. +fn profile_api_key(profile: &Table) -> Option { + profile + .get(API_KEY_KEY) + .or_else(|| profile.get(API_KEY_KEY_LEGACY)) + .cloned() +} + impl Context { pub fn new(profile: Option, disable_tls: bool) -> Result { let config_path = config::get_config_file_path()?; @@ -100,16 +116,16 @@ impl Context { .and_then(normalize_app_uri) .map(str::to_string); - let Some(Value::String(api_key)) = target_profile.get("apikey").cloned() else { + let Some(Value::String(api_key)) = profile_api_key(target_profile) else { return Err(anyhow!( "Expected value of '{}' to be a string", - "apikey".yellow() + API_KEY_KEY.yellow() )); }; if api_key.is_empty() { return Err(anyhow!( "Expected value of '{}' to be present", - "apikey".yellow() + API_KEY_KEY.yellow() )); } @@ -157,7 +173,7 @@ mod tests { grpc_uri = "https://grpc-api.siftstack.com" rest_uri = "https://api.siftstack.com" app_uri = "https://app.siftstack.com" -apikey = "default-key" +api_key = "default-key" [mission] grpc_uri = "https://grpc.example.net" @@ -211,7 +227,7 @@ apikey = "key" "#, ), ( - "apikey", + "api_key", r#" grpc_uri = "https://grpc-api.siftstack.com" rest_uri = "https://api.siftstack.com" @@ -224,6 +240,30 @@ app_uri = "https://app.siftstack.com" } } + #[test] + fn accepts_either_api_key_spelling() { + // COMPLETE_CONFIG uses `api_key` at the top level and the legacy + // `apikey` under [mission], so one load covers both. + assert_eq!( + context(COMPLETE_CONFIG, None).unwrap().api_key, + "default-key" + ); + assert_eq!( + context(COMPLETE_CONFIG, Some("mission")).unwrap().api_key, + "mission-key" + ); + + // `config update` never writes both, but a hand-edited file can hold + // both; the canonical spelling decides. + let both = r#" +grpc_uri = "https://grpc-api.siftstack.com" +rest_uri = "https://api.siftstack.com" +api_key = "canonical" +apikey = "legacy" +"#; + assert_eq!(context(both, None).unwrap().api_key, "canonical"); + } + #[test] fn incomplete_app_uri_remains_loadable_for_recovery() { for app_uri in [ diff --git a/rust/crates/sift_connect/README.md b/rust/crates/sift_connect/README.md index 5fc1cdb920..58bae060ef 100644 --- a/rust/crates/sift_connect/README.md +++ b/rust/crates/sift_connect/README.md @@ -30,11 +30,11 @@ The following is an example of a valid `sift.toml` file: ```toml uri = "http://example-sift-api.com" -apikey = "example-sift-api-key" +api_key = "example-sift-api-key" [mission] uri = "http://example-sift-api.com" -apikey = "my-other-sift-api-key" +api_key = "my-other-sift-api-key" ``` The top-level TOML table is considered the default profile, with the `mission` table being a diff --git a/rust/crates/sift_connect/src/grpc/config.rs b/rust/crates/sift_connect/src/grpc/config.rs index a488d23e30..1620736e0d 100644 --- a/rust/crates/sift_connect/src/grpc/config.rs +++ b/rust/crates/sift_connect/src/grpc/config.rs @@ -20,13 +20,16 @@ pub const SIFT_CONFIG_NAME: &str = "sift.toml"; /// /// ```toml /// uri = "https://api.siftstack.com" -/// apikey = "default-api-key" +/// api_key = "default-api-key" /// /// [production] /// uri = "https://api.siftstack.com" -/// apikey = "production-api-key" +/// api_key = "production-api-key" /// ``` /// +/// The legacy `apikey` spelling is still accepted for files written by earlier +/// releases; `api_key` wins when a table carries both. +/// /// # Direct Credentials /// /// The `Config` variant allows you to provide credentials directly without @@ -102,56 +105,55 @@ impl TryFrom for SiftChannelConfig { .with_context(|| format!("failed to parse {}", config.display())) .help("ensure that the config file is properly formated")?; - match profile { + let (table, location) = match &profile { Some(p) => { - let Some(Value::Table(sub_table)) = config_toml.get(&p) else { + let Some(Value::Table(sub_table)) = config_toml.get(p) else { return Err(Error::new_msg( ErrorKind::ConfigError, format!("expected a '{p}' sub-table in '{}'", config.display()), )); }; - - let Some(Value::String(uri)) = sub_table.get("uri") else { - return Err(Error::new_msg( - ErrorKind::ConfigError, - format!("expected '{p}' to contain 'uri' entry"), - )); - }; - - let Some(Value::String(apikey)) = sub_table.get("apikey") else { - return Err(Error::new_msg( - ErrorKind::ConfigError, - format!("expected '{p}' to contain 'apikey' entry"), - )); - }; - - Ok(SiftChannelConfig::new(uri, apikey)) + (sub_table, format!("'{p}'")) } - None => { - let Some(Value::String(uri)) = config_toml.get("uri") else { - return Err(Error::new_msg( - ErrorKind::ConfigError, - format!( - "expected '{}' to contain a top-level 'uri' entry", - config.display() - ), - )); - }; + None => ( + &config_toml, + format!("a top-level entry in '{}'", config.display()), + ), + }; - let Some(Value::String(apikey)) = config_toml.get("apikey") else { - return Err(Error::new_msg( - ErrorKind::ConfigError, - format!( - "expected '{}' to contain a top-level 'apikey' entry", - config.display() - ), - )); - }; + let Some(uri) = lookup(table, &[URI_KEY]) else { + return Err(Error::new_msg( + ErrorKind::ConfigError, + format!("expected {location} to contain '{URI_KEY}'"), + )); + }; - Ok(SiftChannelConfig::new(uri, apikey)) - } - } + let Some(apikey) = lookup(table, API_KEY_KEYS) else { + return Err(Error::new_msg( + ErrorKind::ConfigError, + format!("expected {location} to contain '{}'", API_KEY_KEYS[0]), + )); + }; + + Ok(SiftChannelConfig::new(uri, apikey)) } } } } + +/// The canonical TOML key for the API key, matching the `api_key` spelling used +/// by the Sift API and by `sift-cli`, followed by the legacy `apikey` spelling +/// that earlier releases wrote. Lookups accept either; nothing writes the legacy +/// one. +pub const API_KEY_KEYS: &[&str] = &["api_key", "apikey"]; + +/// The TOML key naming the gRPC endpoint. +pub const URI_KEY: &str = "uri"; + +/// The first of `keys` present in `table` as a string, or `None`. +fn lookup<'a>(table: &'a Table, keys: &[&str]) -> Option<&'a str> { + keys.iter().find_map(|key| match table.get(*key) { + Some(Value::String(value)) if !value.is_empty() => Some(value.as_str()), + _ => None, + }) +} diff --git a/rust/crates/sift_connect/src/lib.rs b/rust/crates/sift_connect/src/lib.rs index 5445906753..f079e553a4 100644 --- a/rust/crates/sift_connect/src/lib.rs +++ b/rust/crates/sift_connect/src/lib.rs @@ -25,11 +25,11 @@ //! //! ```text //! uri = "http://example-sift-api.com" -//! apikey = "example-sift-api-key" +//! api_key = "example-sift-api-key" //! //! [mission] //! uri = "http://example-sift-api.com" -//! apikey = "my-other-sift-api-key" +//! api_key = "my-other-sift-api-key" //! ``` //! //! The top-level TOML table is considered the default profile, with the `mission` table being a From c2aeda71c2968f925c519ccc634802e2e520a3c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 18:06:14 +0000 Subject: [PATCH 4/4] Apply Simplified Technical English to the credentials docs Rewrite the user-facing prose added on this branch to ASD-STE100 style: the credentials guide, the changelog entry, the docstrings and comments in the resolver and its two consumers, and the Rust doc comments on the config keys. What changed, by rule: no semicolons, no contractions, and no em dashes. Active voice, so a sentence names the actor that reads the file or reports the error. Simple verb forms in place of gerunds and the present perfect. `can` and `must` in place of `may` and `would`. Descriptive sentences under 25 words, with the precedence enumeration in the changelog moved to a vertical list. One term per concept: the older key spelling is "older", not "legacy", and a precedence winner "outranks" rather than "wins" or "beats". Conditions now come before their commands, and the two rationale notes that interrupted procedures are marked `Note:`. The profile option's help text is reworded, so the generated settings table in the pytest plugin docs is regenerated to match. Pre-existing prose in these files is left alone, since rewriting it is a separate pass with its own review. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0151sUrwsupXb4c2vdAQBcuV --- python/CHANGELOG.md | 18 ++- python/docs/guides/credentials.md | 102 +++++++-------- python/docs/guides/index.md | 4 +- .../guides/pytest_plugin/configuration.md | 2 +- .../lib/sift_client/_internal/credentials.py | 121 +++++++++--------- .../_internal/pytest_plugin/options.py | 31 ++--- python/lib/sift_client/_tests/conftest.py | 9 +- .../_tests/pytest_plugin/test_credentials.py | 2 +- .../pytest_plugin/test_typo_detector.py | 6 +- .../sift_client/_tests/test_credentials.py | 6 +- python/lib/sift_client/client.py | 42 +++--- python/lib/sift_client/credentials.py | 12 +- python/lib/sift_client/pytest_plugin.py | 61 ++++----- python/mkdocs.yml | 2 +- rust/crates/sift_cli/src/cmd/config/mod.rs | 4 +- rust/crates/sift_cli/src/cmd/config/tests.rs | 4 +- rust/crates/sift_cli/src/cmd/mod.rs | 16 +-- rust/crates/sift_connect/src/grpc/config.rs | 14 +- 18 files changed, 239 insertions(+), 217 deletions(-) diff --git a/python/CHANGELOG.md b/python/CHANGELOG.md index 446adad2e0..c4ffdb74c0 100644 --- a/python/CHANGELOG.md +++ b/python/CHANGELOG.md @@ -9,7 +9,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). #### Credentials from sift-cli profiles -`SiftClient` now reads the same `sift.toml` profiles that `sift-cli --profile` uses, so an environment configured once for the CLI works from Python with no arguments. +`SiftClient` now reads the same `sift.toml` profiles that `sift-cli --profile` uses. An environment that you configure once for the CLI works from Python with no arguments. ```python client = SiftClient() # the CLI's default profile @@ -17,13 +17,21 @@ client = SiftClient(profile="staging") # a named profile client = SiftClient.from_profile("staging") ``` -Arguments still win, then a profile named in code, then `SIFT_API_KEY` / `SIFT_GRPC_URI` / `SIFT_REST_URI` / `SIFT_APP_URL`, then the profile named by `SIFT_PROFILE`, then the config file's default profile. `client.credential_sources` reports which layer supplied each value. See [Credentials & Profiles](guides/credentials.md). +Precedence, highest first: -The pytest plugin gains `--sift-profile`, the `sift_profile` ini key, and `SIFT_PROFILE`. There, the plugin's existing surfaces still outrank the profile, which fills in whatever they leave unset, so CI-injected values stay authoritative. +1. Arguments. +2. A profile that you name in code. +3. `SIFT_API_KEY`, `SIFT_GRPC_URI`, `SIFT_REST_URI`, and `SIFT_APP_URL`. +4. The profile that `SIFT_PROFILE` names. +5. The config file's default profile. -Passing an `http://` URL now connects without TLS instead of failing: transport security follows the gRPC URL's scheme. `https://` and bare host names are unaffected. +`client.credential_sources` reports the layer that supplied each value. See [Credentials and profiles](guides/credentials.md). -In the config file the API key is spelled `api_key`, matching the rest of Sift's API. The older `apikey` is still accepted everywhere and needs no migration; `api_key` wins if a profile carries both. +The pytest plugin gains `--sift-profile`, the `sift_profile` ini key, and `SIFT_PROFILE`. In the plugin, the existing settings surfaces outrank the profile. The profile supplies only the values that they leave unset, so a key that CI injects stays in effect. + +An `http://` URL now connects without TLS instead of failing, because transport security follows the scheme of the gRPC URL. An `https://` URL and a bare host name behave as before. + +In the config file, the canonical spelling of the API key is `api_key`, which matches the rest of the Sift API. The older `apikey` spelling is still valid, so you do not need to migrate. If a profile holds both keys, the client uses `api_key`. #### List and get data imports diff --git a/python/docs/guides/credentials.md b/python/docs/guides/credentials.md index 69b4bd05bd..af2524a1b1 100644 --- a/python/docs/guides/credentials.md +++ b/python/docs/guides/credentials.md @@ -9,20 +9,20 @@ from sift_client import SiftClient client = SiftClient() ``` -To use a named environment, give it a profile name, the same one you pass to +To use a named environment, give the profile name that you pass to `sift-cli --profile`: ```python client = SiftClient(profile="staging") -# Equivalent, and easier to find in the docs: +# The same thing, and easier to find in the docs: client = SiftClient.from_profile("staging") ``` ## The config file -`sift-cli` keeps one or more profiles in a `sift.toml` under your user config -directory. Create and edit it with the CLI rather than by hand: +`sift-cli` keeps one or more profiles in a `sift.toml` file in your user config +directory. Use the CLI to create and edit that file: ```bash sift-cli config create @@ -30,8 +30,8 @@ sift-cli config update --profile staging sift-cli config where # prints the path ``` -The file looks like this. The top-level table is the default profile; each -named table is a profile: +The top-level table is the default profile. Each named table is one more +profile: ```toml grpc_uri = "https://api.siftstack.com" @@ -52,33 +52,33 @@ api_key = "local" ``` A profile does not inherit from the default profile. If `[staging]` has no -`api_key`, that is an error rather than a silent fall back to the default -profile's key, which would otherwise point your tests at one environment using -another environment's credentials. +`api_key`, the client reports an error. It does not fall back to the default +profile's key, because that key can point your tests at a different +environment. Use an `http://` scheme for a plaintext endpoint, as `[localdev]` does above. -The client reads the scheme to decide whether to use TLS. +The client reads the scheme to select TLS or plaintext. -Older files spell the key `apikey`, which is still accepted everywhere and needs -no migration. `api_key` is canonical, matches the rest of Sift's API, and is what -`sift-cli config update` writes; when a profile somehow carries both, `api_key` -wins. +Older files spell the key `apikey`. That spelling is still valid, so you do not +need to migrate. `api_key` is canonical, matches the rest of the Sift API, and +is what `sift-cli config update` writes. If a profile holds both keys, the +client uses `api_key`. ## Resolution order Highest precedence first: -1. Arguments you pass to `SiftClient`, per field. -2. The fields of a profile named by `profile=`. +1. Arguments that you pass to `SiftClient`, one field at a time. +2. The fields of a profile that you name with `profile=`. 3. The environment variables `SIFT_API_KEY`, `SIFT_GRPC_URI`, `SIFT_REST_URI`, and `SIFT_APP_URL`. -4. The fields of the profile named by the `SIFT_PROFILE` environment variable. +4. The fields of the profile that `SIFT_PROFILE` names. 5. The default (top-level) table of the config file. -Naming a profile in code outranks the environment, so `SiftClient(profile="prod")` -still reaches production in a shell that was pointed somewhere else. -`SIFT_PROFILE` does not, so CI can select a profile for its endpoints and still -inject the API key through `SIFT_API_KEY`: +A profile that you name in code outranks the environment. +`SiftClient(profile="prod")` therefore reaches production even in a shell that +points somewhere else. `SIFT_PROFILE` does not outrank the environment, so CI +can select a profile for its endpoints and still inject the API key: ```bash export SIFT_PROFILE=staging # endpoints from the staging profile @@ -86,23 +86,23 @@ export SIFT_API_KEY="$CI_SECRET" # key from the secret store pytest ``` -Only one profile is ever read. If both `profile=` and `SIFT_PROFILE` are set, -the argument wins and the other profile is ignored entirely. +The client reads one profile at most. If you set both `profile=` and +`SIFT_PROFILE`, the client uses `profile=` and ignores the other profile. -## Where the file is looked for +## Where the client looks for the file -1. `SIFT_CONFIG_FILE`, when set, is used directly. -2. Otherwise the user config directory: `$XDG_CONFIG_HOME/sift.toml` (or - `~/.config/sift.toml`) on Linux, `~/Library/Application Support/sift.toml` - on macOS, and `%APPDATA%\sift.toml` on Windows. +1. If `SIFT_CONFIG_FILE` is set, the client uses that path. +2. If not, the client uses your user config directory: + `$XDG_CONFIG_HOME/sift.toml` (or `~/.config/sift.toml`) on Linux, + `~/Library/Application Support/sift.toml` on macOS, and + `%APPDATA%\sift.toml` on Windows. -The current working directory is not searched, so a `sift.toml` committed to a -repository you cloned cannot supply an API key. +The client does not search the current working directory. A `sift.toml` file in +a repository that you cloned therefore cannot supply an API key. -## Checking what a client resolved +## Check what a client resolved -`credential_sources` reports which layer supplied each value, which is usually -faster than re-deriving the precedence by hand: +`credential_sources` reports the layer that supplied each value: ```python client = SiftClient(profile="staging") @@ -110,15 +110,15 @@ client.profile # 'staging' client.credential_sources # {'grpc_url': 'profile:staging', 'api_key': 'env', ...} ``` -Each value is `arg`, `profile:`, `env`, `default`, or `unset`. Both are -`None` when the client was built from an explicit `connection_config`, which -bypasses resolution entirely. +Each value is `arg`, `profile:`, `env`, `default`, or `unset`. Both +properties are `None` if you build the client from an explicit +`connection_config`, because that path does not resolve credentials. -## Passing credentials directly +## Pass credentials directly -Explicit arguments and `connection_config` work exactly as before. Use them -when credentials come from somewhere the resolver does not know about, such as -a secrets manager: +Explicit arguments and `connection_config` work as before. Use them if the +credentials come from a source that the resolver does not read, such as a +secrets manager: ```python client = SiftClient( @@ -130,19 +130,21 @@ client = SiftClient( ## Errors -When the API key or either URL cannot be resolved, `SiftClient` raises -`SiftCredentialsError`, which subclasses `ValueError`. The message names the -missing variables, the file and profile it looked in, the profiles that file -defines, and the `sift-cli` command that sets them. +If `SiftClient` cannot resolve the API key or either URL, it raises +`SiftCredentialsError`, a subclass of `ValueError`. The message names the +missing variables and the file and profile that the client read. It also lists +the profiles in that file and gives the `sift-cli` command that sets the +missing values. ## Use with pytest -The pytest plugin reads the same profiles. See -[Configuration & Defaults](pytest_plugin/configuration.md) for the plugin's own -settings, and note one difference: in the plugin, the plugin's existing -surfaces (environment variables, `--sift-*` flags, and the -`sift_grpc_uri` / `sift_rest_uri` ini keys) all outrank the profile, which -fills in whatever they leave unset. That keeps CI-injected values authoritative. +The pytest plugin reads the same profiles. For the plugin's own settings, see +[Configuration & Defaults](pytest_plugin/configuration.md). + +The plugin uses a different precedence order. Its environment variables, +`--sift-*` flags, and `sift_grpc_uri` / `sift_rest_uri` ini keys all outrank the +profile. The profile supplies only the values that they leave unset, so a key +that CI injects stays in effect. ```bash pytest --sift-profile staging diff --git a/python/docs/guides/index.md b/python/docs/guides/index.md index a932c93716..4181bad3f2 100644 --- a/python/docs/guides/index.md +++ b/python/docs/guides/index.md @@ -6,8 +6,8 @@ works and how to configure it. For runnable, end-to-end walkthroughs see the ## Available guides -- [Credentials & Profiles](credentials.md): how `SiftClient` resolves its API key - and endpoints from arguments, environment variables, and the `sift.toml` +- [Credentials and profiles](credentials.md): how `SiftClient` resolves its API + key and endpoints from arguments, environment variables, and the `sift.toml` profiles that `sift-cli` manages. - [Pytest Plugin](pytest_plugin/index.md): turn a pytest run into a `TestReport` in Sift. Each test becomes a `TestStep`, measurements are recorded as rows, and diff --git a/python/docs/guides/pytest_plugin/configuration.md b/python/docs/guides/pytest_plugin/configuration.md index 6da4963e10..fc4d5e88f8 100644 --- a/python/docs/guides/pytest_plugin/configuration.md +++ b/python/docs/guides/pytest_plugin/configuration.md @@ -163,7 +163,7 @@ suggestion, so typos like `SIFT_REPORT_SERIALNUM` surface immediately. | Setting | CLI flag | Ini (`[tool.pytest.ini_options]`) | Env var | |---|---|---|---| -| Named sift.toml profile to draw credentials from, as used by `sift-cli --profile`. | `--sift-profile` | `sift_profile` | `SIFT_PROFILE` | +| Name of a sift.toml profile that supplies credentials, as with `sift-cli --profile`. | `--sift-profile` | `sift_profile` | `SIFT_PROFILE` | | Sift API key (secret, env-only). | — | — | `SIFT_API_KEY` | | Sift gRPC endpoint URI. | — | `sift_grpc_uri` | `SIFT_GRPC_URI` | | Sift REST endpoint URI. | — | `sift_rest_uri` | `SIFT_REST_URI` | diff --git a/python/lib/sift_client/_internal/credentials.py b/python/lib/sift_client/_internal/credentials.py index 47cf1250e5..e8c24d9496 100644 --- a/python/lib/sift_client/_internal/credentials.py +++ b/python/lib/sift_client/_internal/credentials.py @@ -1,10 +1,10 @@ """Resolution of Sift credentials from arguments, environment, and ``sift.toml``. -``sift-cli`` stores one or more named profiles in a ``sift.toml`` under the +``sift-cli`` stores one or more named profiles in a ``sift.toml`` file in the user's config directory, and selects between them with ``--profile``. This -module lets the Python client read that same file, so a developer who already -runs ``sift-cli --profile staging`` gets the same endpoints from -``SiftClient(profile="staging")`` without restating them. +module reads that same file. A developer who runs ``sift-cli --profile staging`` +gets the same endpoints from ``SiftClient(profile="staging")`` and does not +restate them. The resolution order, highest precedence first: @@ -15,14 +15,14 @@ 4. The fields of the profile named by ``SIFT_PROFILE``. 5. The default (top-level) table of the config file. -Naming a profile explicitly outranks the ambient environment variables so that -an argument beats a shell that was pointed somewhere else; ``SIFT_PROFILE`` -does not, so per-field environment overrides still work in CI. +A profile that the caller names outranks the environment variables, so an +argument wins over a shell that points somewhere else. ``SIFT_PROFILE`` does +not outrank them, so a per-field environment override still works in CI. -At most one profile table is ever consulted. A named profile does not inherit -missing fields from the top-level table, matching ``sift-cli``: a profile that -omits ``api_key`` is an error rather than a silent fall back to the default -profile's key. +The resolver reads one profile table at most. A named profile does not inherit +missing fields from the top-level table, which matches ``sift-cli``. If a +profile omits ``api_key``, the resolver reports an error. It does not fall back +to the default profile's key. """ from __future__ import annotations @@ -35,8 +35,8 @@ from typing import Any, Mapping from urllib.parse import urlparse -# Shared with the ``[tool.sift]`` loader so the 3.8-3.10 ``tomli`` fallback is -# declared once. +# The ``[tool.sift]`` loader declares the 3.8-3.10 ``tomli`` fallback. Import it +# from there so only one module declares it. from sift_client._internal.pyproject_config import tomllib from sift_client.errors import SiftCredentialsError, SiftWarning @@ -50,9 +50,10 @@ ENV_APP_URL = "SIFT_APP_URL" # Accepted TOML keys -> (public field name, environment variable). The first -# TOML key is canonical, the rest are accepted spellings kept so files written by -# earlier releases keep working. The env spellings are the ones the pytest plugin -# already ships; they differ from the TOML keys for the app URL and both stay. +# TOML key is canonical. The rest are older spellings, which stay valid so that +# files from earlier releases still work. The environment names are the ones the +# pytest plugin ships. They differ from the TOML keys for the app URL, and both +# names stay. _FIELDS = ( (("grpc_uri",), "grpc_url", ENV_GRPC_URI), (("rest_uri",), "rest_url", ENV_REST_URI), @@ -60,9 +61,9 @@ (("api_key", "apikey"), "api_key", ENV_API_KEY), ) -#: Every ``SIFT_*`` variable this module reads. The pytest plugin unions this -#: with its own registry so its unknown-variable warning doesn't flag one of -#: these as a typo. +#: Every ``SIFT_*`` variable that this module reads. The pytest plugin adds this +#: list to its own registry, so its unknown-variable warning does not report one +#: of these names as a typo. CREDENTIAL_ENV_VARS = (ENV_PROFILE, ENV_CONFIG_FILE, *(env for _, _, env in _FIELDS)) _REQUIRED = ("grpc_url", "rest_url", "api_key") @@ -75,9 +76,9 @@ class ResolvedCredentials: """Credentials resolved from arguments, environment, and config file. ``sources`` maps each field name to the layer that supplied it: ``"arg"``, - ``"profile:"``, ``"env"``, ``"default"``, or ``"unset"``. It answers - "which environment am I actually talking to" without re-deriving the - precedence by hand. + ``"profile:"``, ``"env"``, ``"default"``, or ``"unset"``. Use it to + find which environment the client connects to. You then do not have to + work through the precedence order by hand. """ api_key: str @@ -92,14 +93,15 @@ class ResolvedCredentials: def user_config_dir(env: Mapping[str, str] | None = None) -> Path | None: """The directory ``sift-cli`` stores ``sift.toml`` in. - Mirrors Rust's ``dirs::config_dir()``, which is what ``sift-cli`` uses: - ``%APPDATA%`` on Windows, ``~/Library/Application Support`` on macOS, and - ``$XDG_CONFIG_HOME`` (when absolute) or ``~/.config`` elsewhere. Returns - ``None`` when the home directory cannot be determined. + This function matches Rust's ``dirs::config_dir()``, which ``sift-cli`` + uses. That directory is ``%APPDATA%`` on Windows and + ``~/Library/Application Support`` on macOS. Elsewhere it is + ``$XDG_CONFIG_HOME`` when that path is absolute, and ``~/.config`` if not. + Returns ``None`` if this function cannot find the home directory. - This is deliberately hand-rolled rather than delegated to ``platformdirs``, - whose default app-name suffix would put the file somewhere ``sift-cli`` - never looks. + Note: ``platformdirs`` is not used here. It appends an application name by + default, which puts the file in a directory that ``sift-cli`` never + reads. """ environ = os.environ if env is None else env @@ -134,10 +136,12 @@ def config_file_path( ) -> Path | None: """Where to look for ``sift.toml``. - An explicit path wins, then ``SIFT_CONFIG_FILE``, then the user config - directory. There is deliberately no search of the current working - directory: a ``sift.toml`` inside a checkout would let a cloned repository - supply an API key, which needs its own decision before it ships. + An explicit path takes precedence, then ``SIFT_CONFIG_FILE``, then the user + config directory. + + Note: this function does not search the current working directory. A + ``sift.toml`` file in a checkout can supply an API key, so that behavior + needs its own decision before it ships. """ if config_path is not None: return Path(config_path) @@ -150,12 +154,12 @@ def config_file_path( def _load_config(path: Path | None) -> dict[str, Any]: - """Parse the config file, or return ``{}`` when there isn't one. + """Parse the config file, or return ``{}`` if there is no file. - A missing file is not an error on its own, since arguments or environment - variables may supply everything. A file that exists but cannot be read or - parsed does raise: silently ignoring it would surface later as a confusing - "credentials missing" rather than the syntax error it is. + A missing file is not an error on its own, because arguments or environment + variables can supply every field. A file that exists but that this function + cannot read or parse does raise. If it did not raise, the caller would later + see a confusing "credentials missing" error instead of the syntax error. """ if path is None: return {} @@ -202,7 +206,7 @@ def _profile_table( def _first_present(table: Mapping[str, Any], keys: tuple[str, ...]) -> Any: - """The value of the first of ``keys`` set in ``table``, canonical spelling first.""" + """The value of the first key in ``keys`` that ``table`` sets.""" for key in keys: value = table.get(key) if _str_or_none(value) is not None: @@ -211,7 +215,7 @@ def _first_present(table: Mapping[str, Any], keys: tuple[str, ...]) -> Any: def _str_or_none(value: Any) -> str | None: - """Coerce a layer's value, treating empty and non-string values as absent.""" + """Return a layer's value. Empty and non-string values count as absent.""" if isinstance(value, str) and value: return value return None @@ -220,10 +224,10 @@ def _str_or_none(value: Any) -> str | None: def _derive_use_ssl(grpc_url: str, rest_url: str) -> bool: """Infer transport security from the gRPC URL's scheme. - ``sift_py`` strips the scheme off the URI and decides plaintext vs TLS from - ``use_ssl`` alone, so a profile's ``http://localhost:50051`` would otherwise - be dialed over TLS and fail. A bare host with no scheme keeps the TLS - default. + ``sift_py`` removes the scheme from the URI. It then selects plaintext or + TLS from ``use_ssl`` alone. Without this function, a profile that holds + ``http://localhost:50051`` gets a TLS connection to a plaintext port, and + the connection fails. A bare host with no scheme keeps the TLS default. """ grpc_scheme = urlparse(grpc_url).scheme rest_scheme = urlparse(rest_url).scheme @@ -244,7 +248,7 @@ def _select_profile( profile: str | None, environ: Mapping[str, str], ) -> tuple[str | None, bool]: - """The profile to read, and whether it was named explicitly rather than by env.""" + """The profile to read, and whether the caller named it.""" if profile: return profile, True return environ.get(ENV_PROFILE) or None, False @@ -267,21 +271,22 @@ def resolve_credentials( grpc_url: Explicit gRPC endpoint, overriding every other layer. rest_url: Explicit REST endpoint, overriding every other layer. app_url: Explicit Sift web-app origin, overriding every other layer. - profile: Name of a profile in the config file. Outranks the per-field - environment variables; see the module docstring. + profile: Name of a profile in the config file. It outranks the + per-field environment variables. See the module docstring. config_path: Path to a specific config file, bypassing discovery. env: Environment mapping to read, defaulting to ``os.environ``. - require: When ``True``, raise if the API key or either URL is still - missing. Pass ``False`` to resolve as much as is available and - leave the rest empty, as the pytest plugin's offline mode does. + require: If ``True``, raise when the API key or either URL is still + missing. Pass ``False`` to resolve every field that is available + and leave the rest empty. The pytest plugin uses ``False`` for its + offline mode. Returns: The resolved credentials, including which layer supplied each field. Raises: - SiftCredentialsError: The config file is unreadable or malformed, the - named profile does not exist, or (when ``require``) a required - field could not be resolved. + SiftCredentialsError: This function cannot read or parse the config + file, the named profile does not exist, or ``require`` is ``True`` + and a required field is still missing. """ environ = os.environ if env is None else env profile_name, profile_is_explicit = _select_profile(profile, environ) @@ -296,14 +301,14 @@ def resolve_credentials( table = config file_source = "default" - # Every layer is keyed by field name, so picking a value never depends on - # which layer it came from. + # Each layer uses the field name as its key. The choice of a value therefore + # does not depend on the layer that holds it. arg_layer = {"grpc_url": grpc_url, "rest_url": rest_url, "app_url": app_url, "api_key": api_key} env_layer = {field: environ.get(env_key) for _, field, env_key in _FIELDS} file_layer = {field: _first_present(table, toml_keys) for toml_keys, field, _ in _FIELDS} - # Highest precedence first. A profile named explicitly outranks the ambient - # environment; one named by SIFT_PROFILE does not. + # Highest precedence first. A profile that the caller names outranks the + # environment. A profile that SIFT_PROFILE names does not. if profile_is_explicit: layers = [("arg", arg_layer), (file_source, file_layer), ("env", env_layer)] else: @@ -347,7 +352,7 @@ def _missing_message( path: Path | None, config: Mapping[str, Any], ) -> str: - """Explain what is missing, where it was looked for, and how to supply it.""" + """Explain what is missing, where the resolver looked, and how to supply it.""" env_names = {field_name: env_key for _, field_name, env_key in _FIELDS} wanted = ", ".join(env_names[name] for name in missing) diff --git a/python/lib/sift_client/_internal/pytest_plugin/options.py b/python/lib/sift_client/_internal/pytest_plugin/options.py index 57da77553c..6514866f15 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/options.py +++ b/python/lib/sift_client/_internal/pytest_plugin/options.py @@ -75,8 +75,9 @@ class Option: - ``toml``: tuple path under ``[tool.sift...]``, e.g. ``("pytest", "report", "name")`` -> ``tool.sift.pytest.report.name``. - ``env``: full env var name, e.g. ``"SIFT_API_KEY"``. - - ``surfaces``: precedence order, defaulting to env before cli. Override it - where a typed flag should beat an ambient env var, as ``profile`` does. + - ``surfaces``: the precedence order. The default puts env before cli. + Override it if a flag that the user types must outrank an environment + variable, as ``profile`` does. ``category`` groups the option in the docs reference (one of ``CATEGORIES``). """ @@ -129,7 +130,7 @@ def __post_init__(self) -> None: def resolve(self, config: pytest.Config | None) -> Any: """First set value from declared surfaces; ``None`` when unset everywhere. - Walk order is :attr:`surfaces`, env before cli by default. + The walk order is :attr:`surfaces`, which puts env before cli by default. ``getini`` returns the typed default for unset bool/list keys, so this only returns ini values for booleans (always meaningful), non-empty strings, and non-empty lists. @@ -140,9 +141,9 @@ def resolve_with_source(self, config: pytest.Config | None) -> tuple[Any, str]: """Like :meth:`resolve`, but also reports which surface set the value. Returns ``(value, source)`` where ``source`` is one of - ``env``/``cli``/``ini``/``toml``, or ``default`` when nothing set it - (``value`` is then ``None``). Used by the audit log's settings snapshot, - which therefore always reports the surface the run actually used. + ``env``/``cli``/``ini``/``toml``, or ``default`` if nothing set it + (``value`` is then ``None``). The audit log's settings snapshot calls + this method, so it always reports the surface that the run used. """ for surface in self.surfaces: value = self._read_surface(surface, config) @@ -358,13 +359,13 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: ini_default=True, ) -# Credentials. The API key is env-only; the URIs accept env + ini. A profile -# supplies whatever the other three leave unset, from the same sift.toml that -# `sift-cli --profile` reads. +# Credentials. The API key has no ini key. The URIs accept env and ini. A +# profile supplies the values that the other three leave unset, from the same +# sift.toml that `sift-cli --profile` reads. PROFILE_OPTION = Option( name="profile", category=CAT_CONNECTION, - help="Named sift.toml profile to draw credentials from, as used by `sift-cli --profile`.", + help="Name of a sift.toml profile that supplies credentials, as with `sift-cli --profile`.", cli="--sift-profile", env=ENV_PROFILE, ini="sift_profile", @@ -595,12 +596,12 @@ def _escape(cell: str) -> str: def warn_on_unknown_env_vars() -> None: - """Emit a warning for any ``SIFT_*`` env var this plugin doesn't read. + """Emit a warning for any ``SIFT_*`` env var that this plugin does not read. - Known names are the registry's (``opt.env``) plus the credential resolver's - ``CREDENTIAL_ENV_VARS``, which includes variables like ``SIFT_CONFIG_FILE`` - that the resolver honors without the registry declaring them. A ``SIFT_*`` - var matching neither is almost always a typo. + The known names are the registry's (``opt.env``) and the credential + resolver's ``CREDENTIAL_ENV_VARS``. The resolver reads some names, such as + ``SIFT_CONFIG_FILE``, that the registry does not declare. A ``SIFT_*`` + variable in neither list is almost always a typo. """ import difflib diff --git a/python/lib/sift_client/_tests/conftest.py b/python/lib/sift_client/_tests/conftest.py index ecf94b6b24..23f2dd7a62 100644 --- a/python/lib/sift_client/_tests/conftest.py +++ b/python/lib/sift_client/_tests/conftest.py @@ -38,10 +38,11 @@ def _isolate_sift_config_file(monkeypatch, tmp_path_factory): """Point credential resolution at a config file that does not exist. ``SiftClient()`` without a ``connection_config`` reads ``sift.toml`` from - the user config directory, so without this a developer's real - ``~/.config/sift.toml`` would supply credentials a test never set — and - could point a test at a live backend. The plugin suite relies on it too: - its inner sessions inherit this environment through ``runpytest_subprocess``. + the user config directory. Without this fixture, a developer's real + ``~/.config/sift.toml`` supplies credentials that a test never set, and it + can point a test at a live backend. The plugin suite needs this fixture too, + because its inner sessions inherit this environment through + ``runpytest_subprocess``. """ absent = tmp_path_factory.mktemp("sift-config") / "absent.toml" monkeypatch.setenv("SIFT_CONFIG_FILE", str(absent)) diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py b/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py index 6ac2e3757e..3a34367840 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_credentials.py @@ -222,7 +222,7 @@ def test_unknown_profile_is_a_usage_error( monkeypatch: pytest.MonkeyPatch, write_plugin_conftest: Callable[[], None], ) -> None: - """A named profile that isn't in the file aborts and lists the ones that are.""" + """A named profile that the file does not define aborts and lists the ones it does.""" self._write_config(pytester, monkeypatch) write_plugin_conftest() pytester.makepyfile("def test_should_not_run(): pass") diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_typo_detector.py b/python/lib/sift_client/_tests/pytest_plugin/test_typo_detector.py index aea9d04719..b5bfa7fbbf 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_typo_detector.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_typo_detector.py @@ -121,9 +121,9 @@ def test_credential_resolver_env_vars_are_known( ) -> None: """``SIFT_CONFIG_FILE`` is honored by the resolver, so it must not read as a typo. - The registry does not declare it; the known set unions the credential - resolver's own variables so the warning can't tell a user that a - variable the run obeyed was ignored. + The registry does not declare it. The known set therefore includes the + credential resolver's own variables, so the warning cannot tell a user + that the run ignored a variable that it read. """ monkeypatch.setenv("SIFT_CONFIG_FILE", str(pytester.path / "absent.toml")) write_plugin_conftest() diff --git a/python/lib/sift_client/_tests/test_credentials.py b/python/lib/sift_client/_tests/test_credentials.py index c9dca11862..1c71960143 100644 --- a/python/lib/sift_client/_tests/test_credentials.py +++ b/python/lib/sift_client/_tests/test_credentials.py @@ -166,8 +166,10 @@ def test_legacy_apikey_still_accepted(self, config_file): assert resolve(config_file).api_key == "default-key" def test_canonical_wins_when_a_file_carries_both(self, tmp_path): - """`sift-cli config update` drops the legacy key, but a hand-edited - file can hold both; the canonical spelling decides. + """A file that someone edits by hand can hold both keys. + + `sift-cli config update` never writes both. If both are present, the + canonical spelling decides. """ path = tmp_path / "sift.toml" path.write_text( diff --git a/python/lib/sift_client/client.py b/python/lib/sift_client/client.py index c5d1c18e05..5c1c49551c 100644 --- a/python/lib/sift_client/client.py +++ b/python/lib/sift_client/client.py @@ -72,7 +72,7 @@ class SiftClient( from sift_client import SiftClient from datetime import datetime - # Use the same credentials sift-cli uses, from its default profile + # Use the same credentials as sift-cli, from its default profile client = SiftClient() # Or a named profile from sift.toml, like `sift-cli --profile staging` @@ -163,10 +163,10 @@ def __init__( ): """Initialize the SiftClient with specific connection parameters or a connection_config. - Any argument left unset is resolved from the environment and from the - ``sift.toml`` profiles that ``sift-cli`` manages, so ``SiftClient()`` - connects to the same place as ``sift-cli`` with no arguments at all. - See :func:`sift_client.credentials.resolve_credentials` for the full + For any argument that you leave unset, the client reads the environment + and the ``sift.toml`` profiles that ``sift-cli`` manages. ``SiftClient()`` + therefore connects to the same endpoints as ``sift-cli``. See + :func:`sift_client.credentials.resolve_credentials` for the full precedence order. Args: @@ -174,18 +174,18 @@ def __init__( grpc_url: The Sift gRPC API URL. rest_url: The Sift REST API URL. connection_config: A SiftConnectionConfig object to configure the connection behavior of the SiftClient. - When given, it is used as-is and no credential resolution happens. + The client uses it unchanged and resolves no credentials. app_url: The Sift web-app origin (e.g. ``https://app.siftstack.com``). Set this for on-prem or custom deployments whose API host can't be mapped to a frontend automatically; see the ``app_url`` property. A value here takes precedence over ``connection_config.app_url``. - profile: Name of a ``sift.toml`` profile to draw credentials from, - equivalent to ``sift-cli --profile``. Ignored when - ``connection_config`` is given. + profile: Name of a ``sift.toml`` profile that supplies credentials, + as with ``sift-cli --profile``. The client ignores this argument + if you also pass ``connection_config``. Raises: - SiftCredentialsError: No ``connection_config`` was given and the API - key or either URL could not be resolved. + SiftCredentialsError: You passed no ``connection_config``, and the + client cannot resolve the API key or either URL. """ self._credentials: ResolvedCredentials | None = None @@ -199,9 +199,9 @@ def __init__( profile=profile, ) self._credentials = creds - # ``use_ssl`` comes from the gRPC URL's scheme: the transport strips - # the scheme off and would otherwise dial an ``http://`` endpoint - # over TLS. + # ``use_ssl`` comes from the scheme of the gRPC URL. The transport + # removes that scheme, so without this it connects to an ``http://`` + # endpoint over TLS. connection_config = SiftConnectionConfig( grpc_url=creds.grpc_url, rest_url=creds.rest_url, @@ -279,8 +279,9 @@ def __init__( def from_profile(cls, profile: str, **kwargs) -> SiftClient: """Build a client from a named ``sift.toml`` profile. - Equivalent to ``SiftClient(profile=...)``; keyword arguments are passed - through and still take precedence over the profile's values. + This method calls ``SiftClient(profile=...)``. It passes every keyword + argument through, and those arguments still outrank the profile's + values. Args: profile: Profile name, as used by ``sift-cli --profile``. @@ -293,18 +294,19 @@ def from_profile(cls, profile: str, **kwargs) -> SiftClient: @property def credential_sources(self) -> Mapping[str, str] | None: - """Which layer supplied each credential, for diagnosing connections. + """The layer that supplied each credential. Use it to diagnose a connection. Maps ``api_key`` / ``grpc_url`` / ``rest_url`` / ``app_url`` to ``"arg"``, ``"profile:"``, ``"env"``, ``"default"``, or - ``"unset"``. ``None`` when the client was built from an explicit - ``connection_config``, which bypasses resolution. + ``"unset"``. This property is ``None`` if you build the client from an + explicit ``connection_config``, because that path resolves no + credentials. """ return self._credentials.sources if self._credentials else None @property def profile(self) -> str | None: - """The ``sift.toml`` profile this client resolved its credentials from.""" + """The ``sift.toml`` profile that supplied this client's credentials.""" return self._credentials.profile if self._credentials else None @property diff --git a/python/lib/sift_client/credentials.py b/python/lib/sift_client/credentials.py index da0cf632e9..c5a2ba4a43 100644 --- a/python/lib/sift_client/credentials.py +++ b/python/lib/sift_client/credentials.py @@ -1,12 +1,12 @@ """Credential resolution shared by ``SiftClient`` and the pytest plugin. -Reads the same ``sift.toml`` profiles that ``sift-cli --profile`` uses, so an -environment configured once for the CLI is available to the Python client -without restating its endpoints. See :func:`resolve_credentials` for the -precedence order, and the Credentials & Profiles guide for the config file's -shape. +This module reads the same ``sift.toml`` profiles that ``sift-cli --profile`` +uses. An environment that you configure once for the CLI is then available to +the Python client, and you do not restate its endpoints. See +:func:`resolve_credentials` for the precedence order. The Credentials and +profiles guide describes the config file. -This is the public surface; the implementation lives in +This module is the public surface. The implementation is in ``sift_client._internal.credentials``. """ diff --git a/python/lib/sift_client/pytest_plugin.py b/python/lib/sift_client/pytest_plugin.py index aa7d6bdd2f..a143064ba5 100644 --- a/python/lib/sift_client/pytest_plugin.py +++ b/python/lib/sift_client/pytest_plugin.py @@ -187,39 +187,39 @@ def abort(reason: str, returncode: int | None = None) -> NoReturn: def sift_client(pytestconfig: pytest.Config) -> SiftClient: """Default ``SiftClient`` resolved from env vars, ini keys, and sift.toml profiles. - Each credential is read from its environment variable first. The URIs - (``SIFT_GRPC_URI``, ``SIFT_REST_URI``) also fall back to the - ``sift_grpc_uri`` / ``sift_rest_uri`` ini keys, since they are stable - per-org values that are safe to commit. ``SIFT_API_KEY`` is intentionally - env-only; use ``pytest-dotenv`` (already a project dependency) to load - it from a ``.env`` file kept out of version control. - - Anything those surfaces leave unset is filled from a ``sift.toml`` profile, - the same file ``sift-cli --profile`` reads. Name one with ``--sift-profile``, - the ``sift_profile`` ini key, or ``SIFT_PROFILE``; with none named, the - file's default profile is used. Unlike :class:`~sift_client.SiftClient`, - here the profile sits *below* the env vars rather than above them, so a key - injected by CI is never overridden by a profile on the runner. - - Projects that need custom construction (TLS toggles, custom timeouts, - etc.) can override this fixture by defining their own ``sift_client`` - in their ``conftest.py``; pytest fixture resolution prefers the local - definition. - - In ``--sift-offline`` mode the missing-credential check is relaxed: - real env vars and ini values still win when set (so the client is - constructible against a real backend even though no calls are made), but - anything still missing is filled with a placeholder. In ``--sift-disabled`` - mode the credential resolution is skipped entirely and placeholders are - always used. + This fixture reads each credential from its environment variable first. The + URIs (``SIFT_GRPC_URI``, ``SIFT_REST_URI``) also fall back to the + ``sift_grpc_uri`` / ``sift_rest_uri`` ini keys, because those are stable + per-org values that are safe to commit. ``SIFT_API_KEY`` has no ini key, by + design. Use ``pytest-dotenv``, which is already a project dependency, to + load it from a ``.env`` file that you keep out of version control. + + A ``sift.toml`` profile supplies the values that those surfaces leave unset. + It is the same file that ``sift-cli --profile`` reads. Name a profile with + ``--sift-profile``, the ``sift_profile`` ini key, or ``SIFT_PROFILE``. If you + name none, the fixture uses the file's default profile. + + Note: the precedence here differs from :class:`~sift_client.SiftClient`. The + profile ranks below the environment variables, not above them, so a profile + on the runner cannot replace a key that CI injects. + + A project that needs custom construction, such as a TLS toggle or a custom + timeout, can override this fixture. Declare your own ``sift_client`` in + ``conftest.py``, because pytest prefers the local definition. + + ``--sift-offline`` mode relaxes the missing-credential check. A real + environment variable or ini value still takes precedence when it is set, so + the client can point at a real backend even though it makes no calls. A + placeholder fills every field that is still missing. ``--sift-disabled`` + mode resolves no credentials and always uses placeholders. """ if is_disabled(pytestconfig): return build_disabled_client() offline = is_offline(pytestconfig) - # Everything the plugin's own surfaces resolved is passed as an explicit - # argument, so it outranks the profile; the profile fills only what they - # left unset. That keeps CI-injected env vars authoritative. + # Every value that the plugin's own surfaces resolved goes in as an explicit + # argument, so it outranks the profile. The profile then supplies only the + # values that they left unset, which keeps a key from CI in effect. try: creds = resolve_credentials( api_key=API_KEY_OPTION.resolve(pytestconfig), @@ -230,8 +230,9 @@ def sift_client(pytestconfig: pytest.Config) -> SiftClient: require=False, ) except SiftCredentialsError as exc: - # A named profile that doesn't exist, or an unreadable config file, is a - # usage error even offline: the run asked for something specific. + # A named profile that does not exist, or a config file that the resolver + # cannot read, is a usage error even offline, because the run asked for + # something specific. log_event(logger, logging.ERROR, "credentials", error=type(exc).__name__) raise pytest.UsageError(str(exc)) from exc diff --git a/python/mkdocs.yml b/python/mkdocs.yml index 728d92aa81..589c7d58da 100644 --- a/python/mkdocs.yml +++ b/python/mkdocs.yml @@ -66,7 +66,7 @@ nav: - Pytest Plugin Quickstart: examples/pytest_plugin_quickstart.md - Guides: - guides/index.md - - Credentials & Profiles: guides/credentials.md + - Credentials and profiles: guides/credentials.md - Pytest Plugin: - Overview: guides/pytest_plugin/index.md - Configuration & Defaults: guides/pytest_plugin/configuration.md diff --git a/rust/crates/sift_cli/src/cmd/config/mod.rs b/rust/crates/sift_cli/src/cmd/config/mod.rs index fd8f60906f..378fb917cf 100644 --- a/rust/crates/sift_cli/src/cmd/config/mod.rs +++ b/rust/crates/sift_cli/src/cmd/config/mod.rs @@ -246,8 +246,8 @@ fn apply_profile_updates( target.insert(String::from("rest_uri"), Value::String(uri)); } if let Some(token) = api_key { - // Write the canonical spelling and drop the legacy one, so a profile - // never carries both and readers never have to pick a winner. + // Write the canonical spelling and remove the older one, so a profile + // never holds both keys and no reader has to choose between them. target.remove(API_KEY_KEY_LEGACY); target.insert(String::from(API_KEY_KEY), Value::String(token)); } diff --git a/rust/crates/sift_cli/src/cmd/config/tests.rs b/rust/crates/sift_cli/src/cmd/config/tests.rs index 120ab812ae..797885e1d2 100644 --- a/rust/crates/sift_cli/src/cmd/config/tests.rs +++ b/rust/crates/sift_cli/src/cmd/config/tests.rs @@ -232,7 +232,7 @@ mod api_key_key { assert_eq!(config[API_KEY_KEY].as_str(), Some("new")); assert!( !config.contains_key(API_KEY_KEY_LEGACY), - "the legacy key must be removed so readers never see two spellings" + "the older key must go, so that no reader sees two spellings" ); } @@ -245,7 +245,7 @@ mod api_key_key { assert_eq!(mission[API_KEY_KEY].as_str(), Some("new")); assert!(!mission.contains_key(API_KEY_KEY_LEGACY)); - // The default profile is untouched, legacy spelling included. + // The default profile does not change, and keeps the older spelling. assert_eq!(config[API_KEY_KEY_LEGACY].as_str(), Some("top")); } } diff --git a/rust/crates/sift_cli/src/cmd/mod.rs b/rust/crates/sift_cli/src/cmd/mod.rs index 9284c32efa..c11e8b055f 100644 --- a/rust/crates/sift_cli/src/cmd/mod.rs +++ b/rust/crates/sift_cli/src/cmd/mod.rs @@ -25,12 +25,12 @@ pub struct Context { pub app_uri: Option, } -/// The canonical TOML key for the API key, matching the `api_key` spelling the -/// Sift API itself uses. +/// The canonical TOML key for the API key. It matches the `api_key` spelling +/// that the Sift API uses. pub(super) const API_KEY_KEY: &str = "api_key"; -/// The original spelling, still accepted so configs written by older releases -/// keep working. Never written back. +/// The older spelling. It stays valid so that a config from an earlier release +/// still works. The CLI never writes it. pub(super) const API_KEY_KEY_LEGACY: &str = "apikey"; /// The profile's API key under either spelling, canonical first. @@ -242,8 +242,8 @@ app_uri = "https://app.siftstack.com" #[test] fn accepts_either_api_key_spelling() { - // COMPLETE_CONFIG uses `api_key` at the top level and the legacy - // `apikey` under [mission], so one load covers both. + // COMPLETE_CONFIG uses `api_key` at the top level and the older + // `apikey` under [mission], so one load covers both spellings. assert_eq!( context(COMPLETE_CONFIG, None).unwrap().api_key, "default-key" @@ -253,8 +253,8 @@ app_uri = "https://app.siftstack.com" "mission-key" ); - // `config update` never writes both, but a hand-edited file can hold - // both; the canonical spelling decides. + // `config update` never writes both keys. A file that someone edits by + // hand can hold both, and then the canonical spelling decides. let both = r#" grpc_uri = "https://grpc-api.siftstack.com" rest_uri = "https://api.siftstack.com" diff --git a/rust/crates/sift_connect/src/grpc/config.rs b/rust/crates/sift_connect/src/grpc/config.rs index 1620736e0d..9faeb59c1e 100644 --- a/rust/crates/sift_connect/src/grpc/config.rs +++ b/rust/crates/sift_connect/src/grpc/config.rs @@ -27,8 +27,8 @@ pub const SIFT_CONFIG_NAME: &str = "sift.toml"; /// api_key = "production-api-key" /// ``` /// -/// The legacy `apikey` spelling is still accepted for files written by earlier -/// releases; `api_key` wins when a table carries both. +/// The older `apikey` spelling is still valid, so a file from an earlier release +/// still works. If a table holds both keys, the loader uses `api_key`. /// /// # Direct Credentials /// @@ -141,16 +141,16 @@ impl TryFrom for SiftChannelConfig { } } -/// The canonical TOML key for the API key, matching the `api_key` spelling used -/// by the Sift API and by `sift-cli`, followed by the legacy `apikey` spelling -/// that earlier releases wrote. Lookups accept either; nothing writes the legacy -/// one. +/// The accepted TOML keys for the API key, canonical first. `api_key` matches +/// the spelling that the Sift API and `sift-cli` use. `apikey` is the older +/// spelling that earlier releases wrote. A lookup accepts either key. Nothing +/// writes the older one. pub const API_KEY_KEYS: &[&str] = &["api_key", "apikey"]; /// The TOML key naming the gRPC endpoint. pub const URI_KEY: &str = "uri"; -/// The first of `keys` present in `table` as a string, or `None`. +/// The value of the first key in `keys` that `table` sets, or `None`. fn lookup<'a>(table: &'a Table, keys: &[&str]) -> Option<&'a str> { keys.iter().find_map(|key| match table.get(*key) { Some(Value::String(value)) if !value.is_empty() => Some(value.as_str()),