Skip to content

Docker support (#1) #9

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

Closed
wants to merge 1 commit into from
Closed

Docker support (#1) #9

wants to merge 1 commit into from

Conversation

CustomIcon
Copy link

  • added Docker support, users can build a small container for them selves without all the hesitations with one single command: docker build --tag telegram-bot-api:latest . and run with arguments (ENV Variables)
  • api-id on Docker is API_ID
  • api-hash on Docker is API_HASH

* Dockerfile Added
Copy link

@kolayne kolayne left a comment

Choose a reason for hiding this comment

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

Hi! I think that

  1. The timezone should not be hard-coded in the Dockerfile. I have a different timezone and would want to specify it while building/running the docker image

  2. There is no need to introduce new environment variables, see the Usage section of README.md:

specify them using the --api-id and --api-hash options or the TELEGRAM_API_ID and TELEGRAM_API_HASH environment variables.

  1. (I am not anyhow related to the Telegram team, but) I guess, your PR will be better ready to be merged to master if you add a line to readme about the ability to use docker (I think, the explanation in your PR description is good)

@CustomIcon
Copy link
Author

we dont need to set the timezone if i add an arg ARG DEBIAN_FRONTEND=noninteractive. already tested on my vps and it works flawlessly.
the only change i need to add is readme file explaining how to host on docker container. will do that today and ready to be merged ^_^

Copy link

@kolayne kolayne left a comment

Choose a reason for hiding this comment

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

Hi! Really sorry for bothering you with incomplete feedback. Here is a more detailed review

Comment on lines +19 to +20
RUN git clone --recursive https://github.com/tdlib/telegram-bot-api.git && \
cd telegram-bot-api/ && \
Copy link

Choose a reason for hiding this comment

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

git clone shouldn't happen during docker image build for several reasons which include:

  • I am unable to rebuild the image without completely disabling the cache because Docker doesn't know that the repository could have received updates, so this step will be cached
  • This way I am not able to build my own version of Telegram Bot API if I've modified sources

Instead, you should consider that your file is in the root directory of the project and use the context (i.e. run something like COPY . /telegram-bot-api inside of the Dockerfile and have all the waste .dockerignored)

Copy link
Author

Choose a reason for hiding this comment

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

so im thinking of adding COPY . /telegram-bot-api. it should be fixed.. already building the the container after i changed a few things in Dockerfile. i moved TZ as an ENV (not hardcoded)

Comment on lines +21 to +22
mkdir build && \
cd build
Copy link

Choose a reason for hiding this comment

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

I don't know if it's a serious issue, but these two lines are redundant. The next line will do this anyway

Comment on lines +26 to +27
RUN cmake .. -DCMAKE_BUILD_TYPE=Release \
&& cmake --build . --target install
Copy link

Choose a reason for hiding this comment

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

Shouldn't we use more than one CPU core to build the project? I think the easiest way to achieve parallel compilation, but keep the number of cores flexible, is to update this as:

Suggested change
RUN cmake .. -DCMAKE_BUILD_TYPE=Release \
&& cmake --build . --target install
RUN cmake .. -DCMAKE_BUILD_TYPE=Release \
&& cmake --build . --target install -- -j`nproc`

(which is going to make the compiler use all the cores by default) and control the number of cores available to docker (for example docker build ... --cpuset-cpus 0-3 will let using first 4 cores)

RUN cmake .. -DCMAKE_BUILD_TYPE=Release \
&& cmake --build . --target install

CMD ["telegram-bot-api","--api-id=$API_ID", "--api-hash=$API_HASH", "--local"]
Copy link

Choose a reason for hiding this comment

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

There is no need to introduce new environment variables, see the Usage section of README.md:

specify them using the --api-id and --api-hash options or the TELEGRAM_API_ID and TELEGRAM_API_HASH environment variables.

@kolayne
Copy link

kolayne commented Nov 5, 2020

By the way, you may want to pay attention to this and somehow collaborate with its author

@levlam
Copy link
Contributor

levlam commented Nov 5, 2020

This Dockerfile has a lot of drawbacks. It hides a lot of useful server options, creates a very big image and has a lot of other issues mentioned earlier. Even all of this is fixed, there still can't be a Dockerfile that fits everyone needs, so the Dockerfile is better created by the end user. For now we wouldn't add an official Dockerfile, but such a file can be added in the future.

@levlam levlam closed this Nov 5, 2020
@JulyIghor
Copy link

there still can't be a Dockerfile that fits everyone needs, so the Dockerfile is better created by the end user

Most of users will just copy a build commands from the readme, and if the same commands used in a Dockerfile we should get the same thing that fits everyone needs isn't it?

@levlam
Copy link
Contributor

levlam commented Nov 5, 2020

@JulyIghor By copying and running the build commands natively they receive a binary that perfectly fits their operating system. For example, the binary will receive security updates along with all other system software, will use the same shared libraries and so on. The Docker is an amazing software, providing a lot of advantages for its users with a neglible overhead. The commands for Docker image building will be the same, but the Docker image is intended to be portable, so it should have multiarch support, should be based on a smallest possible Linux image, provide an ability to use all features of the Bot API server (likely through ENTRYPOINT), and probably more. And such Dockerfile can be added in the future.

@JulyIghor
Copy link

JulyIghor commented Nov 5, 2020

@JulyIghor By copying and running the build commands natively they receive a binary that perfectly fits their operating system. For example, the binary will receive security updates along with all other system software, will use the same shared libraries and so on. The Docker is an amazing software, providing a lot of advantages for its users with a neglible overhead. The commands for Docker image building will be the same, but the Docker image is intended to be portable, so it should have multiarch support, should be based on a smallest possible Linux image, provide an ability to use all features of the Bot API server (likely through ENTRYPOINT), and probably more. And such Dockerfile can be added in the future.

Thanks for the answer but I still don't understand why you think that "there still can't be a Dockerfile that fits everyone needs".
The multiarch support is job of a build system, not a Dockerfile itself.
And the ENTRYPOINT feature is pretty old, so everyone have it. And if someone don't, the simple solution is just to add the binary name as first argument.

@kolayne kolayne mentioned this pull request Nov 5, 2020
@levlam levlam mentioned this pull request Dec 23, 2020
@lukaszraczylo
Copy link

lukaszraczylo commented Dec 27, 2020

#65 (comment)

Just in case you are still looking for the image ( both AMD64 and ARM64 ) for either your server or raspberry pi :)

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.

5 participants