Conversation
|
Any thoughts on this one? Would be really useful to know whether such functionality can be eventually accepted and whether this is the right approach for it. Thank you. |
Yes please. It is not just about comfort when reviewing diffs; if we ever need to |
| * You must have used {@link #createSecureCompilerConfiguration} to prepare the Groovy shell. | ||
| * @param script a script ready to {@link Script#run}, created for example by {@link GroovyShell#parse(String)} | ||
| * @param whitelist the whitelist to use, such as {@link Whitelist#all()} | ||
| * @return the sandbox for running this script |
There was a problem hiding this comment.
Would be helpful to @link to register and unregister so callers can see how to use it.
| } | ||
| } | ||
|
|
||
| public PreparedScript prepare(ClassLoader loader, Binding binding) throws IllegalAccessException, IOException { |
There was a problem hiding this comment.
Deserves Javadoc at least for the loader parameter, which in evaluate is nontrivial.
| } | ||
|
|
||
| } finally { | ||
| public void cleanUp() throws IOException { |
There was a problem hiding this comment.
Better implement Closeable and make this the close method, so PreparedScript can be used in a Java 7+ try block.
| } catch (NoSuchFieldException nsme) { | ||
| LOGGER.log(Level.FINE, "GroovyShell fields have changed, field loader no longer exists -- memory leak fixes won't work"); | ||
| /** | ||
| * Allows access to execue a script multiple times without preparation and clean-up overhead. |
|
|
||
| public Object run() throws Exception { | ||
| if (sandbox) { | ||
| scriptSandbox.register(); |
There was a problem hiding this comment.
Note to self: register and unregister calls should be cheap enough that they contribute no significant overhead.
| assertTrue((int) res == 6); | ||
| assertTrue((int) b.getVariable("a") == 7); | ||
| } catch (Exception e) { | ||
| script.cleanUp(); |
There was a problem hiding this comment.
This should be in a finally block, not a catch block (or just use Closeable trick mentioned above).
There was a problem hiding this comment.
strange that I did this, I'll check your Closeable suggestion
| assertTrue((int) res == 6); | ||
| assertTrue((int) b.getVariable("a") == 7); | ||
| } catch (Exception e) { | ||
| script.cleanUp(); |
|
Hello, updated according to your reasonable comments. Please let me know if you see further possible improvements (or if I missed anything). One question - would the automatic PR test job deploy a timestamped snapshot or does it have to be deployed in some other way? |
Afraid not.
IIRC you just need a jenkins.io account, then I use a helper script: mvn clean install source:jar deploy:deploy -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/snapshots/ -DskipTestsPlain |
BTW that has been superseded by JEP-305. Docs |
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
Last I checked, the idea sounded fine but I have not looked at the details. Leaving it to plugin owners.
2 participants
Hello, I want to be able to run a groovy script many times without needless overhead.
This is needed for me to implement custom log lines processing in Logstash plugin.
At the same time I'd like the convenience and safety of all the magic presently done
by the secure groovy script plugin. This modification is to allow that.
Sorry about white-space changes. It's because of the editor settings. If you don't
want them, I'll go ahead and revert them. Same about the EditorConfig file. Just
thought it is a useful addition.
If you like to keep the removed needless whilte spaces, you could add
?w=1to urlof the file diff to have them filtered out while reviewing:
https://github.com/jenkinsci/script-security-plugin/pull/169/files?w=1