Skip to content

fix(config): allow --minimize-reads type-scoped mode with --cleanup #653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
riyazsh merged 1 commit into from
Jul 30, 2026
+195 −5
Merged

fix(config): allow --minimize-reads type-scoped mode with --cleanup #653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions datadog_sync/utils/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,8 +587,10 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration:
raise click.UsageError("--minimize-reads requires --resource-per-file")
if not kwargs.get("resources", None):
raise click.UsageError("--minimize-reads requires --resources")
if kwargs.get("cleanup") and kwargs["cleanup"].lower() in ("true", "force"):
raise click.UsageError("--minimize-reads cannot be combined with --cleanup")
# The --cleanup compatibility check moved after the ID-targeting decision
# block below. Rationale: --minimize-reads has two sub-modes with
# different safety properties under --cleanup — see the sub-mode-aware
# guard after the state-strategy decision.

# Validate --skip-state-load constraints.
# The Click decorator lives on `_import.py` only, so the flag is never
Expand Down Expand Up @@ -649,6 +651,43 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration:
)
_state_resource_types = raw_types

# Sub-mode-aware --cleanup compatibility check.
#
# --minimize-reads has two sub-modes selected above:
# * ID-targeted: state.destination[type] loads only the IDs listed by
# --filter exact-IDs or --id-file. A cleanup diff
# (destination[type] - source[type]) over a scoped-load is unsafe —
# legitimate destination records outside the scope are absent from
# the diff, and an ID that dropped from source between prior dispatch
# and now (e.g. a rename, temporary API 404, race) surfaces as an
# orphan and gets DELETE'd from the destination org.
# * Type-scoped: state.destination[type] loads all IDs of the requested
# types via the storage backend's prefix listing — see
# ``BaseStorage.get(origin, resource_types=[...])``. The diff is
# well-defined per type, functionally identical to full-load-then-
# cleanup for the types in scope.
#
# Sub-mode selection completed above. ID-targeted is signalled by
# ``_state_exact_ids is not None``. Note: ``id_payload`` alone is NOT the
# trigger — an --id-file whose types are disjoint from --resources leaves
# ``_state_exact_ids`` at None (intersection empty at line 636-643), the
# path falls through to type-scoped, and cleanup is safe. Gating on
# ``_state_exact_ids`` alone tracks the actual scoping decision.
if (
minimize_reads
and _state_exact_ids is not None
and kwargs.get("cleanup")
and kwargs["cleanup"].lower() in ("true", "force")
):
raise click.UsageError(
"--minimize-reads with --id-file or exact-id --filter cannot be "
"combined with --cleanup: state.destination is scoped to the "
"listed IDs, so the cleanup diff would delete legitimate "
"destination records outside the scope. Use type-scoped "
"--minimize-reads (--resources with no exact-id --filter, and "
"no --id-file whose types overlap --resources) with --cleanup."
)

# Initialize state. --skip-state-load constructs an ImportState (write-only,
# no source/destination accessors, no boot-time load); otherwise the regular
# State class loads from storage on construction.
Expand Down
157 changes: 154 additions & 3 deletions tests/unit/test_minimize_reads_type_scoped.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,26 @@ def test_minimize_reads_not_available_on_import_command(self, runner):
assert result.exit_code != 0
assert "no such option" in result.output.lower()

def test_minimize_reads_cannot_be_combined_with_cleanup(self, runner):
"""--minimize-reads + --cleanup must be rejected before any I/O."""
def test_minimize_reads_type_scoped_with_cleanup_is_accepted(self, runner, tmp_path):
"""--minimize-reads without --filter/--id-file selects the type-scoped
sub-mode, which loads full state.destination[type] via prefix listing.
The cleanup diff destination[type] - source[type] is well-defined in
this sub-mode, so the guardrail must accept the combination.

Regression guard for the sub-mode-aware refinement: prior blanket
rejection incorrectly blocked this safe case, forcing wrapper-side
strips that reintroduced the O(N)→O(K) state-load regression for
types where --id-file is otherwise the fast path.
"""
# Use tmp local storage so the command actually initializes State
# instead of erroring on missing storage config. We assert exit
# against the guard specifically — the run will fail later on
# missing API creds, that's fine; the guard is validated
# separately below.
src = tmp_path / "src"
dst = tmp_path / "dst"
src.mkdir()
dst.mkdir()
result = runner.invoke(
cli,
[
Expand All @@ -154,11 +172,144 @@ def test_minimize_reads_cannot_be_combined_with_cleanup(self, runner):
"--resources=roles",
"--cleanup=Force",
"--source-api-key=x",
"--source-app-key=x",
"--destination-api-key=y",
"--destination-app-key=y",
"--source-resources-path",
str(src),
"--destination-resources-path",
str(dst),
],
)
# The sub-mode-aware guard must NOT fire on type-scoped + cleanup.
# Assert BOTH the negative (guard message absent) AND the positive
# (invocation progressed past validation into runtime — evidenced by
# a downstream failure surface, not by exit_code == 0 which would
# require live API creds). This dual assertion prevents a future
# guard-message rename from silently satisfying only the negative
# substring check.
combined_output = (result.output + str(result.exception)).lower()
assert "cannot be combined with --cleanup" not in combined_output, (
"sub-mode-aware guard incorrectly rejected type-scoped + cleanup: " + combined_output
)
# Positive marker: some downstream indication that build_config
# returned and the sync command began running. UsageError from
# earlier validation stages surfaces as "usage:" in the click output;
# its absence combined with an alternative failure (network, auth,
# or successful exit) confirms we reached runtime.
assert "usage:" not in combined_output, (
"unexpected earlier UsageError blocked the type-scoped path: " + combined_output
)

