Skip to content
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

Abstract MQTT from communication and make mqtt optional #4462

Merged
merged 23 commits into from
Nov 24, 2022

Conversation

NickM-27
Copy link
Sponsor Collaborator

No description provided.

@netlify
Copy link

netlify bot commented Nov 21, 2022

Deploy Preview for frigate-docs canceled.

Name Link
🔨 Latest commit 0826711
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/637e1c14eea8db0009baecda

@NickM-27 NickM-27 marked this pull request as ready for review November 21, 2022 22:36
@NickM-27 NickM-27 changed the title Make MQTT optional Abstract MQTT from communication and make mqtt optional Nov 22, 2022
frigate/comms/dispatcher.py Outdated Show resolved Hide resolved
frigate/comms/dispatcher.py Outdated Show resolved Hide resolved
@blakeblackshear blakeblackshear merged commit 6c09784 into blakeblackshear:dev Nov 24, 2022
@@ -39,6 +39,8 @@ It is not recommended to copy this full configuration file. Only specify values

```yaml
mqtt:
# Optional: Enable mqtt server (default: shown below)
enabled: True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since for HA users it's mandatory to enable mqtt AND set the host, user and passwords, I would recommend having MQTT disabled by default.

At least Frigate would work without having to set anything.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean, with this if nothing is set frigate should still run just fine it'll just have errors in the logs.

And if I set it to disabled by default then that would break every existing users setup by default. even if we had a huge font note in the changelog, there'd still be plenty of users who made issues because they didn't read the changelog and their automations stopped working.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if I set it to disabled by default then that would break every existing users setup by default. even if we had a huge font note in the changelog, there'd still be plenty of users who made issues because they didn't read the changelog and their automations stopped working.

I thought breaking changes were not much of a problem as long as properly documented, they seem to happen in almost every major 0.x releases anyway.

But it's up to you. It was just a suggestion :)

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're definitely not wrong but trying to move in the direction of less breaking changes.

@NickM-27 NickM-27 deleted the optional-mqtt branch November 29, 2022 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants