Use Procfile for process types in Dockerfile apps #1915

pmclanahan · 2016-02-14T20:40:57Z

This is my initial attempt at fixing #1564. I'm opening a PR now to see how the tests in CI go and to get some feedback on direction. If I need to be doing something different I figured earlier was better than later to find that out.

Still todo:

Documentation of the feature
Tests for the feature

josegonzalez · 2016-02-15T00:06:12Z

Tests pass. Can you add docs for this so I can compare it to what we think it should do/our current handling of dockerfile deploys? This is obviously a breaking change, so I'm curious as to what the impact might be on our users.

pmclanahan · 2016-02-15T00:36:13Z

Will do. I'll try to get that done tonight.

pmclanahan · 2016-02-15T01:34:38Z

I could also make this feature off by default and enabled via an environment variable. So something like dokku config:set DOKKU_USE_PROCFILE=true would turn this on? Maybe that would allow for a more smooth roll-out?

josegonzalez · 2016-02-15T01:39:55Z

No, I think we should roll it out by default. We break functionality in 0.5.x, so no need to keep BC. Ideally if it works one way on heroku, it works that way on dokku.

What should happen if we define an entry point?

pmclanahan · 2016-02-15T01:45:18Z

The way it's written is that the entries in the Procfile would be run as arguments to the entrypoint script. I think as long as it's documented that providing a Procfile with a Dockerfile deployment means that the commands in the Procfile are used as the argument to docker run then it should make sense to people and they can use that to their advantage. I'm not sure how it would work otherwise. I could run an entrypoint experiment on heroku to check though.

mmerickel · 2016-02-15T02:15:06Z

We break functionality in 0.5.x, so no need to keep BC.

All I request is that so long as there's no Procfile at the root of the repo we keep things working as-is. :-)

pmclanahan · 2016-02-15T02:19:18Z

@mmerickel that is the idea for sure.

@josegonzalez my first pass at documentation is in.

pmclanahan · 2016-02-15T02:20:37Z

@mmerickel and since it actually won't have any effect unless it's in the docker image, you could add Procfile to .dockerignore and still have a Procfile in your repo if you so desired and it wouldn't affect the dokku deployment.

michaelshobbs · 2016-02-15T02:24:38Z

plugins/ps/pre-deploy


 APP="$1"

+get_procfile $APP


We don't need a herokuish app's Procfile. Can we only extract the Procfile in the necessary case?

I switched the generate_scale_file function to use the local Procfile extracted by this step, so it is used in either case. I figured it'd be the best thing since it was being extracted by that command anyway but thrown away.

Ah I missed that. Personally, I'd like to get away from cluttering the /home/dokku/APP directory. However, I believe that concern is being addressed in another issue (#1558)

That said, if you'd prefer I can revert generate_scale_file changes and only do this for dockerfile apps, but it would mean double extraction for dockerfile apps to generate the scale file and save the procfile for use by dokku deploy.

I'll defer to @josegonzalez

Can we delete the file if it's from a dockerfile deploy?

Not sure what you mean @josegonzalez. We need it in both cases for generating the scale file, and especially in dockerfile deploy for discovering what commands to run.

I meant in the /home/dokku/APP directory, and only extract it when necessary.

Could probably do that, or could go back to never storing it and reading it from the image every time. It just seemed like a useful pattern to keep info necessary for starting containers in app dir like the SCALE file is.

pmclanahan · 2016-02-15T02:43:00Z

I tried an experiment on Heroku with a Dockerfile with an ENTRYPOINT and I can't get it to successfully deploy at all. They, unlike dokku, impose some very strict rules on Dockerfiles for use with Heroku. They don't use Docker; their CLI tool extracts a slug from a local docker image and sends it to their API. So I guess they don't support ENTRYPOINT at all.

michaelshobbs · 2016-02-15T02:46:58Z

Yeah that's the conversation I'd like to have on #1564

pmclanahan · 2016-02-15T02:55:03Z

I personally feel it's a heroku bug rather than how they'd like it to work. They don't (can't) support the full docker feature-set, but dokku can do a better job. If we're going to ignore Procfiles for all but buildpack deployments then we need a better solution than the DYNO variable, but it could be as simple as a PROCESS_TYPE variable.

michaelshobbs · 2016-02-15T02:56:24Z

@pmclanahan would you mind bringing this discussion to #1564?

pmclanahan · 2016-02-15T03:05:30Z

Just did

michaelshobbs · 2016-02-15T03:05:47Z

Gracias!

pmclanahan · 2016-02-15T17:20:28Z

Just added DOKKU_PROC_TYPE variable as well. I can take or leave it, but added it in a separate commit as an easier alternative to using DYNO. Could go with just that or document how best to use DYNO. Could also just be DYNO_TYPE since the history file says that DYNO_TYPE_NUMBER should exist, though I can't find a reference to it in the code.

pmclanahan · 2016-02-15T17:23:46Z

