Skip to content
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

Plugin structure reorganization. #312

Closed
wants to merge 17 commits into from
Closed

Plugin structure reorganization. #312

wants to merge 17 commits into from

Conversation

plietar
Copy link
Contributor

@plietar plietar commented Nov 16, 2013

Was "Fix pluginhook problems by spliting commands into seperate hooks."
Fixes #236 and #272.

This PR used to be only meant to fix problems with pluginhook.
In the end, It is a quite deep change in how plugins are organized.

All commands implemented by plugins are now prefixed by the name of the plugin.
00_standard-dokku was renamed to have a simpler name, core.
So for example the existing url command is now called core:url.

Because it added more trouble with pty issues than it solved, pluginhook is not used anymore for commands, onyl for hooks.
Instead, each command is implemented in a seperate file, and is called directly, w/o using pluginhook.

The commands map as following :

  • "Root commands", in the form <plugin> map to $PLUGIN_PATH/<plugin>/command
  • Sub-commands, in the form <plugin>:<name> map to $PLUGIN_PATH/<plugin>/commands/<name

In order to avoid the need to always type in the full command name, command aliases have been implemented.
These are also required to support commands with fixed names, such as git-* commands.
Aliases also allow to override default commands, by creating or modifying an alias. Fixes #296

Aliases are defined in the $DOKKU_ROOT/ALIAS file in the simple

<name>=<value>

A default set of aliases is included with dokku, and installed by make install. An addons plugin provides commands to manage them.

Additionally, when started with no parameters, dokku runs the _ command. This can be aliased to any command.
See below about the shell command which was added as the default _ alias.

The help command was also modified.
Instead of having each plugin define a help hook or command, every user-facing command's script includes the help message in it's header. See existing commands for the format.
The core:help command (aliased to help) parses this format, looks for a short alias in the ALIAS file, and pretty prints the message.

I also added a dokku shell command. Initially it was just a test for interactive commands, but I find it quite nice, so I've included it with two other tiny commands ls and docker which make working in the shell easier.
The shell should also make it easier to run tests with expect (Although I must admit I having trouble with it)
Basically the shell offers a command prompt, one can type in commands, and they will be run as dokku <command>.

It supports command history, and that's about it. I tried adding basic completion, but there doesn't seem to be an easy way which doesn't involve re-writing the whole readline in bash. Maybe this can be done in a improved shell plugin, written in eg python ruby, overriding the default one.

By default, the _ is aliased to shell, so plain dokku, or ssh dokku@dokku.me starts the shell.

@plietar
Copy link
Contributor Author

plietar commented Nov 17, 2013

Just had a talk with @asm89 on IRC about naming the scripts.
Here's what we've come up to :
All commands are prefixed with namespace, which is the plugin's name.
Commands would be called for example core:run, config:set, ...

The <plugin>:<name> commands maps to the <plugin>/commands/<name> script.
The <plugin> maps to the <plugin>/command script
Eg :

  • core:run => core/commands/run
  • config:set => config/commands/set
  • config => core/command

To avoid the need to prefix core commands, we add some aliases. so run would be an alias to core:run.
Also this allows to override default commands by changing the alias.
Eg change deploy to point to ps:deploy instead of core:deploy.

Aliases are defined in a config file and can be modified by the user.
We however ship a default set of aliases with dokku.

The only problem remaining is how to display the help message.
Should it display the original or the shorter aliased one ? What if multiple aliases point to the same command ?
An easy solution would display the full command, and then print the list of aliases.

@progrium
Copy link
Contributor

Once this is solid, let's plan to include it in 0.3

On Sun, Nov 17, 2013 at 8:33 AM, Paul Lietar notifications@github.comwrote:

Just had a talk with @asm89 https://github.com/asm89 on IRC about
naming the scripts.
Here's what we've come up to :
All commands are prefixed with namespace, which is the plugin's name.
Commands would be called for example core:run, config:set, ...

The : commands maps to the /commands/ script.
The maps to the /command script
Eg :

  • core:run => core/commands/run
  • config:set => config/commands/set
  • config => core/command

To avoid the need to prefix core commands, we add some aliases. so runwould be an alias to
core:run.

Aliases are defined in a config file and can be modified by the user.
We however ship a default set of aliases with dokku.

