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

Introduce create_firewall_mapping Function to Fix Firewall Object Mapping #86

Conversation

cdot65
Copy link
Owner

@cdot65 cdot65 commented Feb 14, 2024

Overview

This PR introduces a dedicated function, create_firewall_mapping, designed to accurately map firewall hostnames to their corresponding Firewall objects and details. The new function replaces the previous use of zip in the batch subcommand, which led to issues with firewall objects not being properly associated with their details due to misalignment in the lists.

Problem

The existing implementation used zip(all_firewalls, firewalls_info) to combine lists of Firewall objects and their information. However, this approach assumed that the order and length of both lists would always align, which was not guaranteed, especially after introducing multithreading for fetching firewall information. This led to incorrect mappings, where firewall details were associated with the wrong Firewall objects.

Solution

To address this issue, we implemented the create_firewall_mapping function. This function constructs a dictionary where each key is a firewall hostname, and the value is another dictionary containing the corresponding Firewall object and its detailed information. The mapping is based on the serial number of the firewalls, ensuring that the details are correctly associated even if the order of objects or information changes.

Benefits

  • Accuracy: By mapping based on the serial number, we eliminate the risk of misalignment between the Firewall objects and their details.
  • Reusability: The function can be used in multiple parts of the script, reducing code duplication and enhancing maintainability.
  • Testability: Isolating the mapping logic into its own function allows for more straightforward unit testing, ensuring the reliability of the mapping.

Changes Made:

  • Added the create_firewall_mapping function.
  • Replaced the zip usage in the batch subcommand with a call to create_firewall_mapping.
  • Updated the corresponding parts of the script to utilize the new mapping.

Testing

Added unit tests for the create_firewall_mapping function to cover various scenarios, including mismatched lists and missing items.

Manually tested the batch subcommand to ensure that firewall details are correctly associated with their Firewall objects.

This change resolves the issue of incorrect firewall object mapping and improves the overall robustness and maintainability of the script.

@cdot65 cdot65 added the bug Something isn't working label Feb 14, 2024
@cdot65 cdot65 self-assigned this Feb 14, 2024
@cdot65 cdot65 linked an issue Feb 14, 2024 that may be closed by this pull request
@cdot65
Copy link
Owner Author

cdot65 commented Feb 14, 2024

Whoops, I accidentally pushed my changes into main instead of this branch. Check out commit #1595fbab1a7b60024579bb42d0cb74be15a33f2b

@cdot65 cdot65 merged commit f985d91 into main Feb 14, 2024
1 check passed
@cdot65 cdot65 deleted the 85-discrepancy-in-targeted-firewalls-for-batch-upgrade-in-ha-configuration branch February 15, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discrepancy in Targeted Firewalls for Batch Upgrade in HA Configuration
1 participant