monitor_dbus_signals: check variant_level equality#328
Conversation
WalkthroughAdds a new Diff subtype Changes
Sequence Diagram(s)sequenceDiagram
participant Monitor as monitor_dbus_signals.py
participant Checker as _check_props
participant DiffList as Diffs
Monitor->>Checker: Provide old and new property maps
Checker->>Checker: Compare per-property values and states
alt Property value changed (emits TRUE)
Checker->>DiffList: Append DifferentProperty / ChangedProperty
else Emits CONST
Checker->>DiffList: Append ChangedProperty
else Variant_level differs (old valid)
Checker->>DiffList: Append DifferentVariantLevel
end
Checker-->>Monitor: Return diffs
Monitor->>Monitor: Report diffs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
mulkieran
force-pushed
the
issue_stratisd_3895
branch
from
a501cfb to
c414e93
Compare
mulkieran
changed the title
mulkieran
force-pushed
the
issue_stratisd_3895
branch
2 times, most recently
from
e718938 to
c9a24ee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/monitor_dbus_signals.py (2)
113-138: LGTM on new Diff type; minor docstring terminology fixThe addition of DifferentVariantLevel is clear and consistent with the existing Diff types. Minor nit: the D-Bus signal is named "PropertiesChanged", not "PropertiesChangedSignal". Consider correcting the terminology in the docstring for accuracy.
Apply this minimal docstring fix:
- level does not match. The variant levels among the GetManagedObjects - result, the Properties.GetAll result and the PropertiesChangedSignal + level does not match. The variant levels among the GetManagedObjects + result, the Properties.GetAll result and the PropertiesChanged result should always be the same, because both are encoded using dicts,
722-729: Guard variant_level access and clarify intent w.r.t. value inequalityDirectly accessing .variant_level may raise AttributeError if either side is a plain Python type (e.g., unit tests or alternate proxy layers). Also, the class docstring says “value is correct but the variant level does not match,” yet this check runs regardless of equality. If that’s intentional (to report both a DifferentProperty and a DifferentVariantLevel), fine—otherwise add the equality guard.
Safer attribute access (treat missing variant_level as 0):
- if ( - old_value is not INVALIDATED - and new_value.variant_level != old_value.variant_level - ): - diffs.append( - DifferentVariantLevel(object_path, ifn, key, old_value, new_value) - ) + if old_value is not INVALIDATED: + old_vl = getattr(old_value, "variant_level", 0) + new_vl = getattr(new_value, "variant_level", 0) + if new_vl != old_vl: + diffs.append( + DifferentVariantLevel(object_path, ifn, key, old_value, new_value) + )If the intent is to only flag variant-level mismatches when the value itself matches, add the equality guard too:
- if new_vl != old_vl: + if new_value == old_value and new_vl != old_vl: diffs.append( DifferentVariantLevel(object_path, ifn, key, old_value, new_value) )Please confirm which behavior you want (always report variant-level mismatches, or only when values otherwise match). If helpful, I can follow up with a small test harness to exercise both paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/monitor_dbus_signals.py(2 hunks)
mulkieran
force-pushed
the
issue_stratisd_3895
branch
from
c9a24ee to
ae41a85
Compare
|
I fixed a comment...but rejected the other suggestion. Actually, I decided it had a point and simplified things a bit. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/monitor_dbus_signals.py (2)
113-139: New diff type looks good; consider exposing variant levels in repr and aligning docstring with behaviorThe class is consistent with existing Diff types. Two small improvements:
- Make it easier to see the actual variant-level mismatch in logs by including the levels in repr (using getattr to stay robust).
- Docstring says “value is correct but … variant level does not match,” while the check (Line 722 onward) runs regardless of value equality. Either clarify the docstring or gate the check to only when values are equal.
Apply this diff to enhance the repr:
@@ def __repr__(self): - return ( - f"DifferentVariantLevel({self.object_path!r}, " - f"{self.interface_name!r}, {self.key!r}, {self.old_value!r}, " - f"{self.new_value!r})" - ) + old_vl = getattr(self.old_value, "variant_level", None) + new_vl = getattr(self.new_value, "variant_level", None) + return ( + f"DifferentVariantLevel({self.object_path!r}, " + f"{self.interface_name!r}, {self.key!r}, {self.old_value!r}, " + f"{self.new_value!r}, old_vl={old_vl!r}, new_vl={new_vl!r})" + )Optionally tweak the opening sentence of the docstring to avoid implying value equality if that’s not required by design.
722-729: Guard variant_level attribute access to avoid AttributeError; optionally restrict to equal valuesnew_value/old_value are often dbus.* instances with variant_level, but defensive guards will prevent crashes if plain Python values slip through. Also, if the intent is “value is correct but levels differ,” you may want to run this only when new_value == old_value.
Apply this diff to harden the check:
- if ( - old_value is not INVALIDATED - and new_value.variant_level != old_value.variant_level - ): + if ( + old_value is not INVALIDATED + and hasattr(new_value, "variant_level") + and hasattr(old_value, "variant_level") + and new_value.variant_level != old_value.variant_level + ): diffs.append( DifferentVariantLevel(object_path, ifn, key, old_value, new_value) )If you do want to enforce that only “value-equal but variant-level-different” cases are reported, consider making this block an
elifto the precedingif new_value != old_value:or addand new_value == old_valueto the condition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/monitor_dbus_signals.py(2 hunks)
mulkieran
force-pushed
the
issue_stratisd_3895
branch
from
ae41a85 to
0deba5b
Compare
Signed-off-by: mulhern <amulhern@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/monitor_dbus_signals.py (1)
708-724: Guard against missingvariant_leveland tighten mutually-exclusive branches.Relying on
assert hasattr(old_value, "variant_level")can still raise at runtime (asserts can be stripped with -O, andnew_value.variant_levelis accessed later without any guard). Usinggetattrdefensively prevents crashes if a plain Python type slips through. Also, switch the secondiftoelifto reflect the exclusivity of enum cases.Apply this diff:
@@ - else: - assert hasattr(old_value, "variant_level") - - if new_value != old_value: - if emits_signal is EmitsChangedSignal.TRUE: + else: + # Be defensive: dbus-python values should carry `variant_level`, + # but guard in case a plain Python type slips through. + old_vl = getattr(old_value, "variant_level", None) + new_vl = getattr(new_value, "variant_level", None) + + if new_value != old_value: + if emits_signal is EmitsChangedSignal.TRUE: diffs.append( DifferentProperty( object_path, ifn, key, old_value, new_value ) ) - if emits_signal is EmitsChangedSignal.CONST: + elif emits_signal is EmitsChangedSignal.CONST: diffs.append( ChangedProperty(object_path, ifn, key, old_value, new_value) ) - elif new_value.variant_level != old_value.variant_level: + elif old_vl is not None and new_vl is not None and new_vl != old_vl: diffs.append( DifferentVariantLevel( object_path, ifn, key, old_value, new_value ) )Would you confirm that in your environments all property values indeed carry
variant_levelvia dbus-python? If not guaranteed, the above guard will avoid spurious AttributeErrors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/monitor_dbus_signals.py(2 hunks)
🔇 Additional comments (3)
scripts/monitor_dbus_signals.py (3)
113-138: Nice addition: explicit diff for variant-level mismatches.The
DifferentVariantLevelDiff type is clear, mirrors the existing Diff classes, and the docstring crisply explains why this matters for a{sv} values across GetManagedObjects/GetAll/PropertiesChanged. The__repr__format also aligns with the rest.
701-708: INVALIDATED handling remains correct and readable.Keeping the NotInvalidatedProperty check gated on
old_value is INVALIDATEDandemits_signal != INVALIDATESmaintains the original behavior and makes the invariant explicit.
724-728: Right placement for variant-level comparison.Checking
variant_levelonly when values are equal is the correct semantics to isolate encoding mismatches without masking actual value changes.
|
Now definitely ignoring CodeRabbit's advice... |
1 participant
Related stratis-storage/stratisd#3895
Summary by CodeRabbit