search

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 for arguments containing spaces #41

Closed harbulot closed 1 year ago

harbulot commented 1 year ago

Describe the bug

It seems impossible to pass Maven options with values that contain spaces. This would generally apply to any jf command that may contain spaces.

Current behavior

jf "mvn-config --repo-deploy-releases libs-release-local --repo-deploy-snapshots libs-snapshot-local"

jf "mvn -Dmaven.test.failure.ignore=true clean install -Ddeploy.testProperty=\"Test Property\""

This results in this error:

[main] ERROR org.apache.maven.cli.MavenCli - Unknown lifecycle phase "Property"".

Reproduction steps

This seems to be due to the fact the jf argument string is simply split by whitespace in JfStep:

builder.add(StringUtils.split(args));

I tried to work around this by setting MAVEN_OPTS, but Maven itself more or less has the same problem: https://issues.apache.org/jira/browse/MNG-4559

In general, this also raises questions about character escaping. For example, even without whitespace, the quotes would be including in the property deployment if you use -Ddeploy.testProperty="TestProperty".

Expected behavior

We should be able to use -Ddeploy.testProperty="Test Property" as a valid option.

JFrog plugin version

1.0.5

JFrog CLI version

2.34.6

Operating system type and version

Linux

Workaround

For Maven, here is a potential workaround, which relies on writing those options in a .mvn/maven.config file in the workspace. This might require more work if such a file already exists in the source tree. (This workaround also requires a number of additional script permissions, in particular w.r.t creating and modifying files, which may or may not be desirable.)

script {
    new File("${WORKSPACE}/.mvn").mkdirs()
    def mavenConfigFile = new File("${WORKSPACE}/.mvn/maven.config")
    mavenConfigFile.write("-Ddeploy.testProperty=Test Property\n") // Note: no quotes
    //mavenConfigFile.append("-Dxyz=123\n")
}

jf "mvn -Dmaven.test.failure.ignore=true clean install"

This only works with Maven 3.9.0 (and later, presumably), and requires one setting per line in this file, as mentioned in the Maven documentation:

NOTICE starting with Maven 3.9.0 each single argument must be put in new line.

