Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
+1! Many thanks @demobox! -- 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-karaf/pull/79#issuecomment-246259756
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
@nacx Updated with some additional changes -- 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-karaf/pull/79#issuecomment-246219889
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
Just two final comments, but LGTM! -- 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-karaf/pull/79#issuecomment-246199894
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
Do you think it makes sense to move the copied classes to their own package so they are easy to reference (if that is needed or helps at some point)? -- 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-karaf/pull/79#issuecomment-246199857
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -51,8 +52,9 @@ public String evaluate(Object obj, String expression) {
> try {
>scriptEngine.put(getType(), obj);
>result = String.valueOf(scriptEngine.eval(expression));
> -} catch (Exception ex) {
> - //Ignore
> +} catch (Exception exception) {
> + result = format("Unable to evaluate expression %s due to: %s. Please
> check your shell confugration",
H that text will go in a column and it will probably be trimmed or
difficult to read. WDYT about changing it to "## VALUE ERROR ##" or whatever
short error marker we like, and move the detailed message to the log, with the
complete stacktrace?
--
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-karaf/pull/79/files/5e0fcbef105bdb8bed0461519fb7b0d2671beed3#r78301091
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
>private final ScriptEngine scriptEngine;
>
>/**
> * Constructor
> * @param engine
> */
> - public ScriptEngineShellTable(String engine) {
> -this.engine = engine;
> -this.scriptEngine = scriptEngineFactory.getEngineByName(engine);
> + public ScriptEngineShellTable(ScriptEngineManager scriptEngineManager,
> String engine) {
> +this.scriptEngine = scriptEngineManager.getEngineByName(engine);
> +if (scriptEngine == null) {
> + throw new IllegalStateException("Unable to load script engine " +
> engine);
> +}
Lgtm!
--
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-karaf/pull/79/files/5e0fcbef105bdb8bed0461519fb7b0d2671beed3#r78301017
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
@nacx Reformatted and added the ISE if the requested engine can't be loaded. Please take a look to see there's anything that still needs to be changed! -- 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-karaf/pull/79#issuecomment-246185884
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -51,8 +52,9 @@ public String evaluate(Object obj, String expression) {
> try {
>scriptEngine.put(getType(), obj);
>result = String.valueOf(scriptEngine.eval(expression));
> -} catch (Exception ex) {
> - //Ignore
> +} catch (Exception exception) {
> + result = format("Unable to evaluate expression %s due to: %s. Please
> check your shell confugration",
@nacx Suggestions for different messages welcome ;-)
--
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-karaf/pull/79/files/5e0fcbef105bdb8bed0461519fb7b0d2671beed3#r78297013
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
>private final ScriptEngine scriptEngine;
>
>/**
> * Constructor
> * @param engine
> */
> - public ScriptEngineShellTable(String engine) {
> -this.engine = engine;
> -this.scriptEngine = scriptEngineFactory.getEngineByName(engine);
> + public ScriptEngineShellTable(ScriptEngineManager scriptEngineManager,
> String engine) {
> +this.scriptEngine = scriptEngineManager.getEngineByName(engine);
> +if (scriptEngine == null) {
> + throw new IllegalStateException("Unable to load script engine " +
> engine);
> +}
@nacx Something like this?
--
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-karaf/pull/79/files/5e0fcbef105bdb8bed0461519fb7b0d2671beed3#r78297007
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -0,0 +1,79 @@ > +/* +1! -- 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-karaf/pull/79/files/0b99dad1aa7786eaf11028bf91fc1d272c189f0b#r78129943
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -0,0 +1,79 @@ > +/* > No changes to the notice file should be required as the copyright owner of > the copied files is already the ASF. Ah, good to know. Then I think we should be OK just to reformat and perhaps tweak the style where applicable? -- 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-karaf/pull/79/files/0b99dad1aa7786eaf11028bf91fc1d272c189f0b#r78120088
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -51,9 +47,7 @@ public String evaluate(Object obj, String expression) {
> try {
>scriptEngine.put(getType(), obj);
>result = String.valueOf(scriptEngine.eval(expression));
> -} catch (Exception ex) {
> - //Ignore
> -}
> +} catch (Exception ignored) {}
> return result;
If we throw the ISE int he constructor, could we assume then that failures here
are due to the expressions used to extract the values of the columns, thus,
non-fatal errors users can fix by editing the shell.cfg file?
--
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-karaf/pull/79/files/6c68f0d234eae7732069b14ac47bad8c885eed27#r78021836
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -0,0 +1,79 @@ > +/* Regarding licensing, according to [this](http://www.apache.org/legal/src-headers.html#3party) and [this](http://www.apache.org/legal/src-headers.html#notice), we should be good to go. Although it is not the right header for software developed at Apache (that is a Felix issue), we should keep it as-is. No changes to the notice file should be required as the copyright owner of the copied files is already the ASF. -- 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-karaf/pull/79/files/0b99dad1aa7786eaf11028bf91fc1d272c189f0b#r78007520
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> I've just tried a rebased version of this branch on top of the shade one and > everything works fine! :) @nacx Nice! Agree with most of your comments - please feel free to add commits to this branch with any changes you think would make sense. Thanks for taking a look, and for testing! -- 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-karaf/pull/79#issuecomment-245504171
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -51,9 +47,7 @@ public String evaluate(Object obj, String expression) {
> try {
>scriptEngine.put(getType(), obj);
>result = String.valueOf(scriptEngine.eval(expression));
> -} catch (Exception ex) {
> - //Ignore
> -}
> +} catch (Exception ignored) {}
> return result;
> That would make more sense for users than a bunch of empty lines.
I find it difficult to decide one way or the other without knowing what's
failing here, and whether that's more of the "fatal error" category, or whether
it could be the result of user error and be recoverable. It would certainly be
good to set `result` for user-related errors, and perhaps to blow up for errors
that indicate a broken configuration.
I'm just not sure if we can pick those apart simply from the exception type?
--
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-karaf/pull/79/files/6c68f0d234eae7732069b14ac47bad8c885eed27#r77951138
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
>private final ScriptEngine scriptEngine;
>
>/**
> * Constructor
> * @param engine
> */
> - public ScriptEngineShellTable(String engine) {
> -this.engine = engine;
> -this.scriptEngine = scriptEngineFactory.getEngineByName(engine);
> + public ScriptEngineShellTable(ScriptEngineManager scriptEngineManager,
> String engine) {
> +this.scriptEngine = scriptEngineManager.getEngineByName(engine);
> We'd better fail here instead of silently returning null?
What would you suggestion? `IllegalStateException` or so?
--
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-karaf/pull/79/files/6c68f0d234eae7732069b14ac47bad8c885eed27#r77950939
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -0,0 +1,79 @@ > +/* > Do you plan to re-style the classes to align them to our coding style and > change them to cleanup the code? I think we should certainly align the code style; not sure about more comprehensive re-writes, though. Agree that we should change the functionality (esp. the silent failures) where we feel we should behave differently. -- 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-karaf/pull/79/files/0b99dad1aa7786eaf11028bf91fc1d272c189f0b#r77950850
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
>Could you give (a CLI built from) this a shot, on top of your "shade" changes >to get the agentproxy to work again? It looks like it should be working I've just tried a rebased version of this branch on top of the `shade` one and everything works fine! :) -- 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-karaf/pull/79#issuecomment-245397642
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -0,0 +1,79 @@ > +/* Do you plan to re-style the classes to align them to our coding style and change them to cleanup the code? Or do you think it makes sense to leave them unchanged in their own package? -- 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-karaf/pull/79/files/0b99dad1aa7786eaf11028bf91fc1d272c189f0b#r77892167
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -51,9 +47,7 @@ public String evaluate(Object obj, String expression) {
> try {
>scriptEngine.put(getType(), obj);
>result = String.valueOf(scriptEngine.eval(expression));
> -} catch (Exception ex) {
> - //Ignore
> -}
> +} catch (Exception ignored) {}
> return result;
Would it make sense to change the default value of the `result` variable to
`N/A` or similar? That would make more sense for users than a bunch of empty
lines.
--
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-karaf/pull/79/files/6c68f0d234eae7732069b14ac47bad8c885eed27#r77891994
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
>private final ScriptEngine scriptEngine;
>
>/**
> * Constructor
> * @param engine
> */
> - public ScriptEngineShellTable(String engine) {
> -this.engine = engine;
> -this.scriptEngine = scriptEngineFactory.getEngineByName(engine);
> + public ScriptEngineShellTable(ScriptEngineManager scriptEngineManager,
> String engine) {
> +this.scriptEngine = scriptEngineManager.getEngineByName(engine);
We'd better fail here instead of silently returning null?
--
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-karaf/pull/79/files/6c68f0d234eae7732069b14ac47bad8c885eed27#r77891853
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
@nacx Could you give (a CLI built from) this a shot? It looks like it should be working ``` jclouds> jclouds:compute-service-create --provider aws-ec2 --identity key --credential secret --name test Waiting for compute service with name: test. jclouds> jclouds:location-list --provider aws-ec2 --name test ** For engine groovy, result is: org.jclouds.karaf.commands.table.internal.O SGiScriptEngine@5c322524 [id][scope] [description] [parent] us-west-2 REGION us-west-2 aws-ec2 eu-west-1 REGION eu-west-1 aws-ec2 ap-northeast-1a ZONE ap-northeast-1a ap-northeast-1 ap-northeast-1c ZONE ap-northeast-1c ap-northeast-1 ap-southeast-1a ZONE ap-southeast-1a ap-southeast-1 eu-west-1a ZONE eu-west-1a eu-west-1 us-east-1b ZONE us-east-1b us-east-1 ``` -- 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-karaf/pull/79#issuecomment-245166148
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> + return null;
> +}
> + }
> +
> + /**
> + * Iterates through all bundles to get the available @link
> ScriptEngineFactory classes
> + * @return the names of the available ScriptEngineFactory classes
> + * @throws IOException
> + */
> + private List findFactoryCandidates(BundleContext context) {
> + Bundle[] bundles = context.getBundles();
> + List factoryCandidates = new ArrayList();
> + for (Bundle bundle : bundles) {
> + if (bundle == null) {
> +continue;
> + }
Wow, some funky formatting there ;-)
--
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-karaf/pull/79/files/0b99dad1aa7786eaf11028bf91fc1d272c189f0b#r77755910
Re: [jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
> @@ -0,0 +1,79 @@ > +/* Lots of cleanup and review still to go with these three classes -- 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-karaf/pull/79/files/0b99dad1aa7786eaf11028bf91fc1d272c189f0b#r77755845
[jclouds/jclouds-karaf] Try to use an OSGi-compliant way of loading JSR 223 script engines (#79)
Alternative to #78, but addressing the same issue. You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds-karaf/pull/79 -- Commit Summary -- * Try to use an OSGi-compliant way of loading JSR 223 script engines -- File Changes -- M commands/src/main/java/org/jclouds/karaf/commands/table/BasicShellTableFactory.java (9) A commands/src/main/java/org/jclouds/karaf/commands/table/internal/OSGiScriptEngine.java (79) A commands/src/main/java/org/jclouds/karaf/commands/table/internal/OSGiScriptEngineFactory.java (83) A commands/src/main/java/org/jclouds/karaf/commands/table/internal/OSGiScriptEngineManager.java (272) M commands/src/main/java/org/jclouds/karaf/commands/table/internal/ScriptEngineShellTable.java (12) M commands/src/main/resources/OSGI-INF/blueprint/jclouds-commands.xml (8) M feature/src/main/resources/feature.xml (1) -- Patch Links -- https://github.com/jclouds/jclouds-karaf/pull/79.patch https://github.com/jclouds/jclouds-karaf/pull/79.diff -- 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-karaf/pull/79
