I did not look at the tests at all since I don’t know where we’re going exactly 😁

Looks like a good start to my dokku-n00b eyes :)

And not that you've gotten there yet, but just so it's mentioned here, the upgrade process would obviously need to delete existing core plugins in the old plugins path, and probably move any non-core plugins into the new custom path.

@michaelshobbs how much of this would be affected by the plugn pr?

There is significant overlap. One of you guys should really take a look at plugn and that branch before going too far down this path. I'd hate for you to waste your time.

@jimeh 👍

@michaelshobbs I looked at your PR and I don’t think there’s a real problem here. I’m pretty sure (but I didn’t check) that we could replicate what I do here with plugn instead of pluginhook. Plugn seems to take an env variable for its plugins directory so the mechanism is pretty similar to the one used here.

Also the goal of this PR isn’t the same than yours. Here we try to give a solution (that seems pretty clean to me) to the problem in issue #1205. Another important point I think is that plugin compatibility is not affected at all by this PR.

Just found a bug: source "$(dirname "$0")/../common/functions" won’t work with symlinks 😕
Anyone knows why source "$PLUGIN_PATH/common/functions" isn’t used instead?

Really? Shouldn't source follow symlinks? Or does dirname $0 return the real path rather than the symlinked one?

well it won’t work with ls too. The thing that works is when you move around with cd because your shell keeps an history from where you’ve “entered” the symlinked directory. And since here there is no context, .. refers to the “real” parent directory.

So we’ll need to use $PLUGIN_PATH instead in all plugins (but compatibility will be broken 😕) or we’ll have to drop symlinks and copy the plugins between directories. Other ideas?

Ah, yeah you're right. The .. does indeed screw it up. Sorry, seemed weird while I was guessing off the top of my head.

Seems symlinks for custom plugins isn't going to work then. But I just did some basic testing and seems custom plugins will work if they simply live in the folder relative to the common symlink, rather than being symlinked in themselves.

I tested a folder structure like this locally with some mock plugins:

/var
 └── lib
     └── dokku
         ├── core-plugins
         |   ├── common
         |   ├── checks
         |   └── domains
         ├── disabled-plugins
         |   └── mongodb
         └── plugins
             ├── common -> /var/lib/dokku/core-plugins/common
             ├── checks -> /var/lib/dokku/core-plugins/checks
             ├── domains -> /var/lib/dokku/core-plugins/domains
             ├── forego
             └── redis

It's mostly just the enable and disable commands that need to simply move the custom plugin folder back and forth, while refusing to operate on symlinks. It might not be the absolutely cleanest solution, but probably the most decent one to avoid breaking backwards compatibility.

And on a side-note, the example I have above would mean even less backwards compatibility issues, as even installation instructions in existing plugins would still be correct when they instruct you to copy/clone/extract the plugin to /var/lib/dokku/plugins.

As for why dirname "$0" is used, seems silly, but I guess someone started the convention and others followed... lol

Just convention. If you guys want to make a separate pr that uses $PLUGIN_PATH, that would work.

@jimeh yes I think your solution will work and if we want to keep plugin compatibility we don’t really have a choice.

@josegonzalez we could then we’ll have to update (almost) all community plugins too 😁 I’m not sure it’s worth the effort.

I think it's worth to change core plugins to use $PLUGIN_PATH in an effort to start setting a new and better convention :)... I'd be happy to submit such a PR.

Ok since you seem to be motivated, I’ll wait for you PR. Meanwhile, I’ll patch my plugins and forks 😉

PR is in, but I don't think we can rely on or assume all 3rd party plugins will be updated any time soon. So plugin management is still probably best done via moving them back and forth. But at least new plugins will hopefully follow the new convention and make life easier for everyone in the future.

Yes but it’s sad.

We can update and submit PR to a lot of them. There’s also a lot of plugins which aren’t active/maintained anymore 😕

Since we’ll need an upgrade script with the new plugins management, we could check for source $(dirname $0)/../common/functions in installed ones and print a warning (and not enable them).

or better yet, replace automatically the line with the new one but still print a warning message to update plugin (or report the error to the author).

That's one solution, although when/how would that check be executed? I'm assuming as part of plugins:enable right?

And come to think of it, the enable/disable thing is nice, but if we use the move solution the benefit is that there's ZERO difference in how things work now. Plugins are enabled by default, they don't run as part of dokku installation, and you can disable plugins via a simple command instead of manually deleting or moving the directory on the server.

The symlink solution feels nicer to me from a architectural point of view, but does alter how the end user installs plugins. An extra step isn't much, but it's still different.

Don't know which one you prefer @Flink, nor am I fully sure which one I prefer, just wanted to point things out for clarity's sake :)

P.S. As a nod towards moving plugins around, OSX actually handles user-installed fonts that way. User fonts live in ~/Library/Fonts, and if you disable one, it's moved to ~/Library/Fonts (Disabled).

Well I think I’m like you on this one, the symlink solution seems “better” but what you say is also very true. Moving plugins instead of symlinking them and changing the directory structure will be better from a user point of view.

Ok I think this PR is done now and should work as expected. I used the approach suggested by @jimeh with directories and mv instead of symlinks. So we don’t need an upgrade script or something like that, it will work out of the box with an existing installation. You just have to upgrade dokku normally 😄

/cc @josegonzalez @michaelshobbs

If we're good with this I'd like to merge it into the plugn branch and merge them both into master via that PR; unless we want this in before 0.4.0. Though I didn't think we were releasing another 0.3.x.

Well it fixes the issue with debian installation so it can be merged in a 0.3.x branch. But I personally don’t have a preference :)

I don't mind either :)... Personally I worked around the installation issue by having the chef cookbook disable/enable plugins itself before installation/upgrade, so I don't mind waiting till 0.4.x for a fix.

We are not doing another 0.3.x release.

Ok so it settled ;) @michaelshobbs Why not merge this in master and then rebase your PR?

We're not rebasing plugn anymore.

@Flink 6 of one have a dozen of the other. Hence me saying I'd like to do it this way. Just a preference.

Ok I was just asking 😅 If it’s easier for you like that then there’s no problem (I don’t really mind 😁)

Yeah no worries at all. It's probably all the same but my thinking was just that if we're going to overhaul plugins then let's just do it in one go.

Okay I'm going to close this PR. Please redo the PR and target the mh-switch-to-plugn branch. We can merge it into there and then see how we like/dislike the experience :)