The only problem remaining is how to display the help message.
Should it display the original or the shorter aliased one ? What if
multiple aliases point to the same command ?
An easy solution would display the full command, and then print the list
of aliases.


Reply to this email directly or view it on GitHubhttps://github.com//pull/312#issuecomment-28649473
.

Jeff Lindsay
http://progrium.com

mikexstudios added a commit to mikexstudios/dokku that referenced this pull request Nov 19, 2013
This is a temporary fix implemented by piping echo into the ssh command
(see plietar's comment on dokku#272) until dokku#312 can be committed.
mikexstudios added a commit to mikexstudios/dokku that referenced this pull request Nov 19, 2013
This is a temporary fix implemented by piping echo into the ssh command
(see plietar's comment on dokku#272) until dokku#312 can be committed.
@plietar
Copy link
Contributor Author

plietar commented Nov 20, 2013

It should be ready now, if someone wants to give it a try.
I wanted to add tests for the addons (and possibly other) plugin, but I'm strugling with expect right now.

This prevents commands from hanging when called from ssh.
Also use `pluginhook -s` to run commands.
Also fix a problem with detaching from the container.
A `docker command which (obviously) runs docker, and an `ls`command
which lists the deployed applications.
Don't use pluginhook to run commands.
Renamed 00_dokku-standard into core.
Every user-facing command should contain a special help message after its
shebang.
The help command parses it, and display the correct command, based on the
ALIAS file.
@stuartpb
Copy link

stuartpb commented Mar 9, 2014

I like the idea of making commands individual scripts and taking the need for command-hacking out of the plugin stack, but I'm not wild about the idea of an ALIAS file.

The whole point of pluginhook as I understand it is to reduce the hacking of individual lines in a special file, and just let the plugin tree dictate the plugins' behavior. If we're using text in a file to select which base command corresponds to which file, why not just go back to using a bash script for command handling?

@stuartpb
Copy link

stuartpb commented Mar 9, 2014

Indeed, there's still a need for command-stack handling for plugins that control other plugins (for instance, a hypothetical admin plugin that whitelists only certain forms of certain commands for general use). This could theoretically be handled by having the install script handle a bunch of sed runs over ALIAS, but, again, that brings us back to the error-prone world of file-content hackery.

@stuartpb
Copy link

stuartpb commented Mar 9, 2014

What I'd like to see is this command structure (where commands are implemented as a sort of hook) implemented for plugins as a plugin itself, kind of like how the backup plugin instantiates the backup hooks for any other plugins. That way, any plugins implemented for the old system don't break, any plugins that want to use the new system can do so, and any plugins that need to mess with the commands hook can also do so.

Indeed, I'm going to go build this into my proposed PR at #458 now.

@stuartpb
Copy link

stuartpb commented Mar 9, 2014

Another thing I'm not wild about is the way that this makes plugin names not decoupled from their use, so changing a plugin's name (for instance, because you want to change its order, or it conflicts with another plugin you're working with) will alter the names of its commands.

On the other hand, it prevents cases where two plugins try to implement the same command, so...

@progrium
Copy link
Contributor

Yeah those are all interesting points. I think the ALIAS file is for user/admin convenience and plugins shouldn't rely on using it. As for plugin name coupling ... I think for simplicity's sake, it's fine. If an admin has to deal with conflicting plugins by renaming, they can also handle using different commands... or perhaps making edits to ALIAS to make them work together with the same command name. However, I would like to see ordering addressed. Maybe optional numeric prefixes to dir names that are otherwise ignored.

@stuartpb
Copy link

My proposed PR allows the (few) plugins that need ordering for their hooks to use a number-prefixed name and the classic "commands" hook for handling their commands.

@michaelshobbs
Copy link
Member

@josegonzalez not sure this is necessary anymore. we've restructured the namespace through other PRs. i'm not a fan of supporting the alias bit. too much complexity for the slight benefit. The dokku shell seems interesting. I'm down to pull that in if you agree.

@josegonzalez
Copy link
Member

Pulling in the shell is fine by me.

josegonzalez added a commit that referenced this pull request Feb 8, 2015
implement dokku shell and ls (by @plietar). refs #312
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow dokku run to support interactive commands
6 participants