-
-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
Gold and platinum integrations should implement diagnostic #117576
Comments
Hey there @fredrike, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) point documentation |
Hey there @fredrike, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) tellduslive documentation |
Hey there @janiversen, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) modbus documentation |
Hey there @dermotduffy, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) hyperion documentation |
Hey there @mdz, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) smarttub documentation |
Hey there @OnFreund, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) risco documentation |
Hey there @rytilahti, @shenxn, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) songpal documentation |
Hey there @exxamalte, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) gdacs documentation |
Hey there @exxamalte, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) geonetnz_quakes documentation |
Hey there @marciogranzotto, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) nightscout documentation |
Hey there @raman325, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) vizio documentation |
Hey there @zewelor, @shenxn, @starkillerOG, @alexyao2015, mind taking a look at this issue as it has been labeled with an integration ( Code owner commandsCode owners of
(message by CodeOwnersMention) yeelight documentation |
Is there documentation for the diagnostic platform implementation somewhere? A search for "diagnostics" returns only hits for |
@MatthewFlamm alas, no, not yet. But the implementation is usually rather simple, especially if one is using data update coordinator, for example: https://github.com/home-assistant/core/blob/dev/homeassistant/components/nam/diagnostics.py shows a very minimal implementation. It's important to go through the data and scrub the unwanted keys as issue reporters may be asked to upload these files. Another example showing much more data being scrubbed can be seen in tplink/diagnostic.py. |
That link would not work for the modbus integration! Be aware that the modbus protocol do not have diagnostic capabilities, so real diagnostic would be nearly impossible to add. We could of course make a service call that turns on debug, which is just about as close you can get to "diagnostic". Please advice if the "should implement" will become a demand. That will exclude quite a number of integrations to maintain a platinum/gold status. Personally I think the correct way would be to define the diagnostic demands before asking integrations to implement "something". |
First of all I want to echo @janiversen's sentiment - if implementing diagnostics is a requirement, we should properly document it. In particular, it would be helpful to understand how it's used and what it's used for. |
A question can "async_get_config_entry_diagnostics" be used by integrations that are yaml based (I cannot find the platform as a configEntry). It seems to work at entity level, but e.g. modbus is connection oriented so there no extra information pr entity as to what is already available in the entity state. At connection level (platform) the only diagnostic available is connected/disconnected which the user already knows if all entities are unavailable. Another example is the fronius integration it uses the modbus integration for communication, so diagnostic what type of diagnostic should that integration provide ? But of course modbus integration can return a near empty dict, but I highly doubt it will help any users. And just to be sure, I think implementing diagnostic where available is a splendid idea but the "should" should be modified to "should if possible and it adds additional information". |
Looking at the how this works In this case where we are unsure that the redaction process is complete, is it sufficient to just add in the config entry data that is completed redacted? This gets to @janiversen comment that this would be useless, but only to satisfy the integration level requirement. |
You can always just compile your own set of data to return. The |
The requirement page at https://developers.home-assistant.io/docs/integration_quality_scale_index/ says: "When communicating with a device or service, the integration implements the diagnostics platform which redacts sensitive information." |
I mean, it still connects to a service. The only thing I could think off would be putting the language code in there. |
@janiversen in |
Modbus isn't config entry based, so does diagnostics even work for them? |
Yes Google Assistant SDK still connects to a service but my understanding of: "When communicating with a device or service" refers to https://developers.home-assistant.io/docs/architecture/devices-and-services/ which doesn't apply here. Putting just the language code in diagnostics isn't particularly useful. |
@joostlek that is my point from earlier, the definition is "When communicating with a device or service, the integration implements the diagnostics platform which redacts sensitive information.", and not like the other demands, which have "if available", so it seems it MUST be implemented even if it does not work. What I will be doing for modbus is to dump state of the entity...but honestly it is only to satisfy the quality demand, as it will not help in diagnosing a problem. There is also something that I do not understand, the diagnostic platform is targeted at end-users, but it seems the dict is not being translated (I might have missed it, but at least the example tplink/diagnostic.py, does not use strings.json ?? |
I think that wording comes from one of the Adr, modbus is a protocol integration like mqtt. And it doesn't connect with a specific device. |
The text says "device or service, modbus communicates with a specific device !! and it was added to this PR so surely someone thinks it should be done. |
I cannot edit the issue description, but nws is completed in #117587. Thanks for the pointers and quick merge! |
I have adjusted the description above. core/script/hassfest/manifest.py Lines 118 to 136 in d1bdf73
|
#118513 takes care of Google Assistant SDK |
The problem
As per home-assistant/developers.home-assistant#1512,
gold
andplatinum
integrations should implement diagnostic.I discovered via #117565 that the following integrations did not implement it.
If the integration is unable to implement diagnostic, then a PR should be opened to add a comment against the integration in
script/hassfest/manifest.py
, explaining why it cannot be implemented.core/script/hassfest/manifest.py
Line 118 in aaa5df9
What version of Home Assistant Core has the issue?
2024.4
What was the last working version of Home Assistant Core?
No response
What type of installation are you running?
Home Assistant OS
Integration causing the issue
No response
Link to integration documentation on our website
No response
Diagnostics information
No response
Example YAML snippet
No response
Anything in the logs that might be useful for us?
No response
Additional information
No response
The text was updated successfully, but these errors were encountered: