Add support for multiple network interface stats #119
Conversation
|
Hi @aacero thanks for your contribution! The code looks very clean. I ran it locally and didn't find any issues, although I don't have multiple active network interfaces. It is common in this part of Regolith (status indicators) that a seemingly benign change causes regressions for users with various configurations we didn't consider. Unfortunately due to the nature of i3blocks these errors can result in big problems like spamming syslog or consuming 100% of a cpu core. Unrelated to your PR, but generally I have low confidence that any indicator shell script changes won't result in some regressions for some users. Given this I propose that in the short term we rename this such that the existing script and your updates are available independently for users. Those wishing for network status for multiple interfaces would install another package, say |
|
@kgilmer -- renaming sounds good to me |
|
Great, can you update the PR such that your changes are in new file as described? Then I'll merge and add a i3xrocks config script and package it for testing. |
|
If you rebase from |
|
Will do. I currently don't have a way to test so it might be a week or two
before I resubmit a PR
…On Sun, Jul 4, 2021, 11:20 Ken Gilmer ***@***.***> wrote:
If you rebase from master the test regression should disappear @aacero
<https://github.com/aacero>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#119 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFSDKETQ5ELHL3JSH3IUQDTWB34TANCNFSM426VZ2DQ>
.
|
|
|
||
| IF_PATH="/sys/class/net/${IF}" | ||
| # Original behavior: use the following to include only routable intefaces: | ||
| declare -a IFACES="${BLOCK_INSTANCE:-$(ip route show default | awk '!/(ppp|tun|tap)/ { print $5 }')}" |
There was a problem hiding this comment.
I hope this PR is picked up again. I also think the awk pattern here should be something like (ppp|tun|tap|vpn0) to ignore vpn connections, but that may be subject to debate.
Some of my machines have multiple default interfaces and this caused the net-traffic script to not work. This pull request adds support for multiple interfaces and is backward compatible with the original script.