Skip to content

Conversation

@Gikkman
Copy link

@Gikkman Gikkman commented Sep 29, 2021

Some pretty bare bones docker support, but for someone like me who likes running stuff via docker from the CLI, this works. We could probably improve it a lot in the future, by (somehow) caching the gradle dependencies in a separate step, and maybe providing a docker-compose file for easier volume mounting. But this is at least a start.

@Gikkman
Copy link
Author

Gikkman commented Sep 30, 2021

I added a docker-compose file too, since it is slightly easier to run with. Though, those using docker are probably already familiar with the ins and outs, but at least it shows a bit more explicit what nobs there are to tweak

@fklemme
Copy link

fklemme commented Oct 4, 2021

Excellent PR! Very clean and understandable code and docker files. 5/5 - would docker again! ;-)

I think the demo setup could be improved, though. A casual user might not be able to figure out the required steps to make it running. So maybe either put something like

echo "1x Storm Crow" > cards.txt
cd run/templates/normal
zip -r ../../../templates/chilli.zip *

in the README, or add the appropriate example files to the repository. Or hint the user towards one of the examples given in run/. Regarding the zip file, I don't see why this should be given as the example. I would rather consider copying the templates into the docker image and point to the (then included) normal template by default. That would make it much more convenient for a starting user. They could still provide own zipped templates through another mapped folder. (Also, the wiki doesn't say it but you can just copy run/templates to templates and then discard the .zip from the template parameter. This might be easier in this case as the folders already exist in the repository. No zipping required.)

To be totally fair, the lack of a good example is not the fault of this PR but rather the state of the current master branch. But it could be an opportunity to improve it. :)

@fklemme
Copy link

fklemme commented Oct 4, 2021

Also, when the example files don't exists upon running docker-compose, default folders will be created. Among it, cards.txt/. Not sure though if that can be prevented within reasonable effort.

@fklemme
Copy link

fklemme commented Oct 4, 2021

The created images folder and all outputs are owned by root by default. So you can't delete or move the rendered images. This is not a major problem, I believe, but it may be fixable. On Linux, you can pass --user $(id -u):$(id -g) to run with the local user/group id. I used this one before to prevent exactly this behavior. However, I don't know if that's portable to Windows. (Or even needed in Windows.)

@fklemme
Copy link

fklemme commented Oct 4, 2021

Text at the bottom is overlapping. It looks like the font size is too large. Maybe some font is missing?

1 Storm Crow

@Gikkman Gikkman changed the title Docker support WIP: Docker support Oct 4, 2021
@Gikkman Gikkman marked this pull request as draft October 4, 2021 21:17
@fklemme
Copy link

fklemme commented Oct 5, 2021

To fix the wrong font size at the bottom, Calibri must be installed. See issue #30.

I hope this setup is clearer, and explains a bit more
into detail how to use the docker setup, and what motivates
the design choices made. I've also tried to rectify the problem
with what user owns the generated files. I'll need someone
with an true linux environment (and not just a VM) to try
it out in practise though
No need to keep this folder around since we can mount
the templates from run/templates
@Gikkman
Copy link
Author

Gikkman commented Oct 6, 2021

So the changes I've done is :

  1. point to the correct dockerfile in docker-compose.yml
  2. mount run/templates instead of requiring a zip file, and let the docker cmd command use the normal template by default for --template
  3. provide a default cards.txt , which is mounted as cardslist , and let the docker cmd use that as default for --cards
  4. set user in docker-compose using templating, so it can be overriden of platforms that needs it. This makes a single command for building and running the docker image
  5. Added a separate README for docker, inside the docker folder, explaining docker setup a bit in detail

I think we should leave #30 for another PR, and focus on solving docker related stuff here.

After some input from others involved in this PR
I descided it was probably best to leave the file
owner problem as an optional solution, instead
of forcing it upon users.
I also clarified the docker readme even more, to
hopefully explain why its designed as it is
@Gikkman Gikkman marked this pull request as ready for review October 12, 2021 21:09
@Gikkman Gikkman changed the title WIP: Docker support Docker support Oct 15, 2021
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.

2 participants