这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@michaelshobbs
Copy link
Member

closes #515

@josegonzalez
Copy link
Member

Looks good to me. Merging.

josegonzalez added a commit that referenced this pull request Jan 15, 2015
add docker-args pluginhook call to build phase
@josegonzalez josegonzalez merged commit a0c5c45 into master Jan 15, 2015
@josegonzalez josegonzalez deleted the 515-docker-args-in-build-phase branch January 16, 2015 21:51
@vincentfretin
Copy link
Contributor

FYI, this change may be dangerous with the current implementation of the https://github.com/dyson/dokku-docker-options plugin.
I use docker-options plugin to mount a volume
dokku docker-options:add $APP "-v /home/dokku/$APP/.volumes/var:/app/var"
In the build phase, it simply deletes the /app/ directory, and so all the files of the volume. :)
I guess the docker-options needs to be adapted to use two different files for build et deploy phase or I simply use a different mount point than /app/something, it should absolutely be outside /app. I think I will simply write a warning in the README of the dokku-options plugin.

@josegonzalez
Copy link
Member

Correct, you'll need to handle that. I believe @michaelshobbs noticed this and may/may not have a fix.

@michaelshobbs
Copy link
Member Author

@vincentfretin see #906 and the subsequent linked PR to that plugin.

@josegonzalez do you think is this enough to revert this change?

@josegonzalez
Copy link
Member

I don't think so. Developers depending upon the docker-args should be checking the phase now before doing anything.

@dyson
Copy link
Contributor

dyson commented Jan 29, 2015

Hey guys, just to clarify, passing phase as an argument is the solution dokku is taking moving forward? I saw mention of perhaps doing doing docker-args-phase in #906.

I am looking to fix https://github.com/dyson/dokku-persistent-storage and https://github.com/dyson/dokku-docker-options this weekend. dyson/dokku-persistent-storage#12 looks good to me although docker-options will require a little more or a fix.

Also from a quick grep of the replo build, deploy and run are the only valid phases correct?

@josegonzalez
Copy link
Member

Those are currently the only valid phases. We may add more in the future, but those will be documented. Either way, as soon as we make a release, those two plugins will be broken on stable (they currently work for latest stable release but not master).

@michaelshobbs thoughts on moving this one forward? We can either move to having the phase in the pluginhook name - and break all old plugins - or continue with our method and say "screw it". I defer to you on this, though the former will require a minor release.

@michaelshobbs
Copy link
Member Author

What about calling both and deprecate in the next major? Or am I stupid? The latter is a valid response 😄

@josegonzalez
Copy link
Member

Fine with me, so long as plugin developers remember to move and not try to support both :P

@michaelshobbs
Copy link
Member Author

I'll do this and add it to the docs somewhere.

@dyson
Copy link
Contributor

dyson commented Jan 29, 2015

Yep I think that's a good idea. We have a reasonable list of plugins in the docs, so could easily reach out and let them know of the changes if need be. It could be a frustrating change for people otherwise.

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.

5 participants