feat(iftop): add iftop-scanning #484
Conversation
this is not even an MVP, but I would like it to exist to allow per client json aggregation also, a future use is a stream response
georgettica
force-pushed
the
georgettica/feature/iftop
branch
from
1ce10b1 to
c5c9858
Compare
georgettica
changed the title
Signed-off-by: Ron Green <[email protected]>
Signed-off-by: Ron Green <[email protected]>
georgettica
force-pushed
the
georgettica/feature/iftop
branch
from
c5c9858 to
338c4a0
Compare
|
@kellyjonbrazil this was at last done and has tests, now we can move towards merging this |
|
code currently does not pass, but works locally :) I will fix the issue with the test tomorrow :) |
jc/parsers/iftop.py
Outdated
| # for backwards compatibility, preset all fields to None | ||
| interface_obj: Dict = { | ||
| "device": None, | ||
| "ip_address": None, | ||
| "mac_address": None, | ||
| "clients": None, | ||
| "total_send_rate": None, | ||
| "total_receive_rate": None, | ||
| "total_send_and_receive_rate": None, | ||
| "peak_rate": None, | ||
| "cumulative_rate": None, | ||
| } |
There was a problem hiding this comment.
You can delete this section. This was for backwards compatibility for another parser.
jc/parsers/iftop.py
Outdated
| "cumulative_rate": None, | ||
| } | ||
|
|
||
| interface_item: Dict = interface_obj.copy() |
There was a problem hiding this comment.
This can be a normal empty dict. I was just copying in the other parser so it would keep the default values for backward compatibility.
|
Looking good! |
|
@kellyjonbrazil fixed review comments and tests still pass :) |
|
I am not sure if it's needed, but I needed the filesize to be converted to int bytes, so I added the functionality I can extract the code to a generic location in utils if you deem it helpful @kellyjonbrazil |
completions/jc_bash_completion.sh
Outdated
There was a problem hiding this comment.
You can delete the completion scripts as I auto generate them with the documentation.
There was a problem hiding this comment.
Or maybe not delete them, but revert them back to original. You should only need to update the parser.py file and the lib.py file for a new parser. I'll handle all the documentation and completion updates when creating the release. This will make the PR smaller and easier to reason about.
requirements.txt
Outdated
There was a problem hiding this comment.
We shouldn't introduce any new requirements/dependencies. There may be a way to vendor this in or create our own function that does something similar.
There was a problem hiding this comment.
It looks like we can pull the tokenize() function and the parse_size() functions from the library. If you want, you can put them directly in the parser and I'll move them to utils.py later.
There was a problem hiding this comment.
|
I expected that we won't add new requirements, and I am ok with reverting the completions. I will do these in a few hours and ping, hopefully we can merge this today 🤞 |
|
Sweet! I'm on a plane now coming at you from 30k feet and will be traveling a bit this week, so I may be able to merge this later this week or this weekend. Also, I can possibly take your work and turn this into a streaming parser. |
|
I am ok with this as long as we are fine with merging this first. as it is already almost mid week this also soundslike a short time to wait. |
Signed-off-by: Ron Green <[email protected]>
jc/parsers/iftop.py
Outdated
| @@ -0,0 +1,622 @@ | |||
| """jc - JSON Convert `iftop` command output parser | |||
|
|
|||
| No `iftop` options are supported. | |||
There was a problem hiding this comment.
Are there only certain options that are supported or can we remove this if all options are supported? From the example and tests it looks like at least some options are supported.
There was a problem hiding this comment.
Oh, I just didn't read this 🤦♂️
I'll see if I can fix it via the web browser.
To be honest I just always pipe it in because it requires sudo and I didn't want to run jc as sufo
this is not even an MVP, but I would like it to exist to allow per client json aggregation
also, a future use is a stream response