Skip to content

refactor(config): typed Config model with consistent CLI/env/file/SSL handling#752

Open
TheKevJames wants to merge 13 commits into
masterfrom
kjames/config-cleanup
Open

refactor(config): typed Config model with consistent CLI/env/file/SSL handling#752
TheKevJames wants to merge 13 commits into
masterfrom
kjames/config-cleanup

Conversation

@TheKevJames

@TheKevJames TheKevJames commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Summary

Cleans up how coveralls-python handles configuration values (CLI flags, CI-system
environment detection, COVERALLS_* env vars, .coveralls.yml, and our own
settings such as disabling SSL verification). Introduces a single typed Config
model and a resolve() pipeline, replacing the untyped self.config dict that was
scattered across api.py.

Motivation

The old design merged three unrelated kinds of value into one untyped dict and then
dumped the whole dict into the submitted JSON. This leaked client-only settings
(base_dir, src_dir, rcfile, and the newly-added timeout family) into the
payload sent to coveralls.io. State was also spread across ad-hoc attributes
(_coveralls_host, _token_required), call-time os.environ reads (SSL/host), and
positional per-service tuples, and the CLI duplicated every option across two
commands.

What changed

New coveralls/configuration.py

  • Typed Config dataclass splitting API payload fields from client-behaviour
    settings
    , with to_payload() as the enforced boundary.
  • Normalized source functions merged by resolve() in the documented precedence
    order (CI env → COVERALLS_* → config file → explicit overrides). Positional
    per-service tuples removed; a single _parse_pr_number helper (trailing-integer)
    now serves the generic, circle and jenkins loaders.
  • token_required is AND-ed across sources (any source may waive it, none may
    re-impose it). Unknown config-file keys are dropped with a warning.

Fixes / internal

  • Client settings no longer leak into the submitted payload.
  • _coveralls_host / _token_required / _normalize_timeouts / _request_timeout
    and the call-time SSL/host env reads folded onto Config.
  • CLI defined once via shared Annotated option aliases; coveralls (default) and
    coveralls debug are thin wrappers over a shared _dispatch(), removing the
    copy-pasted option blocks while keeping the debug subcommand.
  • api.py and cli.py are now typed (mypy passes).
  • Minor: service_name KeyError sharp edge, re.IGNORECASE on a digit regex, a
    docstring typo.

Behaviour changes (for the changelog)

Naming convention introduced: every setting foo_bar is spelled --foo-bar
(CLI), COVERALLS_FOO_BAR (env), and foo_bar (YAML).

Breaking

  • Coveralls.config is now a typed Config object rather than a dict:
    cover.config['x']cover.config.x.
  • The public load_config / load_config_from_* methods were removed (logic
    moved to configuration.resolve).
  • Coveralls.__init__ signature is now (token_required=True, **kwargs);
    service_name is no longer a dedicated positional/keyword param (still accepted
    via **kwargs), so Coveralls(True, 'my-service') positionally no longer works.
  • The submitted JSON payload no longer contains client-only settings
    (base_dir, src_dir, rcfile/config_file, host, skip_ssl_verify, the
    timeout family) or unrecognised .coveralls.yml keys, all of which were
    previously merged into it.

Deprecated (still work, now emit a warning)

  • CLI flags --service--service-name, --basedir--base-dir,
    --srcdir--src-dir.
  • Config keys (.coveralls.yml and Coveralls() kwargs) coveralls_host
    host, config_filercfile.

New

  • CLI flags --host, --skip-ssl-verify, --parallel.
  • host and skip_ssl_verify are now first-class settings configurable through
    every channel (previously skip_ssl_verify was env-only and read at call time;
    host was env + an undocumented kwarg).

Other observable changes

  • --rcfile no longer defaults to .coveragerc; when unset, coverage.py's
    standard auto-discovery is used. (coverage.py treats an explicit .coveragerc
    and auto-discovery identically — including discovering pyproject.toml etc. —
    so this is functionally equivalent, but the value we pass / advertised default
    changed.) This also fixed a bug where the CLI's .coveragerc default clobbered
    an rcfile/config_file set in .coveralls.yml.
  • An empty or comment-only .coveralls.yml is now treated as no config instead of
    raising TypeError.
  • CircleCI/Jenkins pull-request parsing now uses the same trailing-integer rule as
    generic CI; a non-numeric trailing value yields no PR number instead of being
    forwarded verbatim.
  • New deprecation / unknown-key warnings are emitted to the coveralls logger.

