jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Phillips <notificati...@github.com>
Subject Re: [jclouds/jclouds-cli] Fix the script engine in the jclouds CLI (#35)
Date Thu, 10 Nov 2016 00:19:31 GMT
demobox commented on this pull request.



> @@ -344,7 +349,17 @@ private void discoverCommands(CommandProcessorImpl commandProcessor,
ClassLoader
                         @Override
                         public Action createNewAction() {
                             try {
-                                return ((Class<? extends Action>) actionClass).newInstance();
+                               Action action = ((Class< ? extends Action>) actionClass).newInstance();
+                               if (action instanceof ComputeCommandBase) {
+                                   ShellTableFactory shellTableFactory = ((ComputeCommandBase)
action).getShellTableFactory();
+                                   if (shellTableFactory instanceof BasicShellTableFactory)
{
+                                       BasicShellTableFactory factory = (BasicShellTableFactory)
shellTableFactory;
+                                       if (factory.getScriptEngineManager() == null) {
+                                           factory.setScriptEngineManager(new ScriptEngineManager());

>  I preferred to leave jclouds-karaf clean, and add this ugly workaround in the CLI.

That makes sense for me - it's the CLI that is doing the "odd" thing by manually instantiating
the commands in the first place, as you describe above. Looking at the code, we've also added
this change in the place where we are instantiating those objects, which also makes sense
to me.

The more minor questions I would still have are:

* `new ScriptEngineManager()` - do we need a new instance here each time, or can an instance
be shared?
* move the `if (action instanceof ComputeCommandBase) {...` block into a method `configureShellTableFactoryIfNeeded`
or so, just to reduce some of the noise?
* `if (factory.getScriptEngineManager() == null) {` - is that ever expected to be **non**-`null`?
* add a comment describing a bit of what you wrote above? That helped me a lot in understanding
why we are doing this - hopefully others too ;-)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-cli/pull/35
Mime
View raw message