Skip to content

Comments

Improve docker images and add a makefile for local builds#503

Closed
pox wants to merge 2 commits intomempool:masterfrom
pox:docker-fixes
Closed

Improve docker images and add a makefile for local builds#503
pox wants to merge 2 commits intomempool:masterfrom
pox:docker-fixes

Conversation

@pox
Copy link
Contributor

@pox pox commented May 8, 2021

Please consider this PR a suggestion for a couple of improvements. If you like any of them, please let me know and I'll change as requested.

  1. The backend image doesn't have a proper PID entrypoint. Just a CMD. Because of this, for example, when you try to kill it with docker-compose down it hangs for a while (it's not forwarding signals properly. So I've added TINI. If you prefer another handler let me know. Also, if you prefer to just add TINI (it's just a sh file) to the repo, instead of downloading it from an external source, I'd be happy to do that as well.
  2. I've added a simple Makefile for building locally. I couldn't find the instructions for how to do so and had to dig into the GitHub CI yaml. This makes it easier for those who want to build from source and run on their own instead of pulling for docker hub.

@softsimon softsimon requested a review from wiz May 8, 2021 16:30
Copy link
Member

@wiz wiz left a comment

Choose a reason for hiding this comment

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

Hi @pox - sorry for the late response. Yes, we would prefer you to copy the file from the project instead of adding it as a third party dependency to the Dockerfile. Please preserve the original copyright/license and other relevant legal notices at the top of anything you copy from that project. Also I see there is a merge conflict now, so it would be cool if you can resolve that one as well.

Thank you for fixing this!

@wiz
Copy link
Member

wiz commented Jul 24, 2021

@knorrium since it's been over a month did you want to take over this PR ?

@wiz wiz added the enhancement New feature or request label Jul 24, 2021
@knorrium
Copy link
Member

@wiz I'm going to close this PR as it has merge conflicts. we can make these improvements when introducing the other Docker changes.

@knorrium knorrium closed this Jul 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants