-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Avoid calling dokku binary #2242
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
Conversation
| source "$PLUGIN_AVAILABLE_PATH/config/functions" | ||
| source "$PLUGIN_AVAILABLE_PATH/nginx-vhosts/functions" | ||
| source "$PLUGIN_CORE_AVAILABLE_PATH/proxy/functions" | ||
| source "$PLUGIN_AVAILABLE_PATH/proxy/functions" |
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.
Should a user be able to disable the proxy meta plugin?
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.
In theory they should be able to use whatever plugin implementation instead of this proxy one. Only the common and standard plugins are not overrideable.
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.
This plugin isn't a proxy implementation. It's the interface plugin to call the configured proxy plugin.
fd9e84f to
484fd7a
Compare
|
I have a bit of work to do here to make sure I call everything properly, so don't merge this yet (even if tests start passing). |
|
If you're going to move some |
|
The git/tar commands need to be sourceable because things outside of those plugins need to execute them. |
|
Example? |
plugins/git/receive-app
Outdated
| else | ||
| # shellcheck disable=SC2086 | ||
| dokku git-build $APP $REV | ||
| git_build_cmd $cmd $APP $REV |
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.
This needs to execute the function.
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.
Is there an analog in the tar plugin?
|
I'd love to see this is fixed :) |
|
I have some work to do but this will go out 0.6.x. |
6f6616a to
569f578
Compare
683d6cc to
a2f95c2
Compare
a2f95c2 to
fcefd4a
Compare
|
Updated initial comment with implementation details. I think this is g2g assuming tests pass. I'll keep an eye on the test run. |
docs/development/plugin-triggers.md
Outdated
|
|
||
| - Description: | ||
| - Invoked by: `dokku build` | ||
| - Invoked by: `dokku_build` |
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 should probably say these are functions within dokku if anything. I would otherwise leave them as is.
Problem
Whenever we call a dokku subshell, the
DOKKU_APP_NAMEarg persists. As the official remote client uses--appfunctionality, the following will break a bit:Why does it break? The above call is translated to the following:
Which then will do weird stuff if you call a subshell (as we do for docker-options and config set) within the couchdb plugin:
Solution
Rather than sometimes calling the dokku binary and sometimes not, I figure we should move to calling the sourced functions directly so that we standardize on one way of doing things.
The alternative would be to always set
DOKKU_APP_NAMEto an empty string when calling a dokku sub-shell, which we'll totally forget to do.Implementation
dokku builddokku receivedokku releasedokku tar:builddokku tar:build-lockeddokku git:builddokku git:build-lockedreceive-apptrigger in given receiving pluginCloses #2166