aristanetworks / anta

What do you call an ant with frogs legs?
https://anta.arista.com/
Apache License 2.0
46 stars 24 forks source link

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

Closed ecrosswhite closed 1 week ago

ecrosswhite commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 --> https://github.com/aristanetworks/avd/pull/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 commented 1 month ago

@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 commented 1 week ago

Issue is on the AVD side and was fixed by https://github.com/aristanetworks/avd/pull/4212