From ef1f2305abb26e90f91e51269ac954194ec2e720 Mon Sep 17 00:00:00 2001 From: Trevor Basinger Date: Tue, 21 Apr 2026 15:01:59 +0000 Subject: [PATCH] fix(labels): show only changed label-set output --- roar/application/query/label.py | 44 ++++++++++++++++++++++-- roar/application/query/results.py | 6 +++- tests/application/query/test_label.py | 43 +++++++++++++++++++++--- tests/happy_path/test_label_command.py | 46 ++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 7 deletions(-) diff --git a/roar/application/query/label.py b/roar/application/query/label.py index 533a51d..8352cc4 100644 --- a/roar/application/query/label.py +++ b/roar/application/query/label.py @@ -2,6 +2,9 @@ from __future__ import annotations +from copy import deepcopy +from typing import Any + from ...db.context import create_database_context from ...integrations.glaas import GlaasClient from ..label_rendering import flatten_label_metadata @@ -33,6 +36,7 @@ def build_set_labels_summary(request: LabelSetRequest) -> LabelCurrentSummary: service = LabelService(db_ctx, request.cwd) resolved = service.resolve_target(request.entity_type, request.target) patch = parse_label_pairs(request.pairs) + current_metadata = service.current_metadata(resolved) result = service.set_metadata(resolved, patch) heading = ( @@ -40,7 +44,12 @@ def build_set_labels_summary(request: LabelSetRequest) -> LabelCurrentSummary: if result.changed else f"Labels unchanged (version {result.version}):" ) - return _build_current_summary(result.metadata, heading=heading) + changed_metadata = _extract_changed_metadata(current_metadata, result.metadata) + return _build_current_summary( + changed_metadata, + heading=heading, + empty_message="No label changes.", + ) def copy_labels(request: LabelCopyRequest) -> str: @@ -150,13 +159,44 @@ def build_label_history_summary(request: LabelHistoryRequest) -> LabelHistorySum ) -def _build_current_summary(metadata: dict, *, heading: str | None = None) -> LabelCurrentSummary: +def _build_current_summary( + metadata: dict, + *, + heading: str | None = None, + empty_message: str = "No labels.", +) -> LabelCurrentSummary: return LabelCurrentSummary( heading=heading, entries=_build_label_entries(metadata), + empty_message=empty_message, ) +_UNCHANGED = object() + + +def _extract_changed_metadata(before: dict[str, Any], after: dict[str, Any]) -> dict[str, Any]: + changed = _diff_metadata(before, after) + return changed if isinstance(changed, dict) else {} + + +def _diff_metadata(before: Any, after: Any) -> Any: + if isinstance(before, dict) and isinstance(after, dict): + changed: dict[str, Any] = {} + for key, after_value in after.items(): + if key not in before: + changed[key] = deepcopy(after_value) + continue + diff = _diff_metadata(before[key], after_value) + if diff is not _UNCHANGED: + changed[key] = diff + return changed if changed else _UNCHANGED + + if before == after: + return _UNCHANGED + return deepcopy(after) + + def _build_label_entries(metadata: dict) -> list[LabelEntrySummary]: return [ LabelEntrySummary(key=key, display_value=value) diff --git a/roar/application/query/results.py b/roar/application/query/results.py index 7d8a472..11c761c 100644 --- a/roar/application/query/results.py +++ b/roar/application/query/results.py @@ -89,13 +89,17 @@ def render(self, *, indent: str = "") -> str: class LabelCurrentSummary: heading: str | None = None entries: list[LabelEntrySummary] = field(default_factory=list) + empty_message: str = "No labels." def render(self) -> str: lines: list[str] = [] if self.heading: lines.append(self.heading) if not self.entries: - lines.append("No labels.") + message = self.empty_message + if self.heading: + message = f" {message}" + lines.append(message) return "\n".join(lines) indent = " " if self.heading else "" lines.extend(entry.render(indent=indent) for entry in self.entries) diff --git a/tests/application/query/test_label.py b/tests/application/query/test_label.py index 411c9d1..07fa77f 100644 --- a/tests/application/query/test_label.py +++ b/tests/application/query/test_label.py @@ -72,16 +72,26 @@ def _push_request(tmp_path: Path, **overrides) -> LabelPushRequest: ) -def test_build_set_labels_summary_returns_typed_summary(tmp_path: Path) -> None: +def test_build_set_labels_summary_returns_only_labels_changed_by_the_patch( + tmp_path: Path, +) -> None: db_ctx = MagicMock() db_ctx.__enter__.return_value = db_ctx db_ctx.__exit__.return_value = None service = MagicMock() service.resolve_target.return_value = object() + service.current_metadata.return_value = { + "owner": "ml", + "roar": {"operation": {"kind": "run"}}, + } service.set_metadata.return_value = MagicMock( changed=True, version=2, - metadata={"owner": "ml", "stage": "gold"}, + metadata={ + "owner": "ml", + "stage": "gold", + "roar": {"operation": {"kind": "run"}}, + }, ) with ( @@ -92,10 +102,35 @@ def test_build_set_labels_summary_returns_typed_summary(tmp_path: Path) -> None: assert summary.heading == "Updated labels (version 2):" assert [(entry.key, entry.display_value) for entry in summary.entries] == [ - ("owner", "ml"), ("stage", "gold"), ] - assert summary.render() == "Updated labels (version 2):\n owner=ml\n stage=gold" + assert summary.render() == "Updated labels (version 2):\n stage=gold" + + +def test_build_set_labels_summary_reports_no_label_changes_for_noop_updates( + tmp_path: Path, +) -> None: + db_ctx = MagicMock() + db_ctx.__enter__.return_value = db_ctx + db_ctx.__exit__.return_value = None + service = MagicMock() + service.resolve_target.return_value = object() + service.current_metadata.return_value = {"owner": "ml", "stage": "gold"} + service.set_metadata.return_value = MagicMock( + changed=False, + version=2, + metadata={"owner": "ml", "stage": "gold"}, + ) + + with ( + patch("roar.application.query.label.create_database_context", return_value=db_ctx), + patch("roar.application.query.label.LabelService", return_value=service), + ): + summary = build_set_labels_summary(_set_request(tmp_path)) + + assert summary.heading == "Labels unchanged (version 2):" + assert summary.entries == [] + assert summary.render() == "Labels unchanged (version 2):\n No label changes." def test_build_copy_labels_summary_preserves_no_change_heading(tmp_path: Path) -> None: diff --git a/tests/happy_path/test_label_command.py b/tests/happy_path/test_label_command.py index 08e51f1..8e89dfe 100644 --- a/tests/happy_path/test_label_command.py +++ b/tests/happy_path/test_label_command.py @@ -259,6 +259,52 @@ def test_artifact_label_set_noop_does_not_create_new_version( rows = _artifact_label_rows(temp_git_repo, temp_git_repo / "processed.csv") assert rows == [(1, {"owner": "ml", "stage": "raw"})] + def test_job_label_set_output_shows_only_changed_labels_not_system_labels( + self, + temp_git_repo, + roar_cli, + git_commit, + sample_scripts, + sample_data, + python_exe, + ): + result = roar_cli("run", python_exe, "preprocess.py", "input.csv", "processed.csv") + assert result.returncode == 0 + git_commit("After preprocess") + + set_output = _assert_ok( + roar_cli( + "label", + "set", + "job", + "@1", + "phase=preprocess", + check=False, + ) + ) + assert "Updated labels (version 2):" in set_output + assert "phase=preprocess" in set_output + assert "roar.operation.kind=run" not in set_output + assert "roar.git.commit=" not in set_output + + noop_output = _assert_ok( + roar_cli( + "label", + "set", + "job", + "@1", + "phase=preprocess", + check=False, + ) + ) + assert "Labels unchanged (version 2):" in noop_output + assert "No label changes." in noop_output + assert "phase=preprocess" not in noop_output + + label_show = _assert_ok(roar_cli("label", "show", "job", "@1", check=False)) + assert "phase=preprocess" in label_show + assert "roar.operation.kind=run" in label_show + def test_dag_job_and_artifact_labels_are_visible_in_show_and_dag_json( self, temp_git_repo,