-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
fix: snapshot comparison on dicts with different levels #109
fix: snapshot comparison on dicts with different levels #109
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
This is a draft work pushed for review. Unit tests will be added afterwards for the optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -20,6 +20,37 @@ def load_snap(fname: str) -> dict: | |||
{"arp_table": {"properties": ["!ttl"], "count_change_threshold": 10}}, | |||
{"nics": {"count_change_threshold": 10}}, | |||
{"license": {"properties": ["!serial"]}}, | |||
|
|||
# {"license": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should get rid of the commented lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, once I add the unit tests corresponding to these I will remove them in this PR.
By default dictionaries are compared by going down all nested levels. It is possible to configure which keys on each | ||
level should be compared (by default we compare all available keys). This is done using the `properties` parameter. | ||
It's a list of keys that can be compared or skipped on each level. For example, when comparing route tables snapshots are | ||
formatted like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's worth adding a comment about the fact that of a key does not exist in a dict it's ignored. And that this is true in both bases:
- we are on a level where none of the keys match
- we are on the correct level, but the dicts are different, just like the case with licenses that led us to this change.
panos_upgrade_assurance/utils.py
Outdated
@@ -168,13 +169,19 @@ def __init__( | |||
* if `requested_config` is `None` we immediately treat it as if `all` was passed implicitly | |||
(see [`dialect`](/panos/docs/panos-upgrade-assurance/dialect)) - it's expanded to `valid_elements` | |||
* `_requested_config_names` is introduced as `requested_config` stripped of any element configurations. Additionally, we | |||
do verification if elements of this variable match `valid_elements`. An exception is thrown if not. | |||
do verification if all elements of this variable match `valid_elements`, if not, an exception is thrown by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do verification if all elements of this variable match `valid_elements`, if not, an exception is thrown by default. | |
do verification if all elements of this variable match `valid_elements`, if they do not, an exception is thrown by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed.
@@ -248,6 +265,18 @@ def _extract_element_name(config: Union[str, dict]) -> str: | |||
else: | |||
raise exceptions.WrongDataTypeException("Config definition is neither string or dict") | |||
|
|||
def _remove_invalid_requested_config(self, invalid_keys: set) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just wondering if it make sense to build a method for 3 lines of code that won't be used anywhere else besides the constructor
Co-authored-by: Łukasz Pawlęga <[email protected]>
This PR is superseded by #128 which has greater functionality. |
Closing this as PR #128 supersedes this one. |
Description
This PR brings a behavioral change in how the requested configurations are processed and fixes below-mentioned issue on comparing dicts with different nested levels.
Motivation and Context
Fixes #108
With the change, it is also possible to process elements on each level of a nested dictionary. Considering the following dictionary, it is possible to skip or select a specific license like
["!Logging Service"]
which will skip comparing the "Logging Service" license or provide elements on multiple levels like["Logging Service", "!custom"]
which will compare only the "Logging Service" license and exclude "custom" element for comparison. Additionally if different sub-dicts have different elements like "custom" element being absent in "PAN-DB URL Filtering", you can specify["!custom"]
to skip custom key on a dictionary while ignoring it for the other if it doesn't exist. This is achieved by providing an optionalignore_invalid_config
parameter toConfigParser
class.How Has This Been Tested?
Tested with example scripts.
Types of changes
Checklist