[Nexthop] Improve PlatformManager and platform configuration documentation#943
[Nexthop] Improve PlatformManager and platform configuration documentation#943marif-nexthop wants to merge 2 commits intofacebook:mainfrom
Conversation
2d82eda to
bcfd299
Compare
…r "platform_manager" category.
PlatformManager and platform configuration.
bcfd299 to
37c259b
Compare
Scott8440
left a comment
There was a problem hiding this comment.
I really appreciate all the work put into this documentation, but most of this feels unnecessary. In general we want to keep documentation as close to the code that it describes as possible. Most of the time this means comments in .thrift files.
I would also love to see some of this documentation effort put into improving the --help messages. There are several parts here which would fit nicely there.
There are a couple of pieces here which I do think are valuable, and I mentioned where we should move them.
| @@ -0,0 +1,55 @@ | |||
| --- | |||
There was a problem hiding this comment.
I feel like this should be handled by platform_manager --help. Any documentation here feels redundant and prone to becoming outdated if we make any changes to the CLI interface
| @@ -0,0 +1,85 @@ | |||
| --- | |||
There was a problem hiding this comment.
I believe documentation here fits better as comments in the thrift service definition itself.
| @@ -0,0 +1,129 @@ | |||
| --- | |||
There was a problem hiding this comment.
An explanation of presence detection as a feature is useful, but I feel the amount of documentation here is too much. Perhaps this would fit better in a "Features" section of the existing platform_manager.md
Just explain the basics of what this provides and enables. Details about actual config implementation should be left to the thrift file or the sample config.
| @@ -0,0 +1,88 @@ | |||
| --- | |||
There was a problem hiding this comment.
Since kmods.json is a requirement of the BSP, documentation of that should stay there.
Other pieces of the documentation here should be provided by --help
Details about what exactly happens during the package management step feels too detailed to be in documentation.
| @@ -0,0 +1,91 @@ | |||
| --- | |||
There was a problem hiding this comment.
See the existing platform_manager.md doc which provides a high-level overview
| @@ -0,0 +1,75 @@ | |||
| --- | |||
There was a problem hiding this comment.
Overview of exploration flow is valuable. If it doesn't already exist in platform_manager.md I would add that there.
Details of the status and error enums feels redundant/obvious.
|
|
||
| ## Naming Conventions | ||
|
|
||
| Symlink names should: |
There was a problem hiding this comment.
These conventions (except for "be descriptive") should be codified in ConfigValidator instead of a doc.
| @@ -0,0 +1,208 @@ | |||
| --- | |||
There was a problem hiding this comment.
Since block configs are probably the most confusing part of the configs it makes sense to have some documentation on them, but if i remember correctly there's already some pretty good documenation in the thrift file itself.
Examples should be put into the sample config instead of a doc.
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
PlatformManager is a core component of FBOSS platform software responsible for hardware discovery and device management. This PR improves the documentation by adding more information about PlatformManager and platform configuration.
platform_manager.mdintooverview.mdandmodeling.mdTest Plan
Viewed in a local docusaurus instance.