Docker image #42

Merged
Miroslavsckaya merged 13 commits from feature_docker into master 2022-08-13 19:41:20 +03:00
No description provided.
add Dockerfile
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
b8c91b351a
Dockerfile Outdated
@ -0,0 +1,13 @@
FROM python:alpine
Collaborator

It's better to fix our image to more specific image version.

As you're not installing any additional Alpine packages, you can mostly fix only Python version:

  • 3.10-alpine (preferred, more stable, but will require manual update in the future to change minor Python version)
  • 3-alpine (more flexible, but more likely to break over time)
It's better to fix our image to more specific image version. As you're not installing any additional Alpine packages, you can mostly fix only Python version: * `3.10-alpine` (preferred, more stable, but will require manual update in the future to change minor Python version) * `3-alpine` (more flexible, but more likely to break over time)
skobkin marked this conversation as resolved
@ -0,0 +2,4 @@
WORKDIR /bot
COPY . .
Collaborator

You can probably copy only Python files and requirements.txt. We don't need anything else in the image to run the app.

You can use .dockerignore file to acomplish that. Check this documentation and my example.

This file will tell ADD/COPY to ignore some files you don't want to see in the image.

You can probably copy only Python files and `requirements.txt`. We don't need anything else in the image to run the app. You can use `.dockerignore` file to acomplish that. Check [this documentation](https://docs.docker.com/engine/reference/builder/#dockerignore-file) and [my example](https://git.skobk.in/skobkin/magnetico-web/src/branch/master/.dockerignore). This file will tell `ADD`/`COPY` to ignore some files you don't want to see in the image.
skobkin marked this conversation as resolved
Dockerfile Outdated
@ -0,0 +6,4 @@
RUN pip install -r requirements.txt
ENV PYTHONUNBUFFERED='a'
Collaborator

As it's not important what exactly is the value of this variable, it's better to use more intuitive value like 1 or TRUE.

By the way, AFAIR you can omit the quotes here.

As it's not important what exactly is the value of this variable, it's better to use more intuitive value like `1` or `TRUE`. By the way, AFAIR you can omit the quotes here.
skobkin marked this conversation as resolved
Collaborator

Also don't forget to create Docker Compose stack for easy startup (and educational purposes).

Here some of my examples:

https://git.skobk.in/skobkin/docker-stacks

Also don't forget to create [Docker Compose](https://docs.docker.com/compose/) stack for easy startup (and educational purposes). Here some of my examples: https://git.skobk.in/skobkin/docker-stacks
add .dockerignore
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
2b4684275e
Dockerfile Outdated
@ -0,0 +7,4 @@
RUN pip install -r requirements.txt
ENV PYTHONUNBUFFERED=1
Collaborator

I'd recommend to also declare variables used by the app here. It can be done without specifying default value.

I'd recommend to also declare variables used by the app here. It can be done without specifying default value.
skobkin marked this conversation as resolved
add other used environment variables
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
6381661232
code style changes
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
505e5f1ad5
skobkin approved these changes 2022-07-25 23:32:12 +03:00
skobkin left a comment

Looks nice.

Looks nice.
add docker-compose.yml
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
dcc7dda040
remove invalid values in docker-compose.yml and add little documentation for environment values
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
a3327a505c
skobkin left a comment

As you decided to also add Docker Compose configuration here, we now have some things to improve again 😆

As you decided to also add Docker Compose configuration here, we now have some things to improve again 😆
@ -0,0 +6,4 @@
environment:
# DSN schema: postgres://username:password@hostname/database_name
# https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING
- "RSSBOT_DSN=${BOT_DSN}"
Collaborator

If you're using substitution, then it'd be also good to provide an example file like .env.dist which could be simply copied to the .env file and quickly edited before the run.

If you're using substitution, then it'd be also good to provide an example file like `.env.dist` which could be simply copied to the `.env` file and quickly edited before the run.
Collaborator

I also don't see why not to use the same variable name here to make .env file interoperable between direct python usage and Docker Compose.

I also don't see why not to use the same variable name here to make `.env` file interoperable between direct python usage and Docker Compose.
Collaborator

If you're providing docker-compose.yml with PostgreSQL service included, you can at least configure the database and DSN out-of-the box here.

If you're providing `docker-compose.yml` with PostgreSQL service included, you can at least configure the database and DSN out-of-the box here.
skobkin marked this conversation as resolved
@ -0,0 +11,4 @@
- "RSSBOT_TG_TOKEN=${BOT_TOKEN}"
# https://docs.python.org/3/howto/logging.html#logging-levels
- "LOG_LEVEL=${BOT_LOG_LEVEL:-INFO}"
container_name: rss_bot
Collaborator

It's better to remove that if you're not sure that you need to have static container name without stack prefix.

It's better to remove that if you're not sure that you need to have static container name without stack prefix.
skobkin marked this conversation as resolved
@ -0,0 +15,4 @@
depends_on:
- postgres
postgres:
Collaborator

You've probably forgot to define a network to use in postgres and rss-bot services.

You've probably forgot to define a network to use in `postgres` and `rss-bot` services.
Collaborator

Not relevant. Default network is enough.

Not relevant. [Default network](https://docs.docker.com/compose/networking/) is enough.
skobkin marked this conversation as resolved
skobkin added this to the MVP 0.1 milestone 2022-07-26 21:28:21 +03:00
add .env.dist
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
90ac75fc11
skobkin left a comment

Looks nice. Just a bit of polishing and let's merge it.

Looks nice. Just a bit of polishing and let's merge it.
.env.dist Outdated
@ -0,0 +1 @@
RSSBOT_TG_TOKEN=1234567890:yourbotstoken
Collaborator

I'd add database and logging configuration examples here too, but leave them commented to show that they're optional.

I'd add database and logging configuration examples here too, but leave them commented to show that they're optional.
skobkin marked this conversation as resolved
.gitignore Outdated
@ -7,3 +7,3 @@
# Database
/*.db
/*.db
Collaborator

Seems like you don't need that anymore.

Seems like you don't need that anymore.
skobkin marked this conversation as resolved
@ -0,0 +2,4 @@
services:
app:
image: miroslavsckaya/tg_rss_bot
Collaborator

Did you remove build on purpose?

Did you remove `build` on purpose?
skobkin marked this conversation as resolved
@ -0,0 +28,4 @@
restart: unless-stopped
volumes:
postgres_bot_db:
Collaborator

db-data maybe?

`db-data` maybe?
skobkin marked this conversation as resolved
add optional variables in .env.dist; remove *.db from .gitignore; add build command in docker-compose.yml
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
8a82511618
skobkin approved these changes 2022-07-28 00:31:52 +03:00
skobkin left a comment

LGTM.
But I would've made a couple of small fixes here.

LGTM. But I would've made a couple of small fixes here.
.dockerignore Outdated
@ -0,0 +13,4 @@
README.md
# Environment
.env
Collaborator

You forgot to also add .env.dist here 🙂
We definitely don't need it in the image.

You forgot to also add `.env.dist` here 🙂 We definitely don't need it in the image.
skobkin marked this conversation as resolved
.env.dist Outdated
@ -0,0 +5,4 @@
# POSTGRES_USER=xxx
# POSTGRES_PASSWORD=xxx
# POSTGRES_DB=xxx
# LOG_LEVEL=xxx
Collaborator

I'd add some default value for the LOG_LEVEL here because unlike Telegram credentials it can work out of the box and we already know all possible values.

I'd add some default value for the `LOG_LEVEL` here because unlike Telegram credentials it can work out of the box and we already know all possible values.
skobkin marked this conversation as resolved
@ -0,0 +1,32 @@
version: '3.7'
Collaborator

I just understood that README.md has no documentation about running the bot in Docker yet.

I think it's a good idea to fix that.

I just understood that `README.md` has no documentation about running the bot in Docker yet. I think it's a good idea to fix that.
skobkin marked this conversation as resolved
add running the bot and runnig update with Docker documentation to README.md; add .env.dist to .dockerignore; set default value to LOG_LEVEL in .env.dist
Some checks failed
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is failing
41b7eb32b6
.dockerignore Outdated
@ -0,0 +14,4 @@
# Environment
.env
.env.dist
Collaborator

Missing EOL (\n)

Missing EOL (`\n`)
skobkin marked this conversation as resolved
@ -7,3 +7,2 @@
# Database
/*.db
# Environment
Collaborator

Missing EOL (\n) on the last line

Missing EOL (`\n`) on the last line
skobkin marked this conversation as resolved
README.md Outdated
@ -46,1 +68,4 @@
```shell
docker-compose run app update.py
```
Collaborator

Missing EOL (\n)

Missing EOL (`\n`)
skobkin marked this conversation as resolved
change running and update documentation in README.md
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
8c6c256971
skobkin approved these changes 2022-08-13 16:22:10 +03:00
skobkin left a comment

Let's merge it.

Let's merge it.
Miroslavsckaya changed title from WIP:Docker image to Docker image 2022-08-13 19:40:29 +03:00
Miroslavsckaya deleted branch feature_docker 2022-08-13 19:41:20 +03:00
Miroslavsckaya referenced this pull request from a commit 2022-08-13 19:41:21 +03:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
#43 Docker image CI build
Miroslavsckaya/tg_rss_bot
Reference
Miroslavsckaya/tg_rss_bot!42
No description provided.