Skip to content

Add default retain value specific to birth and lastwill MQTT messages#27574

Closed
cicavey wants to merge 1 commit into
home-assistant:devfrom
cicavey:27573-birth-lastwill-retain
Closed

Add default retain value specific to birth and lastwill MQTT messages#27574
cicavey wants to merge 1 commit into
home-assistant:devfrom
cicavey:27573-birth-lastwill-retain

Conversation

@cicavey
Copy link
Copy Markdown

@cicavey cicavey commented Oct 13, 2019

Breaking Change:

MQTT birth and will messages will now be published with retain flag set by default in line with the documentation.

Description:

The documentation states that the retain flags defaults to True for birth / lastwill messages. Through testing, the retain flag defaults to False. Either the default should be changed or the documenation updated to reflect the reality. I have a fix and a PR is incoming.

Related issue (if applicable): fixes #

fixes #27573

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@cicavey cicavey requested a review from a team as a code owner October 13, 2019 03:27
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @cicavey,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link
Copy Markdown

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (mqtt) you are listed as a codeowner for? Thanks!

@MartinHjelmare MartinHjelmare changed the title Added default retain value specific to birth and lastwill MQTT messages Add default retain value specific to birth and lastwill MQTT messages Oct 13, 2019
@emontnemery
Copy link
Copy Markdown
Contributor

It seems the birth and last will messages have been defaulting to non retained since support was added in #4381 almost 3 years ago. Maybe it's better to update the docs?

@cicavey
Copy link
Copy Markdown
Author

cicavey commented Oct 13, 2019

I thought more about this after I submitted the PR last night. I feel like having retained messages for birth/will is useful as a status indicator regarding the state of HA. Without the retention the messages are only useful at the exact instant they're sent which may be missed in a dynamic service environment. This must be the original intent of having these message default to retained. Could changing the retained state negatively effect applications that use the messages? It seems like it would only be beneficial. I'm happy to simply update the docs.

I'll admit this is a little selfish for me for two reasons. I'm trying hard to get hacktoberfest credit with meaingful contributions (I guess updating docs could be another PR) and I have an internal service that communicates over MQTT and knowing if HA is up/down via bith/will messages is very handy to change application behavior.

@emontnemery
Copy link
Copy Markdown
Contributor

@cicavey You're maybe right, retained messages make more sense for birth and last will messages. I updated the issue description with breaking change note.

@fabaff, @balloob Birth and will messages have been documented as having the retain flag set from when it was first documented in home-assistant/home-assistant.io@a03edbc, do you think it makes more sense to change the docs or the implementation?

@towerhand
Copy link
Copy Markdown

If I’m not mistaken the lastwill message doesn’t get sent during a HA restart or proper shutdown.

@emontnemery
Copy link
Copy Markdown
Contributor

@towerhand yes, sure. Last will is published by the server if the client's connection is unexpectedly broken.

@towerhand
Copy link
Copy Markdown

Looks to me that the behavior need to be changed if the the retain option is added and HA should send a last will message during shutdown and restart.

@emontnemery
Copy link
Copy Markdown
Contributor

@cicavey Let's update the docs instead, to not risk breaking anyone's installation. Please open a docs PR instead of this one.

cicavey added a commit to cicavey/home-assistant.io that referenced this pull request Oct 16, 2019
@lock lock Bot locked and limited conversation to collaborators Oct 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MQTT retain flag defaults to incorrect value for birth/lastwill according to docs

4 participants