[WIP] Disk entry rework #67
Conversation
|
Since we merged #70, it's allowed a lot of simplification so I'm just going to (kind of) start afresh from master and force push over the commits already here - hope that's alright! |
The entire entry has been rewritten, and now relies exclusively on a single call to `df -P -k`. * Adds support for multi-line disk output (fixes #55). * Fixes #62 by using a standard `df` call. * Adds configurability to disk labels, and which disks to show. * Modifies `config.json`, `configuration.py` and README accordingly. More detail: Our single call to `df` now saves the results in a table-like format, so we can simply reuse this to extract our disks' mount-points, device paths and used/total blocks. This saves us on slow system calls, and the new structure allows for a more suitable list of dicts (which each represent a filesystem) to be stored in the `value` attribute of the entry. This in turn provides us with a highly configurable entry!
aac01b2 to
d898133
Compare
|
Yes that's perfect. Don't mind looking at/editing as needed the v4.8 milestone. Bye 👋 |
+ Adds missing `,` in README configuration example
+ Sets `blocks_to_human_readable` as private method and improves/DRY its implementation
+ Simplifies SSV parsing implementation (without dialect definition/usage)
+ Renames some configuration attributes
+ Sets default attributes to `None` as `combine_total` is `True` by default
+ Improves documentation and code formatting
+ DRY `output`'s conditional branches
- TODO (BUG) : No entry _name_ with below disk configuration :
-{
- "combine_disks": false,
- "disk_labels": false,
- "hide_entry_name": true`
-}
|
I'm back ! So I've started looking at your changes, and if you can assure me that your solution is 100% working with BTRFS partitions too, we'll benefit a lot from it 😮 🚀 Two questions for you though :
I've pushed some "improvements" (from my PoV), feel free to override/revert/edit as needed 👌 Bye Michael 👋 |
Basically rewrites everything in such a way that we now deal with dicts whose keys are mount points, and values are dicts of other data for that disk. Mount points are used as keys since they are globally unique. This simplifies quite a lot of the internal logic of `Disk`, likely resulting in faster execution (since it eradicates a few now-unneeded iterations of `df`'s output). Additionally: + Cleans up consistency in variable names and documentation, to use: * `mount_point` and "mount point" * `device_path` and "device path" * `filesystem` and "filesystem" + Modifies README to reflect usage of `null`.
I've tested it on three of my systems running BTRFS (one with a RAID-1 setup) and it seems to be working as expected on them. Since I also got the older core 2 duo machine running the other day, I'll try out some whacky configurations on that to test too 😄.
I've gone ahead and rewritten... a lot of the entry based on this, since I think it's a good idea 😅 - see 2017352. Let me know what you think of the changes I've made; it's removed at least two iterations of the
We could either let it happen, since I suppose the user did explicitly ask for it... Or, we could just add a check with
Agreed! This kind of change is why I thought I'd wait until we discussed it 😄 Oh, also, about changing Anyway, see you for now! |
+ Number of blocks is always a positive integer ; `abs` is irrelevant in our case + Optimizes list comprehensions when keys are not being used + Restores the `-k` flag passed to `df` call
```
{
"combine_total": false,
"hide_entry_name": true,
"disk_labels": false
}
```
Awesome !! I've just removed the optional dependency to
Love this !
As it's very very ugly, I opted for one more conditional check to avoid it.
If you're okay with latest changes, I'm all good to write/review/help you with them. I guess we will need many cases 😮
Actually, your first patch did not use flags at all 🤣 |
|
Sorry for the delay again, been a little busy recently!
This implementation looks good to me - I'll get on with writing some tests now and then you can review them if you'd like?
Ah... oops! I didn't even notice that 😆. Yeah, kinda annoying how it's not just a consistent block... but ah well. By the way -- sorry for some of the changes I revert accidentally in commits (like line 48 in disk.py) - I end up doing stuff like rewriting comments, then changing my mind and rewriting them back without realising I removed your changes. Sorry about that! Anyway, see you on the other side of test-writing 👍 |
|
No problem Michael ! I'm all good with the proposal. See you soon, and mostly, take your time/care 👌 |
- remove all previous tests. + add framework for other disk tests.
* Also add a "missing `df`" test for `test_disk_df_output_dict`.
Due to the non-guaranteed dict ordering.
|
So I've done the majority of the tests, I think - you can start looking over them if you'd like. Pretty sure there just needs to be another for the output entry labelling, then that's all of them done! |
Yet more rewriting code! 😨
Reason and / or context
This is a full rework of the
Diskentry in an effort to implement #55 and fix #62. It may also potentially be faster as it relies on only a single call todffor input, and nothing else.This also additionally fixes an (unreported) bug, where we don't actually calculate the correct space usage in
Diskcurrently... oops!How has this been tested ?
It seems to work fine so far... Some tests have been written for the new functionality, some are TBC. (This section to be completed properly before converting to a full PR 😁)
@HorlogeSkynet Could you let me know what you think of this approach? I'm up for changing it but basing all of our calculations around a single basic
dfcall like this is what I came up with after some thinking on how to approach reworking this entry. TIA!Behaviour changes
Since I'm not sure how
df -ldetermines local filesystems, I've taken a guess at defining "local" filesystems as all/dev/xxxdevices, excluding:Loop devices:-
Device mappers:- (since their partitions are already included!)
This is highly likely to be a change in behaviour from our existing implementation (and may be erronous)! From what I can tell, this excludes all
/dev/devices that are not local, while including all local disks only once (since their device-paths are deduplicated). This logic also hinges on all local disks always having an entry in/dev/on all Unix-like/POSIX OSes, which may possibly not be true.Types of changes :
Checklist :