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

Fixes #695 - inconsistent use of domain name in LLDP #698

Merged
merged 5 commits into from Apr 17, 2018

Conversation

ktbyers
Copy link
Contributor

@ktbyers ktbyers commented Apr 12, 2018

Cisco had an inconsistent use of domain name in LLDP brief (as compared to other IOS devices). This was causing a failure.

@ktbyers
Copy link
Contributor Author

ktbyers commented Apr 12, 2018

@bdlamprecht Please review the test data that I included here and make sure you are okay with me including it (I scrubbed it a bit).

@bdlamprecht
Copy link
Contributor

Wow, that's a great turnaround time, much better than I anticipated!

I see you changed the domain names and such, but I'd prefer to clean up some of the hostnames, IPv4 , and IPv6 addresses as well since this is going to be publicly available for the foreseeable future.

The specific strings that I'd request to be altered are the following:
bld-XXX-XXX to 000-XXX-XXX (or something simliar, just remove the bld part)
and
9.88.X.X to 10.0.X.X
as well as
2620:1F7: to 2001:DB8:

I'm happy to cleanup the test data if you'd like me to. Let me know.
Thanks again!

@ktbyers
Copy link
Contributor Author

ktbyers commented Apr 12, 2018

@bdlamprecht Okay, see updated strings in the test data. Just let me know if anything else needs updated in the test data.

@coveralls
Copy link

coveralls commented Apr 12, 2018

Coverage Status

Coverage increased (+0.02%) to 78.943% when pulling 88f8f36 on lldp_fix_blamprecht into c55c7c0 on develop.

@bdlamprecht
Copy link
Contributor

That looks good, except you missed the line with the Port Description: bld/XXXX.

@ktbyers
Copy link
Contributor Author

ktbyers commented Apr 12, 2018

@bdlamprecht Okay, fixed.

@bdlamprecht
Copy link
Contributor

Great, that looks good. I appreciate it very much!

@ogenstad
Copy link
Contributor

+1 from me. Ran into this issue yesterday when writing a tutorial and using an old switch. This PR fixes the issue.

Please merge. :)

@dbarrosop dbarrosop self-requested a review April 17, 2018 07:36
@ktbyers ktbyers merged commit cf0101d into develop Apr 17, 2018
@ktbyers ktbyers deleted the lldp_fix_blamprecht branch December 13, 2018 03:11
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.

None yet

5 participants