search

srl-labs / containerlab

container-based networking labs
https://containerlab.dev
BSD 3-Clause "New" or "Revised" License
1.56k stars 266 forks source link

replace table writer with go-pretty #2292 #2296

Closed hyposcaler-bot closed 1 day ago

hyposcaler-bot commented 1 week ago

@hellt for #2292 Lightly tested, but should be functional enough to judge if you like the change.

hyposcaler-bot commented 1 week ago

I was just targeting the replacement, tried to stick with the original logic that was there.

kaelemc commented 1 week ago

@hyposcaler-bot Tried it out for the fun of it.. Looks good.

image

hyposcaler-bot commented 1 week ago

Some smoke tests related to the inspect are failing, need to get a handle on the smoke tests, having some issued getting them to run locally on my machine. Then will poke and see what's up.

hellt commented 1 week ago

Sounds good. If time permits I'll check the PR on my flight today

If anything I will take care of the tests later as well

hellt commented 1 week ago

Re tests. The devcontainer should handle them I hope. And there is some documentation around it https://containerlab.dev/manual/dev/test/

hyposcaler-bot commented 1 week ago

Re tests. The devcontainer should handle them I hope.

Mostly does, but I get some failures locally on the tests, that don't seem to fail when github runs them.

For the failures when github runs the smoke test, think they are just failing because of the difference between │ and | when it goes to split the output of the inspect.

hellt commented 1 week ago

Yes, the failures are definitely about the table border char Perhaps the test should have a car that keeps the border char to make it easily adaptable

hyposcaler-bot commented 1 week ago

Think I have the smoke tests sorted, added a ${table-delimit} set to │ for the appropriate robot tests.

hellt commented 1 week ago

I did a few drastic changes to the table layout. And open to have a discussion around it https://discord.com/channels/860500297297821756/1307642193300688999

I expect more tests to fail due to each node to occupy two rows

hellt commented 1 day ago

shipping this baby thanks all, great work

codecov[bot] commented 1 day ago

Codecov Report

Attention: Patch coverage is 79.76190% with 17 lines in your changes missing coverage. Please review.

Project coverage is 51.39%. Comparing base (d442919) to head (5d66ede). Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
cmd/inspect.go 77.27% 11 Missing and 4 partials :warning:
runtime/docker/firewall/iptables/client.go 0.00% 1 Missing :warning:
runtime/docker/firewall/nftables/client.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2296 +/- ## ========================================== + Coverage 50.59% 51.39% +0.80% ========================================== Files 172 172 Lines 16770 16830 +60 ========================================== + Hits 8484 8649 +165 + Misses 7369 7257 -112 - Partials 917 924 +7 ``` | [Files with missing lines](https://app.codecov.io/gh/srl-labs/containerlab/pull/2296?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=srl-labs) | Coverage Δ | | |---|---|---| | [cmd/tools\_netem.go](https://app.codecov.io/gh/srl-labs/containerlab/pull/2296?src=pr&el=tree&filepath=cmd%2Ftools_netem.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=srl-labs#diff-Y21kL3Rvb2xzX25ldGVtLmdv) | `70.29% <100.00%> (+0.90%)` | :arrow_up: | | [runtime/docker/firewall/iptables/client.go](https://app.codecov.io/gh/srl-labs/containerlab/pull/2296?src=pr&el=tree&filepath=runtime%2Fdocker%2Ffirewall%2Fiptables%2Fclient.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=srl-labs#diff-cnVudGltZS9kb2NrZXIvZmlyZXdhbGwvaXB0YWJsZXMvY2xpZW50Lmdv) | `0.00% <0.00%> (ø)` | | | [runtime/docker/firewall/nftables/client.go](https://app.codecov.io/gh/srl-labs/containerlab/pull/2296?src=pr&el=tree&filepath=runtime%2Fdocker%2Ffirewall%2Fnftables%2Fclient.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=srl-labs#diff-cnVudGltZS9kb2NrZXIvZmlyZXdhbGwvbmZ0YWJsZXMvY2xpZW50Lmdv) | `71.16% <0.00%> (+3.32%)` | :arrow_up: | | [cmd/inspect.go](https://app.codecov.io/gh/srl-labs/containerlab/pull/2296?src=pr&el=tree&filepath=cmd%2Finspect.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=srl-labs#diff-Y21kL2luc3BlY3QuZ28=) | `76.60% <77.27%> (-2.61%)` | :arrow_down: | ... and [6 files with indirect coverage changes](https://app.codecov.io/gh/srl-labs/containerlab/pull/2296/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=srl-labs)