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

VerifyLLDPNeighbors test adding an extra carriage return, causing an extra line in Markdown report #752

Closed
ecrosswhite opened this issue Jul 8, 2024 · 9 comments

Comments

@ecrosswhite
Copy link

Found that the LLDP Neighbor check is adding an extra line break moving the interface to its own line in the Markdown report.

| 6 | dc2-bsl-01 | Connectivity | VerifyLLDPNeighbors | Verifies that the provided LLDP neighbors are connected properly. | Local: Ethernet1/1 - Remote: dc1-bsl-01 Ethernet1/1 | FAIL | No LLDP neighbor(s) on port(s):
   Ethernet1/1 |

If I modify the file anta/tests/connectivity.py line 167 from failure_messages.append(f"{failure_type}:\n {ports_str}") to failure_messages.append(f"{failure_type}: {ports_str}") my output Markdown file is formatted correctly;

| 6 | dc2-bsl-01 | Connectivity | VerifyLLDPNeighbors | Verifies that the provided LLDP neighbors are connected properly. | Local: Ethernet1/1 - Remote: dc1-bsl-01 Ethernet1/1 | FAIL | No LLDP neighbor(s) on port(s):   Ethernet1/1 |
@gmuloc
Copy link
Collaborator

gmuloc commented Jul 9, 2024

Hi @ecrosswhite - thanks for opening this. the new line is probably present in case multiple ports are in the string to have a nicer display in some other format mode.
Can you please share how you are using the markdown report - is it coming from AVD maybe? Do you see this breaking the table formatting then? (I guess it does)

@ecrosswhite
Copy link
Author

Hello @gmuloc I was thinking the same that the new line was for the case of multiple interfaces. My understanding is the ports_str variable already includes one new line ports_str = "\n ".join(ports) so by having two new lines together in the failure_message it is causing the interface to bump down into the next line in the report.

Yes, this report is coming through via AVD and it doesn't break the table exactly, just causes the interface to be on its own line rather than in the relevant cell.

@nathanmusser
Copy link

nathanmusser commented Jul 11, 2024

Having this same issue, I resolved it locally by patching the md_report.py file in the AVD project to replace newlines with <br /> in messages as it generates rows.

I feel like AVD is the appropriate place to resolve this. As it seems reasonable for ANTA to include newlines in its output, and it's the responsibility of any formatter (like AVDs md generator) to properly escape anything that might break the format it's generating. As the newline in this case is a problem for md but wouldn't necessarily be a problem for other consumers of the ANTA report.

Wanted to run this by here before submitting a PR to AVD though. I noticed that the message field is a list. So it could be considered appropriate to return the new lines as multiple items in the list as part of the test. Regardless AVD should probably by sanitizing anything it's generating in a way that won't break the md formatting, but ANTA might want to make a design decision on whether newlines can be expected in output or not.

@ecrosswhite
Copy link
Author

@nathanmusser That is a good point, I hadn't considered patching it on the AVD side but it seems the best place to fix the issue. I agree that whatever tool is generating the report, AVD in this case, should be the one to fix issues from how it generates the report.

@nathanmusser
Copy link

Just noticed that #740 is adding a native markdown generator to ANTA and already has a method for sanitizing the output. Hopefully AVD migrates to this generator once it's released.

@gmuloc
Copy link
Collaborator

gmuloc commented Jul 12, 2024

Just noticed that #740 is adding a native markdown generator to ANTA and already has a method for sanitizing the output. Hopefully AVD migrates to this generator once it's released.

this is the plan - cc @carl-baillargeon

@carl-baillargeon
Copy link
Contributor

@nathanmusser That is correct. We are transferring the report (MD/CSV) generators to ANTA directly and will eventually use them when ANTA will be integrated to PyAVD --> aristanetworks/avd#4196

Since the integration will take a little bit of time (AVD 5.0) maybe? Please feel free to open a PR in AVD for eos_validate_state to fix the current report. Thanks

@nathanmusser
Copy link

@carl-baillargeon thanks for the heads up, I "borrowed" the safe markdown method from ANTA and opened a PR using it against AVD.

@carl-baillargeon
Copy link
Contributor

Issue is on the AVD side and was fixed by aristanetworks/avd#4212

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

No branches or pull requests

4 participants