I like it! I think it's looking good. My thoughts:

The call to addPlugin() doesn't yet make the plugin visible to Go, right? By default, Go does not check for new plugins in that directory, till a server restart. How it works is, during startup, these things happen, in order:

1. Start plugin infrastructure.
2. That starts a monitor thread which monitors the plugins' location.
3. On change (meaning a file is added, removed or updated), it notifies listeners of the change. The DefaultPluginJarChangeListener gets notified and it does a bunch of checks and then adds a plugin.

In case you haven't seen it, some plugin architecture documentation is here.

To see whether the upload works, you can also set the system property (pluginLocationMonitor.sleepTimeInSecs) to something other than -1. It affects the timer used here, so that Go actually checks every few seconds to see if there are new plugins on the filesystem.

Maybe in a later iteration of this, it'll be useful to surface up errors found during adding of the plugin, to the UI? I think that'll be nice (along with preventing the plugin from being written to the file system). It'll take a little more work around the infrastructure though. I don't want to introduce scope creep into your chosen feature. :)

I think it's looking good! Keep going. Let me know how I can help.

@arvindsv Yup, it's work in progress, and how I tested it was indeed to set the pluginLocationMonitor.sleepTimeInSecs variable to > 0. That way when I refreshed the browser, the monitor thread kicks in, and the plugin shows.

My next steps are -

