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

add support for "cml generate nso" for cat8000v or cat9000v #140

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

sgherdao
Copy link
Contributor

simple PR to fix #139

❯ grep name test-nso*
test-nso:      <name>virl</name>
test-nso:        <remote-name>cisco</remote-name>
test-nso:        <remote-name>cisco</remote-name>
test-nso:    <name>csr1000v-0</name>
test-nso:    <name>iosv-0</name>
test-nso:    <name>xr9kv-0</name>
test-nso:    <name>asav-0</name>
test-nso:    <name>iosvl2-0</name>
test-nso:    <name>nxos9000-0</name>


test-nso-fix:      <name>virl</name>
test-nso-fix:        <remote-name>cisco</remote-name>
test-nso-fix:        <remote-name>cisco</remote-name>
test-nso-fix:    <name>csr1000v-0</name>
test-nso-fix:    <name>iosv-0</name>
test-nso-fix:    <name>xr9kv-0</name>
test-nso-fix:    <name>cat8000v-0</name>  <<
test-nso-fix:    <name>asav-0</name>
test-nso-fix:    <name>iosvl2-0</name>
test-nso-fix:    <name>nxos9000-0</name>
test-nso-fix:    <name>cat9000v-dd-0</name>  <<

@xorrkaz
Copy link
Collaborator

xorrkaz commented Apr 25, 2023

I'm generally okay with this, but I do think some node defs do make use of these types somewhat liberally. This might end up generating NSO files with more than Cisco devices, but I'm cool with that.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4759245945

  • 5 of 10 (50.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 72.317%

Changes Missing Coverage Covered Lines Changed/Added Lines %
virl/generators/nso_payload.py 5 10 50.0%
Files with Coverage Reduction New Missed Lines %
virl/cli/save/commands.py 1 88.57%
Totals Coverage Status
Change from base Build 4344677404: -0.03%
Covered Lines: 2338
Relevant Lines: 3233

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4759245945

  • 5 of 10 (50.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 72.317%

Changes Missing Coverage Covered Lines Changed/Added Lines %
virl/generators/nso_payload.py 5 10 50.0%
Files with Coverage Reduction New Missed Lines %
virl/cli/save/commands.py 1 88.57%
Totals Coverage Status
Change from base Build 4344677404: -0.03%
Covered Lines: 2338
Relevant Lines: 3233

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4759245945

  • 5 of 10 (50.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.7%) to 73.087%

Changes Missing Coverage Covered Lines Changed/Added Lines %
virl/generators/nso_payload.py 5 10 50.0%
Files with Coverage Reduction New Missed Lines %
virl/cli/save/commands.py 1 94.29%
virl/generators/ansible_inventory.py 1 55.8%
virl/generators/nso_payload.py 1 47.31%
Totals Coverage Status
Change from base Build 4344677404: 0.7%
Covered Lines: 2436
Relevant Lines: 3333

💛 - Coveralls

@coveralls
Copy link

coveralls commented Apr 25, 2023

Pull Request Test Coverage Report for Build 4759245945

  • 5 of 10 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 73.117%

Changes Missing Coverage Covered Lines Changed/Added Lines %
virl/generators/nso_payload.py 5 10 50.0%
Totals Coverage Status
Change from base Build 4344677404: 0.8%
Covered Lines: 2437
Relevant Lines: 3333

💛 - Coveralls

@xorrkaz xorrkaz changed the base branch from master to joe-dev April 25, 2023 18:32
@xorrkaz xorrkaz merged commit 05f0380 into CiscoDevNet:joe-dev Apr 25, 2023
@sgherdao
Copy link
Contributor Author

Hey @xorrkaz,

I'm generally okay with this, but I do think some node defs do make use of these types somewhat liberally. This might end up generating NSO files with more than Cisco devices, but I'm cool with that.

Agreed, the logic can be improved, for example knowing that we have iosxrv9000 and iosv, only the order of elif makes the code correct:

try:
type = node.node_definition.lower()
if "nx" in type:
entry["prefix"] = "{{ NX_PREFIX }}"
entry["ned"] = "{{ NX_NED_ID }}"
entry["ns"] = "{{ NX_NAMESPACE }}"
elif "xr" in type:
entry["prefix"] = "{{ XR_PREFIX }}"
entry["ned"] = "{{ XR_NED_ID }}"
entry["ns"] = "{{ XR_NAMESPACE }}"
elif "csr" in type or "ios" in type:
entry["prefix"] = "{{ IOS_PREFIX }}"
entry["ned"] = "{{ IOS_NED_ID }}"
entry["ns"] = "{{ IOS_NAMESPACE }}"
elif "asa" in type:
entry["prefix"] = "{{ ASA_PREFIX }}"
entry["ned"] = "{{ ASA_NED_ID }}"
entry["ns"] = "{{ ASA_NAMESPACE }}"
except KeyError:
pass

I could work something out :)

I see a bigger problem, cml generate nso will only render payload data for the nodes for which the management interface is found which currently relies on CML snooper which is not really 100% reliable, it may be desirable for some to still have the data and manually add the missing addresses.
And maybe separate the logic of generating the payload and pushing to NSO.

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.

cml generate nso does not support cat8000v or cat9000v
4 participants