-
Notifications
You must be signed in to change notification settings - Fork 8
Add the supported targets page #88
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
base: main
Are you sure you want to change the base?
Conversation
|
When testing against autoapi, I did notice that a few projects fail when generating the api documentation. These are:
I'll make a ticket for those projects |
|
How the table generation works. We have some default csv files that contain the descriptions. the The output format depends on the type of file being generated (loaders looks different from filesystem for example), but it comes down to: the supported-targets.rst file uses a |
38dd213 to
981e014
Compare
Schamper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the original ticket did warn that an automated approach might be hard and not lead to a high enough quality result. If you do still intend to attempt to do it in an automated fashion, it must be better and lead to as high a quality result as a handcrafted table.
| return [loader.realattr for loader in LOADERS_BY_SCHEME.values()] | ||
|
|
||
|
|
||
| SUPPORTED_SYSTEMS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this so complex? What's wrong with just iterating i.e. loader.LOADERS?
Besides, it's giving a wrong impression on things that are supported. Not everything that's in dissect/target/filesystems is actually supported or should be listed here. Who cares about ITunesFilesystem if it can't actually be manually opened or selected? What value is there in knowing that it exists? I do care about the ITunes Backup loader (not ITunesLoader, what's that?).
This page should reflect what a user can expect from Dissect, not a factual representation of what modules and classes exist. I open the API documentation if I was looking for that.
docs/source/supported-targets.rst
Outdated
| .. Descriptions can be reworded by changing _defaults/loaders.csv | ||
| .. csv-table:: Supported Loaders | ||
| :header-rows: 1 | ||
| :file: /supported_targets/loaders.csv | ||
| :widths: 15 10 25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSV approach sucks a bit, is there not a better way? Maybe a custom table directive where you can manually override entries, keyed on the module name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought a bit further about it, and having a plugin automatically generate it will be more effort than it is worth. So I opted to write it manually
The contents get generated from the files inside of defaults_/*.csv
These were generated by taking the first line from the class its docstring
981e014 to
3e3f6f4
Compare
3e3f6f4 to
3d50ee3
Compare
Schamper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, remember that this page should not have too much implementation details. Someone reading this should not have to know what a plugin is. Any and all links or references to more in-depth or API stuff should be optional links, and not part of the main text.
Also, maybe vibe some sentences to make them flow nicer.
| Shell </target-shell> | ||
| Mount </target-mount> | ||
| Acquire </acquire> | ||
| Supported Targets </supported-targets> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put this just above tutorial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a change in this PR, but why does this page like to dissect.target and not dissect? And "github" is not even properly capitalized...
Get in touch, join us on
github <https://github.com/fox-it/dissect.target>_!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question
| ~~~~~~~~~~~~~~~~~ | ||
|
|
||
|
|
||
| Dissect includes a range of ``OSPlugins`` that help identify the operating system present on a target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Dissect includes a range of ``OSPlugins`` that help identify the operating system present on a target. | |
| Dissect includes a range of ``OSPlugins`` that help identify the operating system present on a target. |
Unnecessary detail? Just say we support various operating systems.
| :header-rows: 1 | ||
| :widths: 20 | ||
|
|
||
| * - Operating System |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some improper capitalization in this list.
| :widths: 20 | ||
|
|
||
| * - Operating System | ||
| * - :class:`Android <dissect.target.plugins.os.unix.linux.android._os.AndroidPlugin>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe don't list this alphabetically, but rather sorted by inheritance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll sort it by inheritance
| * - Child Target | ||
| - Description | ||
| * - :class:`Colima <dissect.target.plugins.child.colima.ColimaChildTargetPlugin>` | ||
| - Child target plugin that yields Colima containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Child target plugin that yields Colima containers. | |
| - Colima containers. |
This prefix is unnecessary. And what is Colima? Just add one line for everything.
Co-authored-by: Erik Schamper <[email protected]>
_os.pymodulescloses #84