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 to_text for vrf names #772

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wormley
Copy link

@wormley wormley commented Oct 19, 2023

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cisco.nxos.nxos_bgp_global

ADDITIONAL INFORMATION

The cisco.nxos.nxos_bgp_global suffers the same edge case as was fixed in snmp last year. If there are mixed vrf names some having all digits and some having letters the same error occurs in sorted() when it tries to compare int to string.
This is the same code change as was done in snmp_server.

This proposed change may cause some issue for anyone using purely numeric vrf names as "9" will now sort after "10" and the section will be re-ordered.

@wormley wormley temporarily deployed to ack October 19, 2023 18:48 — with GitHub Actions Inactive
@wormley wormley temporarily deployed to ack October 19, 2023 18:51 — with GitHub Actions Inactive
@softwarefactory-project-zuul
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/8aa39b9f057d434293c1cb3268b17cd3

✔️ ansible-galaxy-importer SUCCESS in 5m 12s
✔️ build-ansible-collection SUCCESS in 8m 46s
✔️ ansible-test-network-integration-nxos-cli-python39-scenario01 SUCCESS in 29m 01s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario02 SUCCESS in 28m 04s (non-voting)
ansible-test-network-integration-nxos-cli-python39-scenario03 FAILURE in 36m 17s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario04 SUCCESS in 27m 51s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario01 SUCCESS in 34m 43s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario02 SUCCESS in 34m 42s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario03 SUCCESS in 41m 24s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario04 SUCCESS in 34m 39s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario01 SUCCESS in 35m 55s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario02 SUCCESS in 33m 30s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario03 SUCCESS in 41m 11s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario04 SUCCESS in 32m 08s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario01 SUCCESS in 34m 12s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario02 SUCCESS in 33m 26s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario03 SUCCESS in 40m 16s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario04 SUCCESS in 32m 05s (non-voting)
✔️ ansible-tox-linters SUCCESS in 10m 19s

@NilashishC NilashishC temporarily deployed to ack October 25, 2023 07:20 — with GitHub Actions Inactive
@NilashishC NilashishC self-requested a review October 25, 2023 07:22
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #772 (7676341) into main (d7fcc2d) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #772   +/-   ##
=======================================
  Coverage   85.01%   85.01%           
=======================================
  Files         211      211           
  Lines       18230    18231    +1     
=======================================
+ Hits        15498    15499    +1     
  Misses       2732     2732           

see 1 file with indirect coverage changes

@softwarefactory-project-zuul
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/a0fa436685994789800a66563ad8e998

✔️ ansible-galaxy-importer SUCCESS in 3m 40s
✔️ build-ansible-collection SUCCESS in 8m 37s
✔️ ansible-test-network-integration-nxos-cli-python39-scenario01 SUCCESS in 29m 54s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario02 SUCCESS in 28m 22s (non-voting)
ansible-test-network-integration-nxos-cli-python39-scenario03 FAILURE in 36m 35s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario04 SUCCESS in 27m 49s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario01 SUCCESS in 35m 09s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario02 SUCCESS in 35m 33s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario03 SUCCESS in 41m 13s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario04 SUCCESS in 33m 10s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario01 SUCCESS in 35m 05s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario02 SUCCESS in 34m 20s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario03 SUCCESS in 42m 05s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario04 SUCCESS in 32m 48s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario01 SUCCESS in 33m 50s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario02 SUCCESS in 33m 05s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario03 SUCCESS in 40m 40s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario04 SUCCESS in 31m 39s (non-voting)
✔️ ansible-tox-linters SUCCESS in 10m 19s

@KB-perByte KB-perByte temporarily deployed to ack October 25, 2023 13:04 — with GitHub Actions Inactive
@softwarefactory-project-zuul
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/9b5c3579943c47ae8259c296d1407f6e

✔️ ansible-galaxy-importer SUCCESS in 3m 42s
✔️ build-ansible-collection SUCCESS in 8m 49s
✔️ ansible-test-network-integration-nxos-cli-python39-scenario01 SUCCESS in 29m 40s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario02 SUCCESS in 28m 30s (non-voting)
ansible-test-network-integration-nxos-cli-python39-scenario03 FAILURE in 38m 49s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario04 SUCCESS in 29m 42s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario01 SUCCESS in 36m 50s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario02 SUCCESS in 36m 49s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario03 SUCCESS in 43m 44s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario04 SUCCESS in 34m 05s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario01 SUCCESS in 34m 27s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario02 SUCCESS in 33m 24s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario03 SUCCESS in 41m 06s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario04 SUCCESS in 32m 27s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario01 SUCCESS in 33m 51s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario02 SUCCESS in 32m 55s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario03 SUCCESS in 40m 15s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario04 SUCCESS in 31m 23s (non-voting)
✔️ ansible-tox-linters SUCCESS in 10m 30s

Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/42a72e2058f94a638059694845145fe8

✔️ ansible-galaxy-importer SUCCESS in 4m 42s
✔️ build-ansible-collection SUCCESS in 10m 02s
✔️ ansible-test-network-integration-nxos-cli-python39-scenario01 SUCCESS in 30m 20s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario02 SUCCESS in 28m 59s (non-voting)
ansible-test-network-integration-nxos-cli-python39-scenario03 FAILURE in 37m 03s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario04 SUCCESS in 28m 14s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario01 SUCCESS in 36m 38s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario02 SUCCESS in 34m 48s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario03 SUCCESS in 42m 26s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario04 SUCCESS in 32m 53s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario01 SUCCESS in 35m 47s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario02 SUCCESS in 34m 56s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario03 SUCCESS in 41m 34s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario04 SUCCESS in 32m 58s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario01 SUCCESS in 34m 54s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario02 SUCCESS in 33m 27s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario03 SUCCESS in 41m 18s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario04 SUCCESS in 32m 24s (non-voting)
✔️ ansible-tox-linters SUCCESS in 17m 58s

@KB-perByte
Copy link
Collaborator

Hey @wormley as you correctly pointed out "This proposed change may cause some issue for anyone using purely numeric vrf names as "9" will now sort after "10" and the section will be re-ordered."

I would recommend if we can have a fresh implementation of a sorting algorithm that can handle both the pattern of vrf names and place it in a generic place like netcommon, it would have been the right approach to this issue.

Let us know if you need any information on this.
Regards.

Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/fb6482b838df4ef59ef39ff13b94b678

✔️ ansible-galaxy-importer SUCCESS in 4m 44s
✔️ build-ansible-collection SUCCESS in 10m 11s
✔️ ansible-test-network-integration-nxos-cli-python39-scenario01 SUCCESS in 29m 26s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario02 SUCCESS in 28m 48s (non-voting)
ansible-test-network-integration-nxos-cli-python39-scenario03 FAILURE in 38m 56s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-scenario04 SUCCESS in 28m 08s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario01 SUCCESS in 35m 55s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario02 SUCCESS in 33m 45s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario03 SUCCESS in 40m 43s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable215-scenario04 SUCCESS in 32m 08s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario01 SUCCESS in 35m 01s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario02 SUCCESS in 34m 02s (non-voting)
ansible-test-network-integration-nxos-cli-python39-stable214-scenario03 RETRY_LIMIT in 9m 41s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable214-scenario04 SUCCESS in 31m 54s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario01 SUCCESS in 34m 38s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario02 SUCCESS in 33m 59s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario03 SUCCESS in 41m 08s (non-voting)
✔️ ansible-test-network-integration-nxos-cli-python39-stable213-scenario04 SUCCESS in 31m 16s (non-voting)
✔️ ansible-tox-linters SUCCESS in 11m 50s

Copy link
Collaborator

@NilashishC NilashishC left a comment

Choose a reason for hiding this comment

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

This would require implementing the changes suggested by @KB-perByte, before we can take it forward. Please let us know if you need any help on this.

Thank you for your contribution! We really appreciate it.

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.

5 participants