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

replace table writer with go-pretty #2292 #2296

Merged
merged 19 commits into from
Nov 25, 2024

Conversation

hyposcaler-bot
Copy link
Contributor

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

@hyposcaler-bot
Copy link
Contributor Author

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

@kaelemc
Copy link
Contributor

kaelemc commented Nov 15, 2024

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

image

@hyposcaler-bot
Copy link
Contributor Author

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
Copy link
Member

hellt commented Nov 16, 2024

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
Copy link
Member

hellt commented Nov 16, 2024

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

@hyposcaler-bot
Copy link
Contributor Author

hyposcaler-bot commented Nov 16, 2024

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
Copy link
Member

hellt commented Nov 16, 2024

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
Copy link
Contributor Author

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

@hellt
Copy link
Member

hellt commented Nov 17, 2024

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
Copy link
Member

hellt commented Nov 25, 2024

shipping this baby
thanks all, great work

@hellt hellt merged commit 2269834 into srl-labs:main Nov 25, 2024
66 checks passed
Copy link

codecov bot commented Nov 25, 2024

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 ⚠️
runtime/docker/firewall/iptables/client.go 0.00% 1 Missing ⚠️
runtime/docker/firewall/nftables/client.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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 Coverage Δ
cmd/tools_netem.go 70.29% <100.00%> (+0.90%) ⬆️
runtime/docker/firewall/iptables/client.go 0.00% <0.00%> (ø)
runtime/docker/firewall/nftables/client.go 71.16% <0.00%> (+3.32%) ⬆️
cmd/inspect.go 76.60% <77.27%> (-2.61%) ⬇️

... and 6 files with indirect coverage changes

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.

3 participants