Ah. NM. I see that was later removed and DYNO became what DYNO_TYPE_NUMBER was to be more heroku compatable.

josegonzalez · 2016-02-16T00:05:50Z

@pmclanahan Yeah, remove that please :)

pmclanahan · 2016-02-16T02:00:46Z

gone

michaelshobbs · 2016-02-16T20:48:11Z

dokku

+          if [[ -z "$START_CMD" ]]; then
+            # get cmd from Procfile
+            START_CMD=$(get_cmd_from_procfile $APP $PROC_TYPE)
+          fi


Maybe shorten this block to:

DOKKU_DOCKERFILE_START_CMD=$(dokku config:get $APP DOKKU_DOCKERFILE_START_CMD) DOKKU_PROCFILE_START_CMD=$(get_cmd_from_procfile $APP $PROC_TYPE) START_CMD=${DOKKU_DOCKERFILE_START_CMD:-$DOKKU_PROCFILE_START_CMD}

config_get, not dokku config:get.

Yeah that's the new standard. I don't think that code has been refactored yet

DOKKU_DOCKERFILE_START_CMD=$(config_get $APP DOKKU_DOCKERFILE_START_CMD) DOKKU_PROCFILE_START_CMD=$(get_cmd_from_procfile $APP $PROC_TYPE) START_CMD=${DOKKU_DOCKERFILE_START_CMD:-$DOKKU_PROCFILE_START_CMD}

You'll also need to add this line under the source command for common/functions

source "$PLUGIN_ENABLED_PATH/common/functions"

I like that. It's updated.

You'll also need to add this line under the source command for common/functions

You mean up around line 35? Does that not include the functions from the common plugin?

josegonzalez · 2016-02-17T22:20:11Z

docs/deployment/dockerfiles.md

+will be started basically using the command `docker run bin/run-prod.sh`. If you want to also run
+a worker container for this app, you can run `dokku ps:scale worker=1` and a new container will be
+started by running `docker run bin/run-worker.sh` (the actual `docker run` commands are a bit more
+complex, but this is the basic idea). So if you use an `ENTRYPOINT` in your `Dockerfile` the lines


Missing comma after Dockerfile

Remove the So part of this sentence.

pmclanahan · 2016-02-18T02:35:40Z

Rebased to fix merge conflict and updated docs based on feedback.

michaelshobbs · 2016-02-18T02:46:37Z

We keep the scale file because we can set it via the command line as well as ship it with the app. No need to keep the Procfile around really

pmclanahan · 2016-02-18T02:58:43Z

So maybe keep things as they are but remove the file in plugins/ps/post-deploy?

pmclanahan · 2016-02-18T03:14:48Z

plugins/ps/post-deploy

 APP="$1"
+DOKKU_PROCFILE="$DOKKU_ROOT/$APP/DOKKU_PROCFILE"
+
+[ -f "$DOKKU_PROCFILE" ] && rm -f "$DOKKU_PROCFILE"


@josegonzalez @michaelshobbs like so?

michaelshobbs · 2016-02-18T16:18:13Z

plugins/ps/post-deploy

 APP="$1"
+DOKKU_PROCFILE="$DOKKU_ROOT/$APP/DOKKU_PROCFILE"
+
+[[ -f "$DOKKU_PROCFILE" ]] && rm -f "$DOKKU_PROCFILE"


Is there a simple way to do this in pre-deploy?

The file is primarily needed in the deploy command.

Sorry I meant to say in the deploy phase. IIRC, you call get_cmd_from_procfile() in dokku's deploy phase. Thus, perhaps we can do everything in one swing as opposed to extracting the Procfile in pre-deploy, using it in deploy and removing it in post-deploy.

What do you think?

It's extracted in pre-deploy currently because generate_scale_file which is in pre-deploy now uses the extracted file. Thus it makes sense to do cleanup in post-deploy. We could of course move all of this into deploy in the dokku executable, but I don't see a down side of keeping it as it is.

And actually extract_procfile is probably a better function name than get_procfile. I can change it to be more explicit, and add a cleanup_procfile or remove_procfile function to make post-deploy cleaner.

Seems reasonable. Thanks for talking it out.

michaelshobbs · 2016-02-18T18:22:40Z

tests/unit/10_ps-dockerfile.bats

+}
+
+@test "(ps:scale) procfile commands extraction" {
+  deploy_app dockerfile-procfile


This test seems to be doing double duty given we already test the deployment of this app type in the previous test.

Instead, I recommend running create_app, generating a Procfile manually at $DOKKU_ROOT/$TEST_APP/DOKKU_PROCFILE and then running the get_cmd_from_procfile() tests. This way we don't waste the cycles testing something other than the specific thing we want to test. We follow this pattern in the Dockerfile port extraction tests. https://github.com/dokku/dokku/blob/v0.4.13/tests/unit/30_core_ports_2.bats#L100-L124

good catch. updated and pushed to my branch.

pmclanahan · 2016-02-18T21:24:23Z

For some reason github isn't indicating the passing test run. But it did pass. So hopefully we're nearly done here?

* Document new Procfile feature * Add and update tests * Fix a typo in generate_scale_file (ref dokku#1923)

michaelshobbs · 2016-02-18T23:36:27Z

lgtm

@josegonzalez

josegonzalez · 2016-02-18T23:43:15Z

Ship it at your leisure, @michaelshobbs.

…ile-deploy Use Procfile for process types in Dockerfile apps

josegonzalez · 2016-02-19T01:35:57Z

Note: I backed out this change while I attempt to make a release of dokku for 0.4.x. This change will ship with 0.5.x.

josegonzalez · 2016-02-19T02:23:36Z

Alright, it's now in master :)

pmclanahan · 2016-02-19T02:27:23Z

W00t! 🍰 Thanks guys! Pleasure working with ya.

michaelshobbs · 2016-02-19T02:42:36Z

No no, thank you @pmclanahan 👊

vladfaust · 2023-07-11T17:27:31Z

If I specify ENTRYPOINT [] or ENTRYPOINT [""] in the Dockerfile, and web: npm run web in the Procfile, the build fails with No command specified. However, empty entrypoint is indeed legal and works locally (docker run container npm run web).

vladfaust · 2023-07-11T18:05:14Z

Also, it doesn't seem to run the release process if CMD is set instead of ENTRYPOINT (which is a work around the empty entrypoint bug).

josegonzalez · 2023-07-11T19:00:54Z

@vladfaust pleas file a new issue rather than commenting on an 8 year old closed one. Thank you.

michaelshobbs reviewed Feb 15, 2016
View reviewed changes

michaelshobbs mentioned this pull request Feb 15, 2016

Support Procfile commands with a Dockerfile-based deployment #1564

Closed

josegonzalez added the type: enhancement label Feb 16, 2016

pmclanahan force-pushed the support-procfile-for-dockerfile-deploy branch from 36d2b72 to 0eb34ef Compare February 16, 2016 01:59

michaelshobbs reviewed Feb 16, 2016
View reviewed changes

pmclanahan force-pushed the support-procfile-for-dockerfile-deploy branch 6 times, most recently from f0cb5e5 to cafad6e Compare February 17, 2016 16:45

josegonzalez reviewed Feb 17, 2016
View reviewed changes

pmclanahan force-pushed the support-procfile-for-dockerfile-deploy branch from 4c8b97f to 72e6899 Compare February 18, 2016 02:35

pmclanahan reviewed Feb 18, 2016
View reviewed changes

pmclanahan force-pushed the support-procfile-for-dockerfile-deploy branch from 63c77bb to 9d3c893 Compare February 18, 2016 03:15

michaelshobbs reviewed Feb 18, 2016
View reviewed changes

pmclanahan force-pushed the support-procfile-for-dockerfile-deploy branch 3 times, most recently from d896d79 to 1252a20 Compare February 18, 2016 18:15

michaelshobbs reviewed Feb 18, 2016
View reviewed changes

pmclanahan force-pushed the support-procfile-for-dockerfile-deploy branch from 1252a20 to 4d4e41b Compare February 18, 2016 18:33

Fix dokku#1564: Use Procfile for process types in Dockerfile apps

162a50a

* Document new Procfile feature * Add and update tests * Fix a typo in generate_scale_file (ref dokku#1923)

pmclanahan force-pushed the support-procfile-for-dockerfile-deploy branch from 4d4e41b to 162a50a Compare February 18, 2016 22:03

michaelshobbs added a commit that referenced this pull request Feb 18, 2016

Merge pull request #1915 from pmclanahan/support-procfile-for-dockerf…

4812b74

…ile-deploy Use Procfile for process types in Dockerfile apps

michaelshobbs merged commit 4812b74 into dokku:master Feb 18, 2016

pmclanahan deleted the support-procfile-for-dockerfile-deploy branch February 19, 2016 02:27

dokku locked and limited conversation to collaborators Jul 11, 2023

Uh oh!

Use Procfile for process types in Dockerfile apps #1915

Use Procfile for process types in Dockerfile apps #1915

Uh oh!

Conversation

pmclanahan commented Feb 14, 2016

Uh oh!

josegonzalez commented Feb 15, 2016

Uh oh!

pmclanahan commented Feb 15, 2016 via email

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

josegonzalez commented Feb 15, 2016

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

mmerickel commented Feb 15, 2016

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

michaelshobbs commented Feb 15, 2016

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

michaelshobbs commented Feb 15, 2016

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

michaelshobbs commented Feb 15, 2016

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

pmclanahan commented Feb 15, 2016

Uh oh!

josegonzalez commented Feb 16, 2016

Uh oh!

pmclanahan commented Feb 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmclanahan commented Feb 18, 2016

Uh oh!

michaelshobbs commented Feb 18, 2016

vladfaust commented Jul 11, 2023 •

edited

Loading