jfrog / jenkins-jfrog-plugin

Easy integration between Jenkins and the JFrog Platform.
https://github.com/jfrog/jenkins-jfrog-plugin
Apache License 2.0
39 stars 17 forks source link

Support providing arguments as list #48

Closed yahavi closed 1 year ago

yahavi commented 1 year ago

Fix #41 Supersedes https://github.com/jfrog/jenkins-jfrog-plugin/pull/45

Allow providing arguments either as strings or as a list.

Motivation Providing an argument with space is impossible today:

jf 'mvn clean install -Ddeploy.testProperty=Property with space'

Produces the following error:

ERROR org.apache.maven.cli.MavenCli - Unknown lifecycle phase "with".

Solution Provide arguments as a list:

jf(['mvn', 'clean', 'install', '-Ddeploy.testProperty=Property with space"'])
github-actions[bot] commented 1 year ago

What is Frogbot?

harbulot commented 1 year ago

That's great!

I've noticed a couple of minor typos in the patch:

yahavi commented 1 year ago

Thanks for the code review, @harbulot! I added another commit to fix all of your comments.

harbulot commented 1 year ago

Thanks!

Apologies, I really know the plugin code, I think I may have been wrong about the 3rd point about double quotes. This may in fact have nothing to do with Groovy script variable interpolation, but it would rather be replaced separately by the test framework (in which case single quotes might be safer, if it is then a literal in the executed pipeline): https://github.com/jenkinsci/jfrog-plugin/blob/67e942afd4059a0161f337c6e85f0cbff5b3c181/src/test/java/io/jenkins/plugins/jfrog/integration/PipelineTestBase.java#L251 (Essentially, it was probably fine and better as it was)

What do you think about the comments about the README (I've commented on #41 directly, maybe I should have kept it here)?

harbulot commented 1 year ago

Thanks a lot @yahavi!

Sorry about the delay in commenting: I would have kept single quotes in your test pipe line. As I was saying in this comment, what I believe I was wrong in what I said in my 3rd bullet point in this comment. Really sorry about that.

That said, as it stands, I don't think it's a major issue unless the values of DUMMY_FILE_PATH or LOCAL_REPO contain something that can be interpolated as a Groovy string.