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

Adds MQTT backend #117

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

alex-oleshkevich
Copy link
Member

Based on work done in https://github.com/encode/broadcaster/pull/32/files

Differences:

  1. asyncio-mqtt replaced with aiomqtt
  2. backend code migrated to support library change (https://sbtinstruments.github.io/aiomqtt/migration-guide-v2.html)

Closes #32

Fernando Governatore and others added 14 commits April 22, 2024 17:43
  MyPy would complain the method has a "missing return" even though the
return is explicit. This seems to happen because of the "async" block.

  Putting the value that was returned in a variable and returning the
variable from outside the "async for" block makes MyPy happy.
  MQTT allows for binary messages, deciding for the user that we should
convert it to a string would force us to also decide on an encoding. Lets
leave this decision to the user.
@alex-oleshkevich alex-oleshkevich requested a review from a team April 22, 2024 16:46
This was referenced Apr 23, 2024
@alex-oleshkevich
Copy link
Member Author

@tomchristie could you please review this?

Comment on lines +56 to +60
# eclipse-mosquitto does not support configuration via environment variables
# so it is not possible to use github's service feature to start the broker
# instead, we start the broker using docker run command and stop it at the end of the job
- name: "Start MQTT broker"
run: "docker run -d -p 1883:1883 --name mqtt eclipse-mosquitto:2.0-openssl mosquitto -c /mosquitto-no-auth.conf"
Copy link
Member

Choose a reason for hiding this comment

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

Have tried to keep to a strict style within steps of:

  name: "Name"
  run: scripts/<something>

I'd prefer not to break that if possible.

@tomchristie
Copy link
Member

could you please review this?

Not sure - should we be including this in the package, or push it out to a repository / code sample that we can link to?
(Perhaps same goes for the Kafka backend too?)

Refs #118

@alex-oleshkevich
Copy link
Member Author

could you please review this?

Not sure - should we be including this in the package, or push it out to a repository / code sample that we can link to? (Perhaps same goes for the Kafka backend too?)

Refs #118

Okay, I will then close this PR w/o merging.
As of Kafka - it is already in master.

@tomchristie
Copy link
Member

I will then close this PR w/o merging.

Is it worth creating a gist or repo with the MQTT backend, that we link to as an example of a third party implementation?

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

4 participants