Arguments with spaces fail altogether with Maven 3.8.7 (even if it's just one):

[Info] Running Mvn...
Unable to parse maven.config: Unrecognized maven.config entries: [Property]
yahavi commented 1 year ago

@harbulot thanks for reporting this issue, We created #45 to fix it. We'd also very much appreciate any feedback on the pull request and the solution :)

harbulot commented 1 year ago

Thank you @yahavi, that's a very fast reply!

I've compiled and tested your patch on a Jenkins installation.

I think there are still a few issues regarding quotations marks.

Sample Maven project (to display the properties)

This displays the test.* properties, in plain text and in XML (to see potentially escaped characters)

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <groupId>com.example</groupId>
    <artifactId>echo-properties-test</artifactId>
    <version>1.0.0-SNAPSHOT</version>
    <packaging>jar</packaging>
    <properties>
        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
    </properties>
    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-antrun-plugin</artifactId>
                <version>3.1.0</version>
                <executions>
                    <execution>
                        <id>textoutput</id>
                        <phase>validate</phase>
                        <goals>
                            <goal>run</goal>
                        </goals>
                        <configuration>
                            <target>
                                <echoproperties prefix="test." />
                            </target>
                        </configuration>
                    </execution>
                    <execution>
                        <id>xmloutput</id>
                        <phase>validate</phase>
                        <goals>
                            <goal>run</goal>
                        </goals>
                        <configuration>
                            <target>
                                <echoproperties prefix="test." format="xml" />
                            </target>
                        </configuration>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>
</project>

I've also put this into .mvn/maven-config for reference (This needs at least Maven 3.9.0):

-Dtest.maven-config.test1=MavenConfigTest1
-Dtest.maven-config.test2=Maven Config Test 2
-Dtest.maven-config.test3="Maven Config Test 3"
-Dtest.maven-config.test4='Maven Config Test 4'

Running with a Bash shell directly

I'd use the Bash shell invocation as what I'd tend to expect naturally around quotation marks behaviour.

BASH_VAR_1="Bash Var 1"
BASH_VAR_2='Bash "quote"'
mvn validate -Dtest.bashcli.test1="Test Property 1" -Dtest.bashcli.testvar1="$BASH_VAR_1" -Dtest.bashcli.testvar2="$BASH_VAR_2"
[INFO] [echoproperties] test.bashcli.test1=Test Property 1
[INFO] [echoproperties] test.bashcli.testvar1=Bash Var 1
[INFO] [echoproperties] test.bashcli.testvar2=Bash "quote"
[INFO] [echoproperties] test.maven-config.test1=MavenConfigTest1
[INFO] [echoproperties] test.maven-config.test2=Maven Config Test 2
[INFO] [echoproperties] test.maven-config.test3="Maven Config Test 3"
[INFO] [echoproperties] test.maven-config.test4='Maven Config Test 4'
...
[INFO] [echoproperties] <?xml version="1.0" encoding="UTF-8"?><properties>
[INFO] [echoproperties]         <property name="test.bashcli.test1" value="Test Property 1" />
[INFO] [echoproperties]         <property name="test.bashcli.testvar1" value="Bash Var 1" />
[INFO] [echoproperties]         <property name="test.bashcli.testvar2" value="Bash &quot;quote&quot;" />
[INFO] [echoproperties]         <property name="test.maven-config.test1" value="MavenConfigTest1" />
[INFO] [echoproperties]         <property name="test.maven-config.test2" value="Maven Config Test 2" />
[INFO] [echoproperties]         <property name="test.maven-config.test3" value="&quot;Maven Config Test 3&quot;" />
[INFO] [echoproperties]         <property name="test.maven-config.test4" value="&apos;Maven Config Test 4&apos;" />
[INFO] [echoproperties] </properties>

What we can see from this example is that the quotation marks around "Test Property 1" and "$BASH_VAR_1" and "$BASH_VAR_2" are actually handled by the shell: they are not part of the actual values.

This is in part due to using the features of "$@" with quotes in the mvn command itself. Even if mvn wasn't a shell script but a plain command-line applications, you would expect the argv (or equivalent) to be an array with:

In contrast, the properties defined in .mvn/maven-config are only delimited by the end of line (in Maven 3.9.0, not before), so whatever quotation marks are used are actually part of the value:

test.maven-config.test1=MavenConfigTest1
test.maven-config.test2=Maven Config Test 2
test.maven-config.test3="Maven Config Test 3"
test.maven-config.test4='Maven Config Test 4'

Running with a Bash shell via jf

BASH_VAR_1="Bash Var 1"
BASH_VAR_2='Bash "quote"'
jf mvn validate -Dtest.bashcli.test1="Test Property 1" -Dtest.bashcli.testvar1="$BASH_VAR_1" -Dtest.bashcli.testvar2="$BASH_VAR_2"

This was just to compare, but this produces the same results as without jf.

Running in a Jenkins pipeline with PR #45

(This is based on commit 3e248b19c8f63d06cdf9669d219c9688563db256)

Simple case

jf "mvn validate -Dtest.jfcli.test1=TestProperty1 -Dtest.jfcli.test2=\"Test Property 2\""
[main] INFO org.apache.maven.plugins.antrun.AntRunMojo - [echoproperties] test.jfcli.test1=TestProperty1
[main] INFO org.apache.maven.plugins.antrun.AntRunMojo - [echoproperties] test.jfcli.test2="Test Property 2"

PR #45 is clearly an improvement, since we can now use values with spaces. However the quotation mark is included in the value, which is generally not expected.

The implication is that you couldn't have values with spaces without also having the enclosing quotes as part of the value (that's not something a shell forces you to have, and it doesn't always make sense).

Case with variables

I'm not sure if it's fair to have that expectation, but it would also be good to be able to pass variables (same as one would do in a shell script).

script {
    env.setProperty("TEST_PROP_3", "Test Property 3")
    env.setProperty("TEST_PROP_4", "Test \"Property\" 4")
}

jf "mvn validate -Dtest.jfcli.test3=\"${TEST_PROP_3}\" -Dtest.jfcli.test4=\"${TEST_PROP_4}\""

This works, but also includes the quotes around:

[main] INFO org.apache.maven.plugins.antrun.AntRunMojo - [echoproperties] test.jfcli.test3="Test Property 3"
[main] INFO org.apache.maven.plugins.antrun.AntRunMojo - [echoproperties] test.jfcli.test4="Test "Property" 4"
script {
    env.setProperty("TEST_PROP_5", "Test with one double-quote \" only")
}

jf "mvn validate -Dtest.jfcli.test5=\"${TEST_PROP_5}\""

This one fails with this in the logs:

$ /var/lib/jenkins/tools/io.jenkins.plugins.jfrog.JfrogInstallation/jfrog-cli/jf 'mvn validate -Dtest.jfcli.test5="Test' with one double-quote '" only"'
'jf mvn validate -Dtest.jfcli.test5="Test' is not a jf command. See --help

The previous sucessful example showed this command:

$ /var/lib/jenkins/tools/io.jenkins.plugins.jfrog.JfrogInstallation/jfrog-cli/jf mvn validate '-Dtest.jfcli.test3="Test Property 3"' '-Dtest.jfcli.test4="Test "Property" 4"'

Conclusion

I don't think there's a simple solution to this problem.

Even if you manage to find a regex that gets rid of the paired double quotes surrounding each token, that would only work for fixed values. Anything coming from variables would at least need to be escape, if it's even possible in the general case.

I realise the syntax would be less user-friendly (and I'm not sure if it's possible), but perhaps the jf step could take an array as an argument instead of a string that's subsequently split. It seems otherwise difficult to design something that can safely take values coming from variables.

The problem here is pretty much the same that led to Runtime.exec(String) to be deprecated in Java 18, in favour if Runtime.exec(String[])

harbulot commented 1 year ago

I don't really have any experience with writing Jenkins plugins, but I've tried this fairly simple change:

diff --git a/src/main/java/io/jenkins/plugins/jfrog/JfStep.java b/src/main/java/io/jenkins/plugins/jfrog/JfStep.java
index 0224ee7..96aed93 100644
--- a/src/main/java/io/jenkins/plugins/jfrog/JfStep.java
+++ b/src/main/java/io/jenkins/plugins/jfrog/JfStep.java
@@ -41,14 +41,14 @@ public class JfStep<T> extends Builder implements SimpleBuildStep {
     public static final String JFROG_CLI_BUILD_NAME = "JFROG_CLI_BUILD_NAME";
     public static final String JFROG_CLI_BUILD_NUMBER = "JFROG_CLI_BUILD_NUMBER";
     public static final String JFROG_CLI_BUILD_URL = "JFROG_CLI_BUILD_URL";
-    protected String args;
+    protected String[] args;

     @DataBoundConstructor
-    public JfStep(String args) {
+    public JfStep(String[] args) {
         this.args = args;
     }

-    public String getArgs() {
+    public String[] getArgs() {
         return args;
     }

@@ -78,7 +78,7 @@ public class JfStep<T> extends Builder implements SimpleBuildStep {
             jfrogBinaryPath = FilenameUtils.separatorsToUnix(jfrogBinaryPath);
         }
         builder.add(jfrogBinaryPath);
-        builder.add(StringUtils.split(args));
+        builder.add(args);
         if (isWindows) {
             builder = builder.toWindowsCommand();
         }

The jf calls now expect an array:

jf(["mvn-config","--repo-deploy-releases","libs-release-local","--repo-deploy-snapshots","libs-snapshot-local"])

script {
    env.setProperty("TEST_PROP_3", "Test Property 3")
    env.setProperty("TEST_PROP_4", "Test \"Property\" 4")
    env.setProperty("TEST_PROP_5", "Test with one double-quote \" only")
}

jf(["mvn","validate",
    "-Dtest.jfcli.test1=TestProperty1",
    "-Dtest.jfcli.test2=Test Property 2",
    "-Dtest.jfcli.test3=${TEST_PROP_3}",
    "-Dtest.jfcli.test4=${TEST_PROP_4}",
    "-Dtest.jfcli.test5=${TEST_PROP_5}"])

The output seems to work fine now, even with variables:

[echoproperties] test.jfcli.test1=TestProperty1
[echoproperties] test.jfcli.test2=Test Property 2
[echoproperties] test.jfcli.test3=Test Property 3
[echoproperties] test.jfcli.test4=Test "Property" 4
[echoproperties] test.jfcli.test5=Test with one double-quote " only

The downside is that it's a breaking change in terms of usage of the jf step.

yahavi commented 1 year ago

Thanks for this feedback, @harbulot! We created https://github.com/jfrog/jenkins-jfrog-plugin/pull/48 to implement your suggestion. We'd appreciate your feedback on that.

harbulot commented 1 year ago

That's great @yahavi , thank you! Supporting both strings and lists sounds like a good idea. I've tried the patch and it works with both (handy for fixed commands that don't need parameters (e.g. jf 'rt build-add-git').

I'd just suggest expanding a bit in the README. Perhaps something along these lines:

If the JFrog CLI command contains some arguments with spaces, you can rather provide the arguments as a list:

jf(['mvn', 'clean', 'install', '-Ddeploy.testProperty=Property with space'])

In this case, the quotes that would be required in a shell are not necessary (and would be part of the value itself, if used). The above step is equivalent to a shell invokation as follows:

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

Passing the arguments as a list can also be useful to avoid space and escaping problems if some of those arguments rely on script variables.

I've also noticed a couple of minor typos in the patch (commented on PR #48 directly).

yahavi commented 1 year ago

Thanks again for the suggestion. I'm not a native English speaker and it was helpful. I applied your suggestions in my branch - https://github.com/yahavi/jenkins-jfrog-plugin/tree/list#using-jfrog-cli-in-your-pipeline-jobs

However, I'm afraid that having a JFrog CLI shell command in the README will be confusing:

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

My solution was to upload an image to make it non-copyable.

harbulot commented 1 year ago

I must have missed something in the README update, but that's not necessarily correct. This note probably shouldn't be there, and can be misleading:

IMPORTANT: Notice the single quotes wrapping the command right after the jf step definition.

When reading the README, I'm not sure whether this "IMPORTANT" note refers to the sentence above (single string argument) or below (list argument)

When using a list, it would actually be perfectly fine to have something like this:

def CUSTOM_PROP="This is my build property with spaces"
//...
jf(['mvn','clean','install',"-Dtest.prop=${CUSTOM_PROP}"]) // Using Groovy string interpolation with double-quotes

A list argument makes it safer to use double-quote wrapping (depending on what jf command is actually used, but no worse than jf with a single string argument).

yahavi commented 1 year ago

Hi again @harbulot, Thanks for this feedback. We will consider your suggestions for the README and add them in another PR.

Update - Jenkins Artifactory plugin 1.2.0 is released and will be available shortly in the Jenkins plugin manager. This version includes this feature. We're looking forward to your feedback.

harbulot commented 1 year ago

Thank you @yahavi, the 1.2.0 release works!