def test_minimize_reads_id_file_with_cleanup_is_rejected(self, runner, tmp_path):
"""--minimize-reads + --id-file (or exact-id --filter) selects the
ID-targeted sub-mode, which loads only listed IDs into
state.destination[type]. Cleanup's diff would delete legitimate
destination records outside the ID scope — the guardrail must fire.
"""
id_file = tmp_path / "ids.json"
id_file.write_text('{"monitors": ["12345"]}')
result = runner.invoke(
cli,
[
"sync",
"--minimize-reads",
"--resource-per-file",
"--resources=monitors",
"--id-file",
str(id_file),
"--cleanup=Force",
"--source-api-key=x",
"--source-app-key=x",
"--destination-api-key=y",
"--destination-app-key=y",
],
)
assert result.exit_code != 0
assert "cleanup" in (result.output + str(result.exception)).lower()
combined_output = (result.output + str(result.exception)).lower()
assert "id-file" in combined_output or "exact-id" in combined_output, (
"guard message must name the actual trigger (id-file or exact-id filter): " + combined_output
)
assert "cleanup" in combined_output

def test_minimize_reads_exact_id_filter_with_cleanup_is_rejected(self, runner):
"""Exact-id --filter also selects ID-targeted sub-mode; same guard
must fire with the sub-mode-aware message.
"""
result = runner.invoke(
cli,
[
"sync",
"--minimize-reads",
"--resource-per-file",
"--resources=roles",
"--filter",
"Type=roles;Name=id;Value=role-1;Operator=exactmatch",
"--cleanup=Force",
"--source-api-key=x",
"--source-app-key=x",
"--destination-api-key=y",
"--destination-app-key=y",
],
)
assert result.exit_code != 0
combined_output = (result.output + str(result.exception)).lower()
# Assert on the specific sub-mode-aware guard message, not a generic
# "cleanup or filter" substring — otherwise an unrelated failure that
# happens to mention "filter" would pass this test.
assert "cannot be combined with --cleanup" in combined_output, combined_output
assert "id-file or exact-id" in combined_output, combined_output

def test_minimize_reads_id_file_disjoint_types_with_cleanup_is_accepted(self, runner, tmp_path):
"""Regression guard for the disjoint-types case: --id-file supplies
IDs for types that are NOT in --resources. Sub-mode selection at
configuration.py:636-643 intersects id_payload with --resources; the
empty intersection leaves _state_exact_ids at None and falls through
to type-scoped loading. Guard must NOT fire — the effective sub-mode
is type-scoped, which is safe with --cleanup.

Prior draft of the guard gated on ``bool(id_payload)`` and would
incorrectly fire here.
"""
id_file = tmp_path / "ids.json"
# id-file lists monitors, but --resources requests roles only.
# Intersection is empty; _state_exact_ids stays None; sub-mode is
# type-scoped.
id_file.write_text('{"monitors": ["12345"]}')
src = tmp_path / "src"
dst = tmp_path / "dst"
src.mkdir()
dst.mkdir()
result = runner.invoke(
cli,
[
"sync",
"--minimize-reads",
"--resource-per-file",
"--resources=roles",
"--id-file",
str(id_file),
"--cleanup=Force",
"--source-api-key=x",
"--source-app-key=x",
"--destination-api-key=y",
"--destination-app-key=y",
"--source-resources-path",
str(src),
"--destination-resources-path",
str(dst),
],
)
combined_output = (result.output + str(result.exception)).lower()
assert "cannot be combined with --cleanup" not in combined_output, (
"sub-mode-aware guard incorrectly rejected disjoint-id-file + type-scoped + cleanup: "
+ combined_output
)
# Positive marker: no earlier UsageError, so build_config progressed
# past all validation blocks (see companion test for rationale).
assert "usage:" not in combined_output, (
"unexpected earlier UsageError blocked the disjoint-id-file path: " + combined_output
)


class TestS3TypeScopedGet:
Expand Down
Loading