-
-
Notifications
You must be signed in to change notification settings - Fork 2k
dockerfile support #954
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
dockerfile support #954
Conversation
contrib/dokku_client.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do a separate PR with these changes first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can. it was on my mind at the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sokay as is for now.
|
I'm +1 on the changes, -1 on the fact that it's a big PR. We should be moving towards smaller, more reviewable PRs going forward. Assuming tests pass, I looked it over and can't think of anything glaring at the moment. I'll take a look again in a bit. |
|
That's reasonable. Let me split out the style stuff into another PR. |
plugins/00_dokku-standard/commands
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation happened in 0.3.14, and it will be removed in 0.4.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
541ec1f to
5684754
Compare
5684754 to
179a5e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think this should probably be refactored out of commands directly in the plugin. I don't see the need now to be able to run git-build on it's own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh except, derp!! We moved it to commands so the ps:rebuild, et al. would work. So nevermind, carry on, nothing to see here.
|
I think we should let this simmer for a bit and see if it gets some more feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the /bin/bash part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superfluous, as far as I can tell.
|
Notice to the following authors: @ohardy, @hughfletcher, @ohardy, @luxifer, @ohardy, @jeffutter, @stuartpb, @apmorton, @statianzo, @statianzo, @sehrope, @iskandar, @mikecsh, @nornagon, @pauldub, @cedricziel, @cedricziel, @cameron-martin, @ohardy, @pnegahdar, @ademuk, @alexbeletsky, @thrashr888, @gdi2290, @robv, @davidcollom, @baobaab, @Cactusbone, @jazzzz, @Volune, @mlebkowski, @darkpixel |
|
gg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn’t this pre-hook be in fact before the build?
|
@michaelshobbs thx for the mass ping ;) I havent been into dokku for a while now. I would like to continue to maintain my plugins.. So: Is it documented somewhere how exactly too hook in now? Read: Is there a replacement for the mentioned hooks? |
|
@michaelshobbs, thanks for the ping, fixed in dokku-community/dokku-acl@a81aa53. But you should probably mention that those pluginhooks are just being renamed to |
|
Yeah, we have pluginhook docs coming out pretty soon, which hopefully will have examples for all of them. |
|
@mlebkowski 👍 to reiterate, |
… when using heroku buildpack See dokku/dokku#954
This is a fairly large change to core functionality.
buildandreleaseinto00_dokku-standardplugin(pre/post)-build-dockerfileand(pre/post)-release-dockerfilepluginhooksconfigplugin for dockerfile apps withdocker-args-deploypluginhookreceive-apppluginhook in an attempt to be git agnosticPlease pick this apart with extra vigor.