Conversation
✅ Deploy Preview for viseron canceled.
|
There was a problem hiding this comment.
Pull request overview
Adds a config diffing API intended to detect configuration changes at component/domain/identifier granularity to support targeted reloads.
Changes:
- Introduces dataclasses (
ConfigDiff,ComponentChange,DomainChange,IdentifierChange) to represent config diffs. - Adds diff helpers (
diff_config,diff_domain_config,diff_identifier_config) and supporting extraction methods. - Adds extensive unit tests covering the new diffing primitives and helpers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| viseron/config.py | Adds the diffing dataclasses and diff functions for component/domain/identifier config changes. |
| tests/test_config.py | Adds test coverage for the new diffing dataclasses and functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if self.new_config is not None: | ||
| for domain in Domain: | ||
| domain_value = domain.value | ||
| if domain_value in self.new_config: | ||
| new_domains[domain_value] = self.new_config[domain_value] | ||
|
|
There was a problem hiding this comment.
ComponentChange.get_domain_configs only extracts keys that match the Domain enum. Components like nvr have per-camera identifiers directly under the component key (no nested nvr: domain key), so this will return empty dicts and you can't diff identifier-level changes for that component. Consider special-casing components with top-level identifiers (e.g., treat nvr as a domain whose identifier configs are the component dict) or adding a separate path to extract identifier configs for such components, so reload logic can act on per-identifier changes instead of only whole-component changes.
| if self.new_config is not None: | |
| for domain in Domain: | |
| domain_value = domain.value | |
| if domain_value in self.new_config: | |
| new_domains[domain_value] = self.new_config[domain_value] | |
| # Some components (e.g. "nvr") use top-level identifiers directly under | |
| # the component key instead of nesting them under a Domain entry. | |
| # In that case, no Domain keys will be present and old_domains will be | |
| # empty here. To allow identifier-level diffing for such components, | |
| # treat the entire component config as a single domain keyed by the | |
| # component name. | |
| if not old_domains and self.component_name == "nvr": | |
| old_domains[self.component_name] = self.old_config | |
| if self.new_config is not None: | |
| for domain in Domain: | |
| domain_value = domain.value | |
| if domain_value in self.new_config: | |
| new_domains[domain_value] = self.new_config[domain_value] | |
| # Apply the same special-casing for components with top-level | |
| # identifiers in the new configuration. | |
| if not new_domains and self.component_name == "nvr": | |
| new_domains[self.component_name] = self.new_config |
| # Get all component names from both configs | ||
| old_components = set(old_config.keys()) | ||
| new_components = set(new_config.keys()) | ||
|
|
||
| # Added components | ||
| for component_name in new_components - old_components: | ||
| result.component_changes[component_name] = ComponentChange( | ||
| component_name=component_name, | ||
| old_config=None, | ||
| new_config=copy.deepcopy(new_config[component_name]), | ||
| ) | ||
|
|
There was a problem hiding this comment.
diff_config builds component_changes by iterating over set differences/intersections, which makes insertion order nondeterministic; get_added_components/get_removed_components/get_modified_components will therefore return unstable ordering across runs. If callers/logging rely on deterministic behavior, iterate over sorted component names (and likewise sort domains/identifiers in diff_domain_config/diff_identifier_config).
| old_config: The previous component configuration dictionary | ||
| new_config: The new component configuration dictionary | ||
| component_change: The ComponentChange object these domains belong to |
There was a problem hiding this comment.
The diff_domain_config docstring lists a component_change argument, but the function signature doesn't accept it. Please update the docstring so it matches the actual parameters (or add the missing argument if it was intended).
| old_config: The previous component configuration dictionary | |
| new_config: The new component configuration dictionary | |
| component_change: The ComponentChange object these domains belong to | |
| component_name: The name of the component these domain configurations belong to | |
| old_config: The previous domain configuration dictionary for the component | |
| new_config: The new domain configuration dictionary for the component |
| if self.new_config is not None: | ||
| cameras = self.new_config.get("cameras", {}) | ||
| if self.domain == "camera": | ||
| cameras = self.new_config | ||
| else: | ||
| cameras = self.new_config.get("cameras", {}) |
There was a problem hiding this comment.
In DomainChange.get_identifier_configs, cameras = self.new_config.get("cameras", {}) is immediately overwritten in both the camera and non-camera branches, so the first assignment is dead code. Removing it will simplify the control flow.
1 participant
Adds support for diffing two configs and finding component, domain and identifier level changes in order to know what to reload