Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Feature template deserialize #505

Merged
merged 8 commits into from
Mar 6, 2024
Merged

Feature template deserialize #505

merged 8 commits into from
Mar 6, 2024

Conversation

PrzeG
Copy link
Collaborator

@PrzeG PrzeG commented Mar 5, 2024

Pull Request summary:

Adjustments to Feature Template deserialization, adding proper support for nested values and correct field names

Description of changes:

Checklist:

  • Make sure to run pre-commit before committing changes
  • Make sure all checks have passed
  • PR description is clear and comprehensive
  • Mentioned the issue that this PR solves (if applicable)
  • Make sure you test the changes


def find_template_values(
template_definition: dict,
templated_values: dict = {},
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's generally not recommended to instantiate mutable objects like dictionaries or lists as default parameter values in Python function signatures. This is because mutable default values are evaluated only once when the function is defined, and subsequent calls to the function will reuse the same object, potentially leading to unintended behavior.

Suggested change
templated_values: dict = {},
templated_values: Optional[dict] = None,

from catalystwan.api.templates.device_variable import DeviceVariable


def find_template_values(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The find_template_values function is integral to the UX2 transformation process. It's imperative to conduct thorough unit tests to ensure its reliability and accuracy. Testing should encompass both default Cisco models and fully populated models.

for template in self.templates:
definition = template.template_definiton
with self.subTest(template_name=template.name):
parsed_values = find_template_values(definition)
Copy link

Choose a reason for hiding this comment

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

Address the underlying issues causing the test function to fail for our demo vManage.

Copy link

@ghost ghost Mar 5, 2024

Choose a reason for hiding this comment

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

Please consider to write this as unit test to not rely on having fully configured vManage and speed up test execution. This will also be useful since github workflows only run unit tests. (For now)

@PrzeG PrzeG force-pushed the feature_template_deserialize branch from ca08cfa to 062d3d9 Compare March 6, 2024 13:34
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

It's working for Demo vManage

image

@PrzeG PrzeG merged commit bf2ece1 into main Mar 6, 2024
12 checks passed
@PrzeG PrzeG deleted the feature_template_deserialize branch March 6, 2024 14:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants