Skip to content
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: calculate_diff_on_dicts to Support Integer Values for ARP Table ttl #153

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

cdot65
Copy link
Contributor

@cdot65 cdot65 commented Feb 4, 2024

Description

This PR addresses an issue encountered when comparing ARP tables using the calculate_diff_on_dicts function, specifically when ttl values are present as integers. Previously, the function would raise a WrongDataTypeException due to its inability to handle integer values, causing failures in scripts that rely on comparing ARP table entries.

Addresses #152

Motivation and Context

The calculate_diff_on_dicts function was limited to handling strings and dictionaries, overlooking the possibility of integer values such as those found in ttl fields of ARP tables. This limitation led to a WrongDataTypeException when the function encountered an integer ttl value, particularly evident when comparing ARP tables with single entries.

To resolve this issue, the comparison logic within calculate_diff_on_dicts has been extended to include integer values. The updated logic now checks for both strings and integers, allowing for a successful comparison of ttl values without raising exceptions. The specific change involved modifying the conditional check to:

How Has This Been Tested?

I tested the changes extensively in a controlled environment to ensure that the updated calculate_diff_on_dicts function now correctly handles ARP tables with integer ttl values. Specifically, I performed the following tests:

  • Captured ARP table entries from a Palo Alto Networks device, ensuring the presence of integer ttl values.
  • Ran the modified calculate_diff_on_dicts function to compare two ARP tables: one before the change and one with the updated ttl handling.
  • Verified that the function no longer throws WrongDataTypeException and accurately reflects the differences in ARP entries.

Testing Environment:

  • Python version: 3.11.2
  • Operating System: macOS
  • Device: Palo Alto Networks firewall (for capturing ARP table entries)

These tests confirmed that the function now performs as expected, enabling accurate comparison of ARP tables, including those with integer ttl values, without any errors.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have updated the documentation accordingly. (If you have updated the function's docstring or any related documentation)
  • I have read the CONTRIBUTING document. (Assuming you have, which is important before making a PR)
  • I have added tests to cover my changes if appropriate. (Mark this if you have added unit tests for your changes)
  • All new and existing tests passed. (Assuming you have run the project's existing tests to ensure they still pass with your changes)

@alperenkose alperenkose changed the title Extend calculate_diff_on_dicts to Support Integer Values for ARP Table ttl fix: calculate_diff_on_dicts to Support Integer Values for ARP Table ttl Feb 8, 2024
Copy link
Collaborator

@alperenkose alperenkose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cdot65 one of the unittest is failing where we were testing for the WrongDataTypeException is being raised because "int" is a valid value now.

I would do that but since it's from a fork branch, could you please update the following unittest in test_snapshot_compare.py with a non-valid input like below with a float perhaps (12.3 instead of 123)?

    def test_calculate_diff_on_dicts_invalid_input(self):
        left_snapshot = {"key1": 12.3, "key2": "value2"}
        right_snapshot = {"key1": {"nested_key1": "value1"}, "key2": "value2"}

        with pytest.raises(WrongDataTypeException) as exception_msg:
            SnapshotCompare.calculate_diff_on_dicts(left_snapshot, right_snapshot)

        assert str(exception_msg.value) == "Unknown value format for key key1."

@adambaumeister
Copy link
Collaborator

@alperenkose whats up with the unit tests job in the pipeline? Something weird:

HttpError: Resource not accessible by integration
    at /home/runner/work/_actions/orgoro/coverage/v3.1/webpack:/typescript-action/node_modules/@octokit/request/dist-node/index.js:86:1
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

Copy link
Collaborator

@adambaumeister adambaumeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alperenkose
Copy link
Collaborator

@alperenkose whats up with the unit tests job in the pipeline? Something weird:

@adambaumeister I'm working on fixing this. Actually the root cause is that "write" access in github actions is not allowed from fork repo PRs. I was investigating it on how to securely allow commenting unittests coverage results on fork repo PRs and found a solution. I will open a separate PR for that.

@alperenkose alperenkose merged commit 47ebea5 into PaloAltoNetworks:main Feb 16, 2024
6 of 7 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 7, 2024
## [0.3.4](v0.3.3...v0.3.4) (2024-03-07)

### Features

* bgp peers snapshot comparison ([#154](#154)) ([4fec622](4fec622))

### Bug Fixes

* calculate_diff_on_dicts to Support Integer Values for ARP Table ttl ([#153](#153)) ([47ebea5](47ebea5))
* **firewall_proxy/is_panorama_connected:** Added fix for panorama check for FWs in version PAN-OS 11 or later ([#159](#159)) ([e617dbc](e617dbc))
* route snapshot missing entries fix ([#146](#146)) ([d946462](d946462))
@horiagunica
Copy link
Collaborator

🎉 This PR is included in version 0.3.4 🎉

The release is available on PyPI and GitHub release

Posted by semantic-release bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants