Skip to content
This repository was archived by the owner on Aug 19, 2025. It is now read-only.

Conversation

@alex-oleshkevich
Copy link
Contributor

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
Contributor 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
Contributor

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.

@lovelydinosaur
Copy link
Contributor

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
Contributor 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.

@lovelydinosaur
Copy link
Contributor

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?

@alex-oleshkevich
Copy link
Contributor Author

Not to be merged. Will post it as gist.

@alex-oleshkevich
Copy link
Contributor Author

Published as gist - https://gist.github.com/alex-oleshkevich/68411a0e7ad24d53afd28c3fa5da468c

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants