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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Publish birth and will messages by default #37371
Publish birth and will messages by default #37371
Conversation
Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with an integration ( |
vol.Optional(CONF_WILL_MESSAGE): MQTT_WILL_BIRTH_SCHEMA, | ||
vol.Optional(CONF_BIRTH_MESSAGE): MQTT_WILL_BIRTH_SCHEMA, | ||
vol.Optional( | ||
CONF_WILL_MESSAGE, default=dict(DEFAULT_WILL) |
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 copy it?
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 want to avoid hard to find bugs because the DEFAULT_WILL
ends up modified somewhere. Is it unnecessary?
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 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 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.
You're validating against dict
in your test, not {}
. When you do {}
it will check the keys and their values and make a copy.
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.
You're right, I dropped the useless copy.
@@ -719,7 +740,7 @@ async def async_disconnect(self): | |||
|
|||
def stop(): | |||
"""Stop the MQTT client.""" | |||
self._mqttc.disconnect() | |||
# Do not disconnect, we want the broker to always publish will |
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.
Shouldn't HomeAssistant set the DEFAULT_PAYLOAD_NOT_AVAILABLE
explicitly and then disconnect gracefully here? Otherwise, it may take a while until the MQTT broker detects that the client disappeared, leading to an unnecessary delay...
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 connection to the MQTT server is actively broken, and the broker will immediately publish the will.
What you describe is an issue if the connection is not actively broken, in which case the TCP connection will linger until it times out.
If you think this is not the case, please provide a way to reproduce the issue you suggest.
Breaking change
MQTT birth message defaults to:
{"topic": "homeassistant/status", "payload": "online"}
MQTT will message defaults to:
{"topic": "homeassistant/status", "payload": "offline"}
MQTT will published also on clean connect from broker
Proposed change
MQTT birth message defaults to:
{"topic": "homeassistant/status", "payload": "online"}
MQTT will message defaults to:
{"topic": "homeassistant/status", "payload": "offline"}
MQTT will published also on clean connect from broker
Type of change
Example entry for
configuration.yaml
:# Example configuration.yaml
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale: