Eugeny / tabby-web

Tabby Web - an SSH/Telnet/Serial client in your browser.
https://tabby.sh
MIT License
867 stars 127 forks source link

Optimising Dockerfile #69

Closed modem7 closed 1 year ago

modem7 commented 2 years ago

Heya, great project! Looking forward to using it, hope you don't mind the PR!

Changes


Things to note:


Happy to discuss any changes/concerns!

Eugeny commented 2 years ago

Wow, this is fantastic! Nice trick with <<EOF, I'll use it in my other projects :D

P.S. check out the build failure, the new dockerize URL isn't available for arm64

modem7 commented 2 years ago

Wow, this is fantastic! Nice trick with <<EOF, I'll use it in my other projects :D

  • The cryptography package that is now in requirements.txt is a dependency of poetry, and at the time, latest cryptography required Rust to build and didn't have wheels available (it's not used in the runtime anyway).
  • The manage.sh is a bug indeed - before dockerize there used to be a wait script in /wait.
  • I'll mention the buildkit requirement in the Readme
  • I don't have a machine to test arm64 builds on unfortunately but will be happy to merge

P.S. check out the build failure, the new dockerize URL isn't available for arm64

Heya, glad it's acceptable!!

modem7 commented 2 years ago

Instead of Dockerise, would something like https://github.com/vishnubob/wait-for-it be a better solution (which I'm guessing is similar to what you used to have)?

This would both reduce dependencies on an extra (outdated) binary, and make things lighter as well (and kill two birds with one stone).

Eugeny commented 2 years ago

wait-for-it looks pretty good - I don't have any specific preference as long as it works

As for cryptography, going for a more recent version is fine (the one that's explicitly installed in the Dockerfile is only used in the build stage anyway)

modem7 commented 2 years ago

Heya,

What I'll do for now is revert the Dockerize URL so it downloads.

Then we can create another PR/do more testing on wait-for-it.sh etc etc etc in another PR.

That way we don't change too much too quickly causing issues if we need to revert.

Mostly also because work is taking up a lot of my time at the mo, so additional testing will take a while for me to investigate.

I'd say priority wise:

At some point, the dockerfile really needs investigating properly as building the backend + frontend like that may (read: probably will) cause issues later down the line. The last stage in the multistep build should be a single image with everything running together (if doing multi-service in one).

I'd say it should be relatively simple to just use the start script to start both services at once.