Skip to content
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

Add dockerfile generator #1600

Closed
kraih opened this issue Nov 7, 2020 · 14 comments · Fixed by #1602
Closed

Add dockerfile generator #1600

kraih opened this issue Nov 7, 2020 · 14 comments · Fixed by #1602

Comments

@kraih
Copy link
Member

kraih commented Nov 7, 2020

I want to be able to do

$ ./myapp.pl generate dockerfile

and

$ ./script/my_app generate dockerfile

to get a minimal but working Dockerfile for a Mojolicious app. It should probably start with an official perl container from Docker Hub, run cpanm --installdeps to install dependencies, and finally start the app in prefork mode. Nothing fancy, just enough to get started.

And of course there should be a new recipe in the Deployment section of the cookbook. Describing how to declare dependencies and also showing a minimal Dockerfile to build a container for a Mojolicious app.

@mohawk2
Copy link
Contributor

mohawk2 commented Nov 7, 2020

Comment moved to graphql-perl/graphql-perl#29.

@kraih
Copy link
Member Author

kraih commented Nov 7, 2020

@mohawk2 I strongly disagree, getting it "right" (for whatever definition) from the start is not important at all. All we have to do is give our users a minimal starting point for their first experiments with containerization. Any complexity with multiple dockerfiles would be counter productive. More complex generators can be shipped as 3rd party modules on CPAN.

@zakame
Copy link
Contributor

zakame commented Nov 7, 2020

(Putting my docker-perl hat on...)

Yep, probably simplest is a ./script/myapp generate dockerfile that produces:

FROM perl:5.32
ADD . /app
WORKDIR /app
RUN cpanm Mojolicious && cpanm --installdeps .
EXPOSE 3000
CMD ["./script/myapp","prefork"]

There are a few assumptions like whether there's a cpanfile or Makefile.PL already around, what ports to use, etc. but I suppose these can be parameterized/templated (or just document calling script/myapp generate makefile prior.) Also probably worth noting (but not too important) is the base perl image to use; the regular variant is heavy but good for general development, but for documenting deployments it might be worth using a :slim variant...

@kraih
Copy link
Member Author

kraih commented Nov 7, 2020

The default port 3000 should be fine, no need to make everything configurable. I trust our users do edit a dockerfile if they have to. 😉 The generator should probably output a warning though if a Makefile.PL or cpanfile is missing. Caring about a :slim variant doesn't seem that important, maybe as a side note in the cookbook recipe.

@tianon
Copy link
Contributor

tianon commented Nov 7, 2020

Given it's generating in the context of a my_app / myapp.pl, do you think it makes sense to do things like WORKDIR /opt/my_app (instead of the generic /app) and/or pin cpanm Mojolicious to the particular version being currently used (like the makefile generator does)?

I would imagine if folks have a cpanfile or Makefile.PL that it already lists Mojolicious, so we probably should do one or the other of cpanm Mojolicious and cpanm --installdeps (not both) right?

FROM perl:5.32
RUN cpanm Mojolicious@8.64
WORKDIR /opt/my_app
COPY . .
EXPOSE 3000
CMD ["./script/my_app", "prefork"]

or:

FROM perl:5.32
WORKDIR /opt/my_app
COPY . .
RUN cpanm --installdeps .
EXPOSE 3000
CMD ["./script/my_app", "prefork"]

@tianon
Copy link
Contributor

tianon commented Nov 7, 2020

For the cookbook (to get folks thinking about how to improve their cacheability, etc), I would probably go slightly more complex with something like this, which would dovetail nicely with the makefile generator's output:

FROM perl:5.32
WORKDIR /opt/my_app
COPY Makefile.PL .
RUN cpanm --installdeps .
COPY . .
EXPOSE 3000
CMD ["./script/my_app", "prefork"]

(This makes sure the cpanm only reinstalls deps when Makefile.PL changes, although doesn't work in more complex cases like sourcing VERSION from another module, so isn't a sane default for the generator.)

@kraih
Copy link
Member Author

kraih commented Nov 7, 2020

@tianon The VERSION is hardcoded in the Makefile.PL created by ./myapp generate makefile though.

@tianon
Copy link
Contributor

tianon commented Nov 7, 2020

Right, so my thought is that this should either use cpanm Mojolicious@8.64 (matching the makefile generator) or should assume users use that makefile generator and instead use just cpanm --installdeps .

@tianon
Copy link
Contributor

tianon commented Nov 7, 2020

Do you think it makes sense to change the output depending on whether a Makefile.PL exists, or should we just assume that one does and/or throw a warning if it is missing?

What about for a Lite app - should the result copy just the single script or should it still be basically the same?

(Edit: I've pushed the very absolute basics to master...tianon:dockerfile, shamelessly copying from makefile.pm -- happy to make a PR or do more on it with some guidance but don't want to pester 😄 IMO it definitely needs to get clever with ->app to at least get the right app name in there 😇)

@kraih
Copy link
Member Author

kraih commented Nov 7, 2020

If you're creating a dockerfile for a lite app i think we should assume that there's more stuff to be copied in the directory (like templates and static files at least).

Edit: Yes, it should detect the right script to put into the dockerfile.

@tianon
Copy link
Contributor

tianon commented Nov 8, 2020

I've updated master...tianon:dockerfile with a bit more - I'd love some feedback (I'm admittedly a little bit over my own head, but trying to work it out 😅 ❤️).

One thing I'm not a huge fan of is that my $exe detection currently will turn mojo generate dockerfile into something like ../../../bin/mojo (for a system installed mojo binary), so I'm thinking it might make sense to add some detection there and convert anything that starts with ../ into script/$name instead?

I'm happy to adjust or even let someone else take over if I'm completely on the wrong track, but I'm also enjoying my attempt. 😄

@kraih
Copy link
Member Author

kraih commented Nov 8, 2020

@tianon That might just be the best we can do for script detection. Personally, i don't care too much about the mojo generate dockerfile case (it could just as well use ... if $self->app->isa('Mojo::HelloWorld') to set some static default). Or we could use $ENV{MOJO_EXE} || ('script/' . $self->app->moniker). There is no need to overthink it.

@tianon
Copy link
Contributor

tianon commented Nov 8, 2020

Totally fair, and makes sense -- folks who've renamed their script ought to know what they're getting into already. 😄

I've pushed the updated version to master...tianon:dockerfile -- do you think it's ready for a PR, or is there more I ought to put into this first?

@kraih
Copy link
Member Author

kraih commented Nov 8, 2020

Sure, there will be style change requests, but it seems enough to get the review process started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants