Support scripts.dokku. in app.json #1836

michaelshobbs · 2016-01-07T02:19:27Z

supports scripts.dokku.predeploy and scripts.dokku.postdeploy in (/app/app.json in the app image)
adds a couple json parsing utility functions
adds IMAGE_TAG post-deploy plugn trigger call
and oh yeah, adds a test and docs :P

josegonzalez · 2016-01-07T02:26:53Z

dokku

This should really be cleaned up imo. port and then ip? ew.

The pre-deploy hook is:

plugn trigger pre-deploy $APP $IMAGE_TAG

Maybe we should follow that and instead do:

plugn trigger post-deploy $APP $IMAGE_TAG $ipaddr $port

Or similar? What uses the port/ipaddr from here that cannot retrieve it from some other helper?

Oh I see. This would break any post-deploy plugins that used those arguments. While I agree with you that this should probably happen at some point, I don't think it should go in this PR.

I meant post-deploy in my second example.

Yeah I meant that too :)

josegonzalez · 2016-01-07T02:28:27Z

What happens for Dockerfile users that don't have bash in their containers?

michaelshobbs · 2016-01-07T02:37:28Z

Yeah the current implementation only supports buildpack apps anyway since we don't nullglob on sourcing files from /app/.profile.d/*. Not sure dockerfile support makes sense since app.json is a Herokuism anyway. Thoughts?

EDIT: if this is to only be supported in buildpack apps (which i'm leaning that way) then I'll need to drop the dockerfile reference from the documentation.

michaelshobbs · 2016-01-07T03:25:52Z

Tests are failing because CircleCi did not include their docker cp patch in the 1.9.0-circleci daemon binary we're using now. We can downgrade back to 1.8.2-circleci but I'd like to see if they will patch the newer version first. I've opened a support ticket that asks for this.

josegonzalez · 2016-01-07T04:28:46Z

Yeah lets drop the dockerfile reference.

michaelshobbs · 2016-01-07T04:35:10Z

Noooooo! 😜 This really doesn't need to wait for 0.5.0.

josegonzalez · 2016-01-07T04:55:23Z

I'd like to leave features up for 0.5.0 to push people to upgrade :)

michaelshobbs · 2016-01-07T04:55:47Z

Bah! Ok 😿

josegonzalez · 2016-01-07T05:15:58Z

Is there a good reason why we shouldn't incentivize updates?

michaelshobbs · 2016-01-07T05:18:07Z

Meh?

josegonzalez · 2016-01-07T05:32:22Z

Note, I don't mind doing it in 0.4.x if that's your wish.

michaelshobbs · 2016-01-07T05:39:18Z

no. 0.5.0 works for me

michaelshobbs · 2016-01-08T02:04:13Z

I've split out the commits that don't belong here into other PRs. Once those tests pass, I'll update this one.

michaelshobbs · 2016-01-08T15:49:35Z

I'm wondering if postdeploy makes any sense. The way dokku deploys is illustrated below.

# copy app to container
docker run
docker wait
docker commit dokku/$APP

# trigger pre-build
# build
docker run
docker wait
docker commit dokku/$APP
# tigger post-build

# trigger pre-release
# release
docker run ...
docker wait
docker commit dokku/$APP
# trigger post-release

# trigger pre-deploy (run app.json: scripts.dokku.predeploy)
# deploy
docker run /start web (this is the deployed app container)
docker wait
docker commit dokku/$APP
# trigger check-deploy
# trigger post-deploy (run app.json: scripts.dokku.postdeploy)

If an app.json postdeploy script makes changes to the app image, the app will never see those changes because our container is already running with the previous image layer.

As far as I can tell, we have 2 options.

drop scripts.dokku.postdeploy support
don't commit the scripts.dokku.postdeploy container
- committing this step ends up creating an image layer that is never used by the app and drops the image name from docker ps output

CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
2fcdcdb6d725        f84f300e0aed        "/start web"        25 minutes ago      Up 25 minutes                           node-js-express.web.1

I'm leaning towards option 2. Thoughts?

josegonzalez · 2016-01-08T17:54:55Z

Option 2

michaelshobbs · 2016-01-08T18:42:00Z

Assuming tests pass, I think this is good to go.

josegonzalez · 2016-01-17T01:09:28Z

This looks good to me for the 0.5.0 release.

jvanbaarsen · 2016-01-19T16:28:19Z

🎉 Awesome! @michaelshobbs Thanks for working on this ❤️

josegonzalez · 2016-02-19T07:12:54Z

Don't know why help generation is failing very weird, as master passes...

michaelshobbs · 2016-02-19T22:32:48Z

Must have been something weird with your rebase. I rebased and forced-push. Tests pass now

josegonzalez · 2016-02-20T07:11:03Z

Merge when ready :)

support scripts.dokku. in app.json

josegonzalez reviewed Jan 7, 2016
View reviewed changes

michaelshobbs mentioned this pull request Jan 7, 2016

Add back some deploy tests that test dokku functionality #1837

Merged

josegonzalez added the 0.5.0 label Jan 7, 2016

michaelshobbs force-pushed the 1824_mh-app-json branch from c7a201e to 3596e42 Compare January 7, 2016 05:28

michaelshobbs added the type: enhancement label Jan 7, 2016

michaelshobbs force-pushed the 1824_mh-app-json branch 8 times, most recently from 29182d9 to bfa26fe Compare January 8, 2016 01:53

michaelshobbs self-assigned this Jan 8, 2016

michaelshobbs added the don't-merge label Jan 8, 2016

michaelshobbs force-pushed the 1824_mh-app-json branch from bfa26fe to 75e2272 Compare January 8, 2016 04:20

michaelshobbs force-pushed the 1824_mh-app-json branch from 75e2272 to 2a7e2f4 Compare January 8, 2016 18:41

michaelshobbs removed the don't-merge label Jan 8, 2016

michaelshobbs modified the milestone: 0.5.0 Jan 13, 2016

michaelshobbs removed the 0.5.0 label Jan 13, 2016

michaelshobbs mentioned this pull request Jan 29, 2016

Change to use DOKKU_SCALE file instead of SCALE michaelshobbs/dokku-app-predeploy-tasks#20

Closed

josegonzalez force-pushed the 1824_mh-app-json branch from 2a7e2f4 to 91a8a54 Compare February 19, 2016 04:02

support scripts.dokku in app.json. closes #1824

48f2dc0

michaelshobbs force-pushed the 1824_mh-app-json branch from 91a8a54 to 48f2dc0 Compare February 19, 2016 21:30

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

Merge pull request #1836 from dokku/1824_mh-app-json

a9de299

support scripts.dokku. in app.json

michaelshobbs merged commit a9de299 into master Feb 20, 2016

josegonzalez deleted the 1824_mh-app-json branch February 21, 2016 01:09

josegonzalez changed the title ~~support scripts.dokku. in app.json~~ Support scripts.dokku. in app.json Mar 26, 2016

Uh oh!

Support scripts.dokku. in app.json #1836

Support scripts.dokku. in app.json #1836

Uh oh!

Conversation

michaelshobbs commented Jan 7, 2016

Uh oh!

josegonzalez Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

michaelshobbs Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

josegonzalez Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

michaelshobbs Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

josegonzalez Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

michaelshobbs Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

josegonzalez commented Jan 7, 2016

Uh oh!

michaelshobbs commented Jan 7, 2016

Uh oh!

michaelshobbs commented Jan 7, 2016

Uh oh!

josegonzalez commented Jan 7, 2016

Uh oh!

michaelshobbs commented Jan 7, 2016

Uh oh!

josegonzalez commented Jan 7, 2016

Uh oh!

michaelshobbs commented Jan 7, 2016

Uh oh!

josegonzalez commented Jan 7, 2016

Uh oh!

michaelshobbs commented Jan 7, 2016

Uh oh!

josegonzalez commented Jan 7, 2016

Uh oh!

michaelshobbs commented Jan 7, 2016

Uh oh!

michaelshobbs commented Jan 8, 2016

Uh oh!

michaelshobbs commented Jan 8, 2016

Uh oh!

josegonzalez commented Jan 8, 2016

Uh oh!

michaelshobbs commented Jan 8, 2016

Uh oh!

josegonzalez commented Jan 17, 2016

Uh oh!

jvanbaarsen commented Jan 19, 2016

Uh oh!

josegonzalez commented Feb 19, 2016

Uh oh!

michaelshobbs commented Feb 19, 2016

Uh oh!

josegonzalez commented Feb 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants