search

MasterGroosha / telegram-report-bot

A simple bot to handle reports from users in your chat
MIT License
110 stars 24 forks source link

Add Docker compatibility #2

Closed PRIHLOP closed 3 years ago

PRIHLOP commented 3 years ago

Add Docker compatibility to simple and fast launch. Also add documentation about Docker.

PRIHLOP commented 3 years ago

Hello.

Yes I agree that code must be clean and readable. I will fix all of you noted. Sorry I made hastily Pull request.

PRIHLOP commented 3 years ago

Hi. Well. I made changes and tested it. Can you accept my PR?

PRIHLOP commented 3 years ago

Sorry, I was also delete build-essential and python-dev packages as unused.

MasterGroosha commented 3 years ago

@PRIHLOP Thank you for your edits! Please take a look at my recent commits. I finally pushed completely refactored code including what you pointed to me.

PRIHLOP commented 3 years ago

I reviewed code and made some changes.

But best practices for work with Docker is config variables in container environment. Can you make read from environment first, and if variables is not present read it from config.py file?

MasterGroosha commented 3 years ago

@PRIHLOP Thank you for your edits. However I have some questions/issues with your PR:

1) I don't use config.py file anymore, but config.ini (uses Python configparser from standard library). INI-files are generally more readable by non-techy humans. 2) Is env.dist file automatically sourced by docker-compose? If yes, what's the difference between .env, .env.dist and simply entering variables in docker-compose.yml? 3) Why are you trying to downgrade docker-compose.yml version from 3.8 to 3.7? 4) Why are you sourcing timezone file from host? (I think I know the reason, but just to be sure)

Regarding "Can you make read from environment first, and if variables is not present read it from config.py file?" This sounds like a great idea, however this might be not the best approach if bot is used outside of Docker (deployed on host machine via Systemd). Suggestions?

PRIHLOP commented 3 years ago
  1. Ok, I was reverted use of config.ini file
  2. No, only .env file automatically sourced by docker-compose. In .env.dist file are variable names for use. In best practies docker-compose.yml file do not contain config variables, it must be in different file, and .env file is ideal.
  3. In different systems different versions have different compabilitiies, version 3.7 more stable for more distributives.
  4. It is more simple and stable way to configure timezone, host time zone used as point of start time count(I tried differnet ways, but only this more simple and stable).

I do not suggest to cut config file fully, only add possibility to receive variables from environment. For example, do check variables in environment first, if not present read it from file. Also best practices for Systemd https://serverfault.com/a/413408/349539 (and it more secure way, if use systemd service)

PRIHLOP commented 3 years ago

Hi.

What do you think about my changes and answer to your questions?

MasterGroosha commented 3 years ago

@PRIHLOP Hello. Sorry I've been a bit busy recently. I've tested some .env options and I think I'll drop config.ini in favor of dotenv file as you suggested, since I only have 4 options to store currently

MasterGroosha commented 3 years ago

Sorry it took so long! I've finally implemented what you requested, so please review this commit

As I noted before, I'm currently using self-hosted GitLab instance to host most of my code, so I cannot merge your PR and did the same manually instead. Closing the PR, but feel free to comment.