Skip to content

Fix NIST platform builder for unparseable versions - #890

Open
NataliaPerez08 wants to merge 1 commit into
networktocode:developfrom
NataliaPerez08:fix/861-nist-dataclass-error
Open

Fix NIST platform builder for unparseable versions#890
NataliaPerez08 wants to merge 1 commit into
networktocode:developfrom
NataliaPerez08:fix/861-nist-dataclass-error

Conversation

@NataliaPerez08

Copy link
Copy Markdown

Fixes #861

version_metadata() may return a dictionary containing keys that are not
defined on the dynamically generated platform dataclass when a version cannot
be parsed.

This change filters the returned metadata to fields defined by the platform
dataclass before constructing the object.

A regression test was added for the Cisco IOS version 03.03.05SE.

Tests:

  • 809 passed
  • 10 skipped

@jeffkala

Copy link
Copy Markdown
Collaborator

@NataliaPerez08 thanks for the PR! Quick question. Is the version in the regression test actually coming out of show version or similar from an actual legacy device?

Just curious as the leading zero in the major version field is actually not following semver: https://semver.org/spec/v2.0.0.html#spec-item-2

Trying to ensure that this isn't a "made up failure" vs an actual bug.

@NataliaPerez08

Copy link
Copy Markdown
Author

Thanks for raising that. I checked the provenance of the test value.

03.03.05SE is a real Cisco IOS XE release, not a synthetic SemVer value. Cisco documents it for Catalyst 3650/3850 and maps it to a separate IOSd version.

That said, you are right that the test currently passes it with platform="ios", while netutils distinguishes IOS XE in some of its internal library mappings (cisco_xe vs cisco_ios).

So the version itself is valid, but I want to make sure the platform value in this regression test reflects the intended NIST API semantics. I can update the test to use the appropriate IOS XE platform identifier if that is preferred.

@jeffkala

Copy link
Copy Markdown
Collaborator

guess my point here is if cisco see 03.03.05SE as valid. Netutils should be able to parse it successfully.

Meaning update the basic_regex to be able to find leading zeros and then strip them. Like this

    # If version is not SemVer 2.0.0, attempt to find major/minor only.
    basic_regex: re.Pattern[str] = re.compile(
        r"""
        ^
        0*(?P<major>\d+)
        \.
        (?:0*(?P<minor>\d+))?
        .*$
        """,
        re.VERBOSE,
    )
    ```
    
    Which new regex fixes it to
    ```
>>> from netutils.nist import version_metadata
>>> version_metadata("cisco", "ios", "03.03.05SE")
{'major': '3', 'minor': '3', 'vendor_metadata': False}
>>> 
>>> version_metadata("cisco", "ios", "3.03.05SE")
{'major': '3', 'minor': '3', 'vendor_metadata': False}
>>> version_metadata("cisco", "ios", "3.3.05SE")
{'major': '3', 'minor': '3', 'vendor_metadata': False}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error key from returned dict incorrectly passed as argument to dynamically created dataclass

2 participants