Note on changelog generation: cliff.toml's commit_preprocessors strips
everything after the commit subject, so BREAKING CHANGE: footers are not
picked up — git-cliff renders only subjects plus a [**breaking**] marker driven
by a ! in the subject. None of the commits here use ! yet; see the open
question below.

Testing

  • New tests/api/config_test.py (Config unit tests, incl. payload-leak regression
    guards) and tests/api/resolve_test.py (per-source resolution + precedence, PR
    parsing, deprecated keys, empty-file handling).
  • configuration_test.py trimmed to Coveralls integration + ensure_token.
  • Full suite: 127 passed, 1 skipped. pre-commit run -a clean.

Open question

To get the breaking changes above to render as [**breaking**] in the git-cliff
changelog, the relevant commit subjects need a ! (e.g. refactor(api)!: ...).
That means amending already-pushed commits and force-pushing this branch — happy
to do so if you're OK with a force-push here, or to squash-merge with a curated
message instead. Let me know which you'd prefer.

Known follow-ups (out of scope)

  • --help still renders COMMAND [ARGS]... (Typer group); left as-is.
  • mypy --strict flip, exception __eq__ removal, the misleading resubmission
    test, and lockfile reconciliation remain separate concerns.

Introduce a typed home for coveralls-python configuration, splitting fields
into API payload fields (emitted via to_payload) and client-behaviour settings
that must never enter the submitted JSON. Absorbs the timeout validation and
(connect, read) derivation that previously lived as ad-hoc helpers on the
Coveralls class.

Not yet wired into Coveralls/CLI; subsequent commits move the resolution logic
onto this type.
Port the CI-service detection, COVERALLS_* env vars, and .coveralls.yml
loading into normalized source functions that each return a uniform partial
config, merged by resolve() in the documented precedence order into a typed
Config.

- Replace the per-service positional (name, job, number, pr) tuples with
  partial dicts and a shared _pr_from_path helper for URL-style PR parsing.
- token_required is AND-ed across sources: any source may waive it (TravisCI,
  debug/output) but none may re-impose it.
- Unknown keys from the config file are dropped with a warning, giving a clean
  migration path for the upcoming coveralls_host/config_file YAML renames.

Not yet wired into Coveralls; that swap lands next.
…o payload

Coveralls now resolves configuration into a typed Config via resolve() instead
of building an untyped dict, and create_data() emits only Config.to_payload()
(the API payload fields). This fixes base_dir/src_dir/rcfile and the timeout
family leaking into the submitted JSON.

- Delete the load_config_from_* loaders, _normalize_timeouts, _request_timeout,
  _coveralls_host and _token_required; all now live on Config/resolve.
- ensure_token, submit_report, parallel_finish and get_coverage read typed
  fields; the 422 branch uses config.service_name (never None post-resolve),
  removing the KeyError sharp edge.
- Rename the coverage rcfile constructor kwarg config_file -> rcfile.
- Trim configuration_test.py to Coveralls integration + ensure_token; the
  per-CI/per-timeout resolution is covered by resolve_test.py/config_test.py.
The call-time os.environ reads for these were removed when Coveralls moved onto
Config; add an override-path test to lock in that host and skip_ssl_verify are
settable through any channel, not only COVERALLS_HOST/COVERALLS_SKIP_SSL_VERIFY.
…d aliases

Collapse the duplicated coveralls()/debug() commands into one command with a
--debug flag, removing the copy-pasted option set (incl. the duplicated
--version callback and timeout flags).

- Add first-class --host, --skip-ssl-verify and --parallel flags.
- Rename --service/--basedir/--srcdir to --service-name/--base-dir/--src-dir,
  following the field/env/flag naming convention; keep the old spellings as
  hidden aliases that log a deprecation warning.
- Fully type cli.py; build Config overrides via a helper that only forwards
  explicitly-set boolean flags.
Update the configuration and troubleshooting docs for the single-command CLI,
the new --host/--skip-ssl-verify/--parallel flags, and the renamed
--service-name/--base-dir/--src-dir flags (with deprecation notes). Document
the .coveralls.yml key renames and clean the leaked config_file/base_dir keys
out of the example report fixture.

BREAKING CHANGE: configuration now uses one consistent name per setting across
CLI flags, COVERALLS_* env vars and .coveralls.yml. The 'coveralls_host' and
'config_file' config-file keys are renamed to 'host' and 'rcfile'; the
--service/--basedir/--srcdir flags are renamed to --service-name/--base-dir/
--src-dir (old spellings still work but are deprecated); the 'debug' subcommand
becomes a --debug flag; and the Coveralls(config_file=...) constructor kwarg is
renamed to rcfile. Client settings (base_dir, src_dir, rcfile, host,
skip_ssl_verify, timeouts) are no longer leaked into the submitted payload.
…a warning

Mirror the deprecated CLI flag aliases for config keys: translate the renamed
coveralls_host -> host and config_file -> rcfile keys to their canonical names
(warning on use) instead of dropping them. The same rule applies to both the
config file and Coveralls() keyword arguments, since both are dicts of
user-supplied config keys.

This restores backwards compatibility for existing .coveralls.yml files and for
Coveralls(config_file=...) callers, removing those breaking changes. An
explicit canonical key in the same source still takes precedence.
@TheKevJames

Copy link
Copy Markdown
Owner Author

Update: backwards-compat for renamed config keys

Applied the same deprecated-alias-with-warning treatment (used for CLI flags) to
the renamed config keys, via a single general rule in resolve() that
translates deprecated keys to canonical ones for both the config file and
Coveralls() kwargs:

  • .coveralls.yml: coveralls_host/config_file now work again (warn) instead
    of being dropped.
  • Coveralls(config_file=...) / Coveralls(coveralls_host=...) now work again
    (warn) instead of raising TypeError.
  • An explicit canonical key in the same source still wins.

Re: "does this make it non-breaking?" — verified, partially

Key aliasing removes the common user-facing breaks. Remaining breaks after
this change:

Break Status
YAML coveralls_host/config_file dropped ✅ fixed (deprecated + warn)
Coveralls(config_file=...) TypeError ✅ fixed (deprecated + warn)
CLI --service/--basedir/--srcdir ✅ already aliased + warn
coveralls debug subcommand → --debug flag ⚠️ still breaking
cover.config is now a Config object, not a dict (.config['x'].config.x) ⚠️ still breaking (inherent to the typing goal)
Removed public load_config_from_* methods ⚠️ still breaking (unusual to call directly)

So it is not strictly non-breaking, but the realistically-impactful surfaces
(existing .coveralls.yml files and basic Coveralls() usage) are now
compatible.

Open question: the debug subcommand

