Skip to content
This repository has been archived by the owner on Sep 2, 2022. It is now read-only.

Docker MQTT deployment #48

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

Conversation

diogos88
Copy link
Contributor

Modified your project to deploy with docker-compose and fix the configuration implementation (mqtt).

Modified dockerfile for auto deployment
Modified mqtt_daemon.py to always use contract_id in topic
Copy link
Owner

@titilambert titilambert left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. I put some comments, I just need to understand some of your changes.
Globally, I don't understand:

  • Why you changed the dockerfile
  • Why you changed the mqtt push data
  • Why using docker-compose, when there is only one container ... a single docker command can do the trick

@@ -1,6 +1,9 @@
#!/bin/sh
set -e

# Copy config file if does not exists
rsync -a -v --ignore-existing config.yaml.sample config/config.yaml
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you doing this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the persistence of the mqtt config file. You want to create the file if it's missing in the config without overwriting it.

Copy link
Owner

Choose a reason for hiding this comment

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

I would not do that, I think it better to crash if the file doesn't exists instead of copying a bad config file. What do you think ?

pyhydroquebec/mqtt_daemon.py Show resolved Hide resolved

COPY requirements.txt ./
RUN pip install -r requirements.txt --force-reinstall --no-cache-dir
WORKDIR /usr/pyhydroquebec
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you change the working dir ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put everything in the same root folder instead of having one root folder for the code and one root folder for the config.

Copy link
Owner

Choose a reason for hiding this comment

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

OK, but /usr/SOMETHING it's a weird folder. I would use something like /usr/local/pyhydroquebec.
In fact, we are copyng application's sources, that's why I put /usr/src/app, but I should put /usr/local/src/pyhydroquebec.

What do you think using /usr/local/src/pyhydroquebec ?

@diogos88
Copy link
Contributor Author

Thanks for the review,

Why you changed the dockerfile
I modified the dockerfile to integrate the mqtt portion and the persistence of the config file. With mqtt added in the dockerfile, it will simplify users integration in their environment and domotic application.

Why you changed the mqtt push data
This part was modified to only have one topic per account/contract_id. Correct me if I am wrong but the balance topic should be in the contract_id topic.

Why using docker-compose, when there is only one container ... a single docker command can do the trick
I use docker-compose because it's easier to read and structure, that's the only reason. At the end it's the same thing as a docker run command.

@titilambert
Copy link
Owner

titilambert commented Dec 23, 2019

Thanks for your answers !!

Why you changed the dockerfile
I modified the dockerfile to integrate the mqtt portion and the persistence of the config file. With mqtt added in the dockerfile, it will simplify users integration in their environment and domotic application.

I'm sorry but I still don't understand those changes in the Dockerfile. Today, I'm using the current master docker image with MQTT without any issue. What are the issues that you're a trying to solve with theses change ?

Why you changed the mqtt push data
This part was modified to only have one topic per account/contract_id. Correct me if I am wrong but the balance topic should be in the contract_id topic.

Thanks for this one !

Why using docker-compose, when there is only one container ... a single docker command can do the trick
I use docker-compose because it's easier to read and structure, that's the only reason. At the end it's the same thing as a docker run command.

OK, Thanks for this one too !

@titilambert
Copy link
Owner

BTW, I will handle the merge conflict (I created it ...)

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.

None yet

2 participants