1. Instead of returning true or false, I want to return a response that has both success and error messages. And of course this should percolate to the UI.
2. I want to add some sort of validation on the file (e.g. check it's a jar file, etc). Right now the monitor does the validation whenever it detects updated/added/removed plugins, but I want to make sure on the input side we're doing validation as well.
3. I want to kick off the restart mechanism after a plugin is successfully uploaded, so people don't have to restart the server on the box themselves. It seems like there're multiple ways of achieving - I could, for example, start a new monitor thread whenever there's an uploaded plugin. Any ideas? On a related note, I was wondering why we designed the Monitor & its internal Thread the way they are. It seems to me that Thread knows too much (logic, environment variables, etc). Do you think there's a way we can improve this?

@arvindsv worked on file type validation today with @pwen. Still thinking about the restart mechanism. Any thoughts on improving the current way the monitor handles threading?

@pamo and @pwen, sorry was away on Saturday and a little busy today. Will respond tomorrow. At a very high level, I feel that it can (needs to) be changed a bit as well. I'll think about it a bit more and respond tomorrow.

@arvindsv Don't worry about it, not expecting you to work on weekends. :-p

We should definitely have a conversation about the monitor thread and the restart mechanism in general. But I think what we have done so far - creating a form to let the user upload the jar directly from the UI - is already an improvement from the current workflow (aka access the box and drop the jar file there manually). Already some value added, what do y'all think? Yeah, they still have to restart the server, and yea, and we should continue to work on this, maybe in another pull request?

Yea I agree. Restarting the machine should be broken out into another PR. @danielsiwiec suggested using file change notifications as the mechanism to restart the machine. https://docs.oracle.com/javase/tutorial/essential/io/notification.html

I suppose my ideal approach would be to remove the file system monitor, completely. Upload should be possible through the UI and through a well-documented and easy API. Watching the file system for changes should not even be needed (it should be read once, at startup, obviously).

About your (@pwen's) thoughts about changing parts of the monitor and thread: Yes, sure. There's room for improvement there, and reducing the amount of knowledge of each of those classes. It, kind of, gets hard to name classes well. But, if you have ideas for change, propose them, and we can talk about it.

I think I'd start tackling it at around here. I think there should be one way to add a plugin to the system. This new way should provide facilities such as validateBeforeAdding() and add(), and should probably work with a stream (contents of JAR), so that it would help send messages back to the user, about failed uploads, etc. Validation should happen before adding the file to the filesystem.

If we're keeping the file system monitor, in whatever form, I think it should be something like this:

Monitor -------x
               |
               v
                --------> One way to add plugins, with validation
               ^
               |
UI ------------x

@pamo, I think there is value in having the ability to upload a plugin (without restarting, for now). I haven't taken a look at the changes, since I last commented. If you want, I can take a look, give feedback and maybe accept it, if everything looks ok. Do you want me to?

@arvindsv Yes that would be great! Please review the changes and give some feedback! We'd like to see this go through :)

Yup, yup. @pwen and I are pulling your negotiation leg hint hint
"Let's write another story for that feature" 😁

Overall, I'm almost ready to merge this in (even with my thoughts about UI changes). The two things that are making me squirm a bit are:

1. The Gemfile change.
2. The DefaultPluginManager change.

If those are resolved, I can accept this change, while you continue further, on changing the structure of how plugins are loaded, if you want.

@arvindsv So I reverted the .gitignore file change and refactored DefaultPluginManager. I also found a bug where the website blows up when someone clicks "upload" when nothing is selected from the disk yet. How do things look now?

I agree with you that uploading a plugin is probably not the most common use case. I can take a look at your UI suggestions tonight. On a related note, where are we testing the javascript interactions on the rails side (e.g. toggle hide/show of the upload form when button is clicked).

@pwen, It's looking good!

About testing Javascript, it's Jasmine tests. You can see them running in the build here. Not visually, but the fact that they're running. Username: view and password: password.

Usually, I leave only a small initialization step in the ERB and move the Javascript out into a different file, so that it can be used. I'll talk about this with an example. Here's the commit of a feature in progress. Confusingly enough, it is the UI for feature toggles, which will be used for nearly every feature, going forward, so that a release can be made at any time, without worrying too much about what has gone in. In fact, I think I will feature toggle the plugin upload feature as well, for a little while, so that we can iterate on it, without confusing anyone, if we make a release. We can get rid of the toggle soon.

Back to the commit:

1. As I said, I usually add an initialization step in the ERB and verify it.
2. Then, the Javascript is in a different file, and Jasmine specs are written for it.

I don't like the slowness of the feedback loop during running Jasmine specs, if I run them the way the build runs it. That's because it brings up the server, using JRuby, etc. I prefer to write my Jasmine tests outside, using just plain Jasmine and then, bring the code and specs over to the Go codebase. In the build, it is the right thing to do, because the specs run against the Javascript from the asset pipeline, that is the real Javascript used by Go.

Hope that makes sense. Let me know if it doesn't.

Forgot to mention: There's always some old code which doesn't do this (and has inline Javascript, which is not tested well). I do this, to make sure that it is tested. I'd definitely recommend this.

@arvindsv makes sense, thanks for the explanation!

anything else we need to do for this to be merged?

Nope, nothing else for this one, I think. I was running all the tests, to make sure it's ok. They're all passing. I'll accept this now. I worked on feature toggling this as well. So, I'll merge this and then apply that on top.

I've submitted the feature toggle as well, here. The build is running. Thank you, @pamo and @pwen! This is looking like the start of a lot of nice changes around this area or other areas! 👍

@arvindsv Hey! @pamo and I are having our second hack night, and are excited to contribute to this.

What do you think should be the next thing in this capability that we should work on? We were thinking about the restart mechanism. If that's the case, we'd like some guidance on this. What are your thoughts?

Hi @pwen and @pamo. Very cool. Here are some thoughts:

I'd not look at the restart mechanism. I don't think the restart mechanism should be strictly necessary. It should be possible to upload a plugin and load it, without having to restart the server. As I might have told you before, it was done to make sure that the Java perm gen space is properly freed after an older version of a plugin is removed (Java's GC does not garbage collect types, at least the older ones didn't). So, I'd look at the ability to run verifications on a plugin, without placing it on the filesystem, and rejecting the upload. If that doesn't seem interesting enough :), I'd consider reloading plugins on the agent side. Right now, plugins get reloaded only on the server side.

If those two don't interest you, then definitely look at the restart mechanism (maybe you've already started looking at it). Let me know if you need any help.