Making coveralls debug non-breaking would mean re-adding it as a hidden
deprecated subcommand — which reintroduces the full option duplication this PR
deliberately removed (#4). I left it as a --debug flag. Happy to add a thin
deprecated debug subcommand if we'd rather keep that fully compatible; let me
know your preference.

Bring back 'coveralls debug' as a subcommand (the documented troubleshooting
entry point) instead of a --debug flag, while avoiding the copy-pasted
typer.Option blocks that CODE_REVIEW #4 called out.

Each option spec is defined once as a shared Annotated type alias and
referenced by both the default command and the debug subcommand, which are thin
wrappers over a common _dispatch(). Changing a flag name or help string now
updates both entry points automatically.
@TheKevJames

Copy link
Copy Markdown
Owner Author

Update: coveralls debug restored as a subcommand

Reverted the --debug flag back to the coveralls debug subcommand (the
documented troubleshooting entry point), without re-introducing the
duplicated typer.Option(...) blocks that #4 flagged.

Each option spec is now defined once as a shared Annotated type alias; the
default command and the debug subcommand are thin wrappers over a common
_dispatch(). Adding/renaming a flag or editing its help text updates both
entry points automatically, so they can't drift.

coveralls debug [OPTIONS] accepts the full option set (verified by a new
test). This also removes the debug subcommand from the earlier breaking-change
list.

service_name is now a plain str defaulting to 'coveralls-python' rather than
str | None resolved to that value later, so submit_report()'s
service_name.startswith('github') check can never hit None. The default literal
lives on the Config field (DEFAULT_SERVICE_NAME); _detect_ci() returns None
when no CI is detected and lets that default apply.
The CLI always forwarded rcfile='.coveragerc' as a highest-precedence override,
so an rcfile/config_file set in .coveralls.yml was silently ignored (while its
deprecation warning still fired, implying it took effect).

Default --rcfile to None and let resolve() drop it, matching how the other
string flags already work, so the config-file value survives. coverage.py
treats config_file='.coveragerc' and the auto-discovery default identically, so
the no-config behaviour is unchanged.
@TheKevJames

Copy link
Copy Markdown
Owner Author

Good catch — reproduced and fixed.

Root cause was exactly as described: _build_overrides always forwarded
rcfile='.coveragerc' (a non-None default), which is highest precedence in
resolve() and clobbered any rcfile/config_file from .coveralls.yml while
the deprecation warning still fired.

Fix: --rcfile now defaults to None and resolve() drops it (the same
None-forwarding the other string flags already rely on), so the config-file
value survives.

I went with the "only forward when passed" option rather than just documenting,
after verifying it's behaviour-preserving for the no-config case: coverage.py
treats config_file='.coveragerc' and its auto-discovery default (True)
identically — even a missing .coveragerc falls through to discovering
setup.cfg/tox.ini/pyproject.toml. So dropping the hardcoded .coveragerc
default changes nothing when no config is set.

Regression coverage added:

  • resolve() with a rcfile=None override no longer clobbers a
    config_file:/rcfile: set in the file.
  • CLI tests assert rcfile=None is forwarded when --rcfile is omitted.

_from_file() relies on 'yaml.safe_load(content) or {}' to treat an empty or
comment-only .coveralls.yml (which safe_load()s to None) as no config; the old
code crashed with a TypeError on None. Lock this in for empty, blank-line, and
comment-only files.
@TheKevJames

Copy link
Copy Markdown
Owner Author

Added test_empty_config_file_is_ignored (parametrized over empty, blank-line-only, and comment-only files — all of which yaml.safe_load() returns None for), asserting resolve() falls back to defaults instead of crashing. Locks in the ... or {} behaviour that replaced the old update(None) TypeError.

…field

service_name was special-cased as an explicit __init__ param and re-injected
into overrides, while every other Config field flows through **kwargs. It is
always passed by keyword, so drop the special case and let it flow through
kwargs; resolve()'s None-drop replaces the old truthy guard.

token_required stays explicit: it is the documented positional argument the CLI
passes as Coveralls(token_required, **overrides), so it genuinely cannot
collapse into **kwargs.
@TheKevJames

Copy link
Copy Markdown
Owner Author

Collapsed — service_name now flows through **kwargs like every other field, and resolve()'s None-drop replaces the old if service_name: guard (verified equivalent for the keyword and CLI-None paths). Signature is now Coveralls(token_required=True, **kwargs).

Kept token_required explicit: it's the documented positional arg the CLI passes as Coveralls(token_required, **overrides), so unlike service_name it can't move into **kwargs without breaking positional calls.

Generic CI parsed CI_PULL_REQUEST with NUMBER_REGEX (trailing integer) while
circle/jenkins used a separate split('/')[-1] helper with no numeric
validation -- two semantics for the same value (both read CI_PULL_REQUEST).

Replace _pr_from_path with a single _parse_pr_number using the Coveralls-
documented trailing-integer semantic, shared by the generic, circle and jenkins
loaders. For the real-world URL/int inputs this is identical; it additionally
rejects non-numeric trailing segments rather than forwarding them verbatim.
@TheKevJames

Copy link
Copy Markdown
Owner Author

Unified rather than commented, since consolidating PR-parsing is a stated goal of this PR.

Both callers of the old _pr_from_path (circle, jenkins) read CI_PULL_REQUEST — the same var the generic loader already parses with NUMBER_REGEX — so there was no good reason for two semantics. Replaced it with a single _parse_pr_number using the Coveralls-documented trailing-integer rule, now shared by the generic, circle, and jenkins loaders.

For real-world inputs (bare int / .../pull/42 URL) it's identical; the only difference is it now rejects non-numeric trailing segments instead of forwarding them verbatim. Added a parametrized test for the helper covering int/path/URL/empty/None/trailing-slash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant