1
0
mirror of https://github.com/SAP/jenkins-library.git synced 2025-01-20 05:19:40 +02:00

Fix maven parameter handling (#1493)

Avoid maven error `Unknown lifecycle phase \"-\"` when the value of a define contains `-`.

Don't split and trim maven arguments. Expect they come in as a list, keep them as list.

This is a breaking change compared to the old Groovy implementation which relied on using a shell for calling maven.

As an example, consider this diff:

```diff
-        goals: 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
-        defines: "-Dexpression=$pomPathExpression -DforceStdout -q",
+        goals: ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
+        defines: ["-Dexpression=$pomPathExpression", "-DforceStdout", "-q"],
```
This commit is contained in:
Florian Wilhelm 2020-05-06 17:43:32 +02:00 committed by GitHub
parent 190ff83caa
commit eaf5479e9c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 235 additions and 90 deletions

View File

@ -4,12 +4,13 @@ import (
"github.com/SAP/jenkins-library/pkg/command"
"github.com/SAP/jenkins-library/pkg/log"
"github.com/SAP/jenkins-library/pkg/maven"
sliceUtils "github.com/SAP/jenkins-library/pkg/piperutils"
"io/ioutil"
"github.com/SAP/jenkins-library/pkg/telemetry"
)
var writeFile = ioutil.WriteFile
func mavenExecute(config mavenExecuteOptions, _ *telemetry.CustomData) {
runner := command.Command{}
err := runMavenExecute(config, &runner)
@ -24,22 +25,16 @@ func runMavenExecute(config mavenExecuteOptions, runner execRunner) error {
ProjectSettingsFile: config.ProjectSettingsFile,
GlobalSettingsFile: config.GlobalSettingsFile,
M2Path: config.M2Path,
Goals: splitAndTrimParams(config.Goals),
Defines: splitAndTrimParams(config.Defines),
Flags: splitAndTrimParams(config.Flags),
Goals: config.Goals,
Defines: config.Defines,
Flags: config.Flags,
LogSuccessfulMavenTransfers: config.LogSuccessfulMavenTransfers,
ReturnStdout: config.ReturnStdout,
}
output, err := maven.Execute(&options, runner)
if err == nil && config.ReturnStdout {
err = ioutil.WriteFile(".pipeline/maven_output.txt", []byte(output), 0644)
err = writeFile(".pipeline/maven_output.txt", []byte(output), 0644)
}
return err
}
// We *must not* deduplicate the parameters here as this will break commands such as `mvn -pl a -pl b`,
// which would become `mvn -pl a b` which is invalid
func splitAndTrimParams(params []string) []string {
return sliceUtils.SplitAndTrim(params, " ")
}

View File

@ -3,33 +3,87 @@ package cmd
import (
"github.com/SAP/jenkins-library/pkg/mock"
"github.com/stretchr/testify/assert"
"os"
"testing"
)
func TestMavenExecute(t *testing.T) {
t.Run("mavenExecute should cleanup the parameters", func(t *testing.T) {
mockRunner := mock.ExecMockRunner{}
t.Run("mavenExecute should write output file", func(t *testing.T) {
// init
config := mavenExecuteOptions{
Flags: []string{"--errors --fail-fast "},
Defines: []string{" -DoutputFile=mvnDependencyTree.txt"},
Goals: []string{" dependency:tree"},
Goals: []string{"goal"},
LogSuccessfulMavenTransfers: true,
ReturnStdout: true,
}
mockRunner := mock.ExecMockRunner{}
mockRunner.StdoutReturn = map[string]string{}
mockRunner.StdoutReturn[""] = "test output"
var outputFile string
var output []byte
oldWriteFile := writeFile
writeFile = func(filename string, data []byte, perm os.FileMode) error {
outputFile = filename
output = data
return nil
}
defer func() { writeFile = oldWriteFile }()
// test
err := runMavenExecute(config, &mockRunner)
// assert
expectedParams := []string{
"--errors",
"--fail-fast",
"-DoutputFile=mvnDependencyTree.txt",
"-Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn",
"--batch-mode",
"dependency:tree",
"--batch-mode", "goal",
}
assert.NoError(t, err)
assert.Equal(t, "mvn", mockRunner.Calls[0].Exec)
assert.Equal(t, expectedParams, mockRunner.Calls[0].Params)
if assert.Equal(t, 1, len(mockRunner.Calls)) {
assert.Equal(t, "mvn", mockRunner.Calls[0].Exec)
assert.Equal(t, expectedParams, mockRunner.Calls[0].Params)
}
assert.Equal(t, "test output", string(output))
assert.Equal(t, ".pipeline/maven_output.txt", outputFile)
})
t.Run("mavenExecute should NOT write output file", func(t *testing.T) {
// init
config := mavenExecuteOptions{
Goals: []string{"goal"},
LogSuccessfulMavenTransfers: true,
}
mockRunner := mock.ExecMockRunner{}
mockRunner.StdoutReturn = map[string]string{}
mockRunner.StdoutReturn[""] = "test output"
var outputFile string
var output []byte
oldWriteFile := writeFile
writeFile = func(filename string, data []byte, perm os.FileMode) error {
outputFile = filename
output = data
return nil
}
defer func() { writeFile = oldWriteFile }()
// test
err := runMavenExecute(config, &mockRunner)
// assert
expectedParams := []string{
"--batch-mode", "goal",
}
assert.NoError(t, err)
if assert.Equal(t, 1, len(mockRunner.Calls)) {
assert.Equal(t, "mvn", mockRunner.Calls[0].Exec)
assert.Equal(t, expectedParams, mockRunner.Calls[0].Params)
}
assert.Equal(t, "", string(output))
assert.Equal(t, "", outputFile)
})
}

View File

@ -178,7 +178,7 @@ func PrepareConfig(cmd *cobra.Command, metadata *config.StepData, stepName strin
}
}
stepConfig.Config = convertTypes(stepConfig.Config, options)
stepConfig.Config = checkTypes(stepConfig.Config, options)
confJSON, _ := json.Marshal(stepConfig.Config)
_ = json.Unmarshal(confJSON, &options)
@ -194,7 +194,7 @@ func PrepareConfig(cmd *cobra.Command, metadata *config.StepData, stepName strin
return nil
}
func convertTypes(config map[string]interface{}, options interface{}) map[string]interface{} {
func checkTypes(config map[string]interface{}, options interface{}) map[string]interface{} {
optionsType := getStepOptionsStructType(options)
for paramName := range config {
@ -205,7 +205,7 @@ func convertTypes(config map[string]interface{}, options interface{}) map[string
paramValueType := reflect.ValueOf(config[paramName])
if paramValueType.Kind() != reflect.String {
// We can only convert from strings at the moment
// Type check is limited to strings at the moment
continue
}
@ -214,14 +214,15 @@ func convertTypes(config map[string]interface{}, options interface{}) map[string
switch optionsField.Type.Kind() {
case reflect.String:
// Types already match, ignore
// Types match, ignore
logWarning = false
case reflect.Slice, reflect.Array:
if optionsField.Type.Elem().Kind() == reflect.String {
config[paramName] = []string{paramValue}
logWarning = false
}
// Could do automatic conversion for those types in theory,
// but that might obscure what really happens in error cases.
log.Entry().Fatalf("Type mismatch in configuration for option '%s'. Expected type to be a list (or slice, or array) but got %s.", paramName, paramValueType.Kind())
case reflect.Bool:
// Sensible to convert strings "true"/"false" to respective boolean values as it is
// common practice to write booleans as string in yaml files.
paramValue = strings.ToLower(paramValue)
if paramValue == "true" {
config[paramName] = true

View File

@ -3,6 +3,7 @@ package cmd
import (
"encoding/json"
"fmt"
"github.com/SAP/jenkins-library/pkg/log"
"io/ioutil"
"os"
"path/filepath"
@ -131,36 +132,52 @@ func TestGetProjectConfigFile(t *testing.T) {
}
func TestConvertTypes(t *testing.T) {
// Init
options := struct {
Foo []string `json:"foo,omitempty"`
Bar bool `json:"bar,omitempty"`
Baz string `json:"baz,omitempty"`
Bla int `json:"bla,omitempty"`
}{}
t.Run("Converts strings to booleans", func(t *testing.T) {
// Init
options := struct {
Foo bool `json:"foo,omitempty"`
Bar bool `json:"bar,omitempty"`
}{}
options.Foo = true
options.Bar = false
stepConfig := map[string]interface{}{}
stepConfig["baz"] = "ignore"
stepConfig["foo"] = "element"
stepConfig["bar"] = "True"
stepConfig["bla"] = "42"
stepConfig := map[string]interface{}{}
stepConfig["foo"] = "False"
stepConfig["bar"] = "True"
// Test
stepConfig = convertTypes(stepConfig, options)
// Test
stepConfig = checkTypes(stepConfig, options)
confJSON, _ := json.Marshal(stepConfig)
_ = json.Unmarshal(confJSON, &options)
confJSON, _ := json.Marshal(stepConfig)
_ = json.Unmarshal(confJSON, &options)
// Assert
assert.Equal(t, []string{"element"}, stepConfig["foo"])
assert.Equal(t, true, stepConfig["bar"])
// Assert
assert.Equal(t, false, stepConfig["foo"])
assert.Equal(t, true, stepConfig["bar"])
assert.Equal(t, false, options.Foo)
assert.Equal(t, true, options.Bar)
})
t.Run("Exits on unsupported type mismatch", func(t *testing.T) {
// Init
hasFailed := false
assert.Equal(t, []string{"element"}, options.Foo)
assert.Equal(t, true, options.Bar)
exitFunc := log.Entry().Logger.ExitFunc
log.Entry().Logger.ExitFunc = func(int) {
hasFailed = true
}
defer func() { log.Entry().Logger.ExitFunc = exitFunc }()
assert.Equal(t, "ignore", stepConfig["baz"])
assert.Equal(t, "42", stepConfig["bla"])
options := struct {
Foo []string `json:"foo,omitempty"`
}{}
assert.Equal(t, "ignore", options.Baz)
assert.Equal(t, 0, options.Bla)
stepConfig := map[string]interface{}{}
stepConfig["foo"] = "entry"
// Test
stepConfig = checkTypes(stepConfig, options)
// Assert
assert.True(t, hasFailed, "Expected checkTypes() to exit via logging framework")
})
}

View File

@ -4,6 +4,23 @@
## ${docGenParameters}
!!! note "Breaking change in `goals`, `defines` and `flags` parameters"
The `goals`, `defines` and `flags` parameters of the step need to be lists of strings with each element being one item.
As an example consider this diff, showing the old api deleted and the new api inserted:
```diff
-goals: 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
-defines: "-Dexpression=$pomPathExpression -DforceStdout -q",
+goals: ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
+defines: ["-Dexpression=$pomPathExpression", "-DforceStdout", "-q"],
```
Additionally please note that in the parameters _must not_ be [shell quoted/escaped](https://www.tldp.org/LDP/Bash-Beginners-Guide/html/sect_03_03.html).
What you pass in is literally passed to Maven without any shell interpreter inbetween.
The old behavior is still available in version `v1.23.0` and before of project "Piper".
## ${docGenConfiguration}
## ${docJenkinsPluginDependencies}
@ -15,5 +32,5 @@ None
## Example
```groovy
mavenExecute script: this, goals: 'clean install'
mavenExecute script: this, goals: ['clean', 'install']
```

View File

@ -155,6 +155,7 @@ func getParametersFromOptions(options *ExecuteOptions, utils mavenUtils) ([]stri
parameters = append(parameters, "--batch-mode")
parameters = append(parameters, options.Goals...)
return parameters, nil
}

View File

@ -81,7 +81,8 @@ func TestExecute(t *testing.T) {
Flags: []string{"-q"}, LogSuccessfulMavenTransfers: true,
ReturnStdout: false}
expectedParameters := []string{"--global-settings", "anotherSettings.xml", "--settings", "settings.xml",
"-Dmaven.repo.local=.m2/", "--file", "pom.xml", "-q", "-Da=b", "--batch-mode", "flatten", "install"}
"-Dmaven.repo.local=.m2/", "--file", "pom.xml", "-q", "-Da=b", "--batch-mode",
"flatten", "install"}
mavenOutput, _ := Execute(&opts, &execMockRunner)

View File

@ -2,11 +2,13 @@ package com.sap.piper
class BashUtils implements Serializable {
static final long serialVersionUID = 1L
public static final String ESCAPED_SINGLE_QUOTE = "'\"'\"'"
/**
* Put string in single quotes and escape contained single quotes by putting them into a double quoted string
*/
static String quoteAndEscape(String str) {
// put string in single quotes and escape contained single quotes by putting them into a double quoted string
def escapedString = str.replace("'", "'\"'\"'")
def escapedString = str.replace("'", ESCAPED_SINGLE_QUOTE)
return "'${escapedString}'"
}
}

View File

@ -159,8 +159,8 @@ static String evaluateFromMavenPom(Script script, String pomFileName, String pom
String resolvedExpression = script.mavenExecute(
script: script,
pomPath: pomFileName,
goals: 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
defines: "-Dexpression=$pomPathExpression -DforceStdout -q",
goals: ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
defines: ["-Dexpression=$pomPathExpression", "-DforceStdout", "-q"],
returnStdout: true
)
if (resolvedExpression.startsWith('null object or invalid expression')) {

View File

@ -16,6 +16,9 @@ class MavenArtifactVersioning extends ArtifactVersioning {
@Override
def setVersion(version) {
script.mavenExecute script: script, goals: 'org.codehaus.mojo:versions-maven-plugin:2.7:set', defines: "-DnewVersion=${version} -DgenerateBackupPoms=false", pomPath: configuration.filePath
script.mavenExecute script: script,
goals: ['org.codehaus.mojo:versions-maven-plugin:2.7:set'],
defines: ["-DnewVersion=${version}", "-DgenerateBackupPoms=false"],
pomPath: configuration.filePath
}
}

View File

@ -89,14 +89,14 @@ class ArtifactSetVersionTest extends BasePiperTest {
mvnExecuteRule.setReturnValue([
'pomPath': 'pom.xml',
'goals': 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
'defines': '-Dexpression=project.version -DforceStdout -q',
'goals': ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
'defines': ['-Dexpression=project.version', '-DforceStdout', '-q'],
], version)
mvnExecuteRule.setReturnValue([
'pomPath': 'snapshot/pom.xml',
'goals': 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
'defines': '-Dexpression=project.version -DforceStdout -q',
'goals': ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
'defines': ['-Dexpression=project.version', '-DforceStdout', '-q'],
], version)
shellRule.setReturnValue("date --utc +'%Y%m%d%H%M%S'", '20180101010203')
@ -114,8 +114,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
assertEquals(new JenkinsMavenExecuteRule.Execution([
pomPath: 'pom.xml',
goals: 'org.codehaus.mojo:versions-maven-plugin:2.7:set',
defines: '-DnewVersion=1.2.3-20180101010203_testCommitId -DgenerateBackupPoms=false'
goals: ['org.codehaus.mojo:versions-maven-plugin:2.7:set'],
defines: ['-DnewVersion=1.2.3-20180101010203_testCommitId', '-DgenerateBackupPoms=false']
]), mvnExecuteRule.executions[1])
assertThat(shellRule.shell.join(), stringContainsInOrder([
@ -158,8 +158,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
assertEquals(new JenkinsMavenExecuteRule.Execution([
pomPath: 'pom.xml',
goals: 'org.codehaus.mojo:versions-maven-plugin:2.7:set',
defines: '-DnewVersion=1.2.3-20180101010203_testCommitId -DgenerateBackupPoms=false'
goals: ['org.codehaus.mojo:versions-maven-plugin:2.7:set'],
defines: ['-DnewVersion=1.2.3-20180101010203_testCommitId', '-DgenerateBackupPoms=false']
]), mvnExecuteRule.executions[1])
assertThat(((Iterable)shellRule.shell).join(), stringContainsInOrder([
"git add .",
@ -282,8 +282,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
assertEquals('1.2.3-20180101010203_testCommitId', environmentRule.env.getArtifactVersion())
assertEquals(new JenkinsMavenExecuteRule.Execution([
pomPath: 'pom.xml',
goals: 'org.codehaus.mojo:versions-maven-plugin:2.7:set',
defines: '-DnewVersion=1.2.3-20180101010203_testCommitId -DgenerateBackupPoms=false'
goals: ['org.codehaus.mojo:versions-maven-plugin:2.7:set'],
defines: ['-DnewVersion=1.2.3-20180101010203_testCommitId', '-DgenerateBackupPoms=false']
]), mvnExecuteRule.executions[1])
assertThat(shellRule.shell, not(hasItem(containsString('commit'))))
}

View File

@ -40,14 +40,14 @@ class MavenArtifactVersioningTest extends BasePiperTest{
mvnExecuteRule.setReturnValue([
'pomPath': 'pom.xml',
'goals': 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
'defines': '-Dexpression=project.version -DforceStdout -q',
'goals': ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
'defines': ['-Dexpression=project.version', '-DforceStdout', '-q'],
], version)
mvnExecuteRule.setReturnValue([
'pomPath': 'snapshot/pom.xml',
'goals': 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
'defines': '-Dexpression=project.version -DforceStdout -q',
'goals': ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
'defines': ['-Dexpression=project.version', '-DforceStdout', '-q'],
], version)
}
@ -60,13 +60,13 @@ class MavenArtifactVersioningTest extends BasePiperTest{
assertEquals(2, mvnExecuteRule.executions.size())
assertEquals(new JenkinsMavenExecuteRule.Execution([
pomPath: 'pom.xml',
goals: 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
defines: '-Dexpression=project.version -DforceStdout -q'
goals: ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
defines: ['-Dexpression=project.version', '-DforceStdout', '-q']
]), mvnExecuteRule.executions[0])
assertEquals(new JenkinsMavenExecuteRule.Execution([
pomPath: 'pom.xml',
goals: 'org.codehaus.mojo:versions-maven-plugin:2.7:set',
defines: '-DnewVersion=1.2.3-20180101 -DgenerateBackupPoms=false'
goals: ['org.codehaus.mojo:versions-maven-plugin:2.7:set'],
defines: ['-DnewVersion=1.2.3-20180101', '-DgenerateBackupPoms=false']
]), mvnExecuteRule.executions[1])
}
@ -79,13 +79,13 @@ class MavenArtifactVersioningTest extends BasePiperTest{
assertEquals(2, mvnExecuteRule.executions.size())
assertEquals(new JenkinsMavenExecuteRule.Execution([
pomPath: 'snapshot/pom.xml',
goals : 'org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate',
defines: '-Dexpression=project.version -DforceStdout -q'
goals: ['org.apache.maven.plugins:maven-help-plugin:3.1.0:evaluate'],
defines: ['-Dexpression=project.version', '-DforceStdout', '-q']
]), mvnExecuteRule.executions[0])
assertEquals(new JenkinsMavenExecuteRule.Execution([
pomPath: 'snapshot/pom.xml',
goals : 'org.codehaus.mojo:versions-maven-plugin:2.7:set',
defines: '-DnewVersion=1.2.3-20180101 -DgenerateBackupPoms=false'
goals: ['org.codehaus.mojo:versions-maven-plugin:2.7:set'],
defines: ['-DnewVersion=1.2.3-20180101', '-DgenerateBackupPoms=false']
]), mvnExecuteRule.executions[1])
}
}

View File

@ -10,5 +10,6 @@ void call(Map parameters = [:]) {
List credentials = [ ]
final script = checkScript(this, parameters) ?: this
parameters = DownloadCacheUtils.injectDownloadCacheInMavenParameters(script, parameters)
piperExecuteBin(parameters, STEP_NAME, METADATA_FILE, credentials)
}

View File

@ -1,6 +1,6 @@
import com.sap.piper.DownloadCacheUtils
import com.sap.piper.BashUtils
import groovy.transform.Field
import static com.sap.piper.Prerequisites.checkScript
@Field def STEP_NAME = getClass().getName()
@ -10,7 +10,15 @@ def call(Map parameters = [:]) {
final script = checkScript(this, parameters) ?: this
parameters = DownloadCacheUtils.injectDownloadCacheInMavenParameters(script, parameters)
List credentials = [ ]
validateParameter(parameters.defines, 'defines')
validateParameter(parameters.flags, 'flags')
validateParameter(parameters.goals, 'goals')
validateStringParameter(parameters.pomPath)
validateStringParameter(parameters.projectSettingsFile)
validateStringParameter(parameters.globalSettingsFile)
validateStringParameter(parameters.m2Path)
List credentials = []
piperExecuteBin(parameters, STEP_NAME, METADATA_FILE, credentials)
String output = ''
@ -25,3 +33,46 @@ def call(Map parameters = [:]) {
}
return output
}
private void validateParameter(parameter, name) {
if (!parameter) {
return
}
String errorMessage = "Expected parameter ${name} with value ${parameter} to be of type List, but it is ${parameter.class}. "
// Specifically check for string-like types as the old step (v1.23.0 and before) allowed that as input
if (parameter in CharSequence) {
errorMessage += "This is a breaking change for mavenExecute in library version v1.24.0 which allowed strings as input for defines, flags and goals before. " +
"To fix that, please update the interface to pass in lists, or use v1.23.0 which is the last version with the old interface. "
if (parameter.contains(BashUtils.ESCAPED_SINGLE_QUOTE)) {
errorMessage += "It looks like your input contains shell escaped quotes. "
}
error errorMessage + "Note that *no* shell escaping is allowed."
}
if (!parameter in List) {
error errorMessage + "Note that *no* shell escaping is allowed."
}
for (int i = 0; i < parameter.size(); i++) {
String element = parameter[i]
validateStringParameter(element)
}
}
private void validateStringParameter(String element) {
if (!element) {
return
}
if (element =~ /-D.*='.*'/) {
echo "[$STEP_NAME WARNING] It looks like you passed a define in the form -Dmy.key='this is my value' in $element. Please note that the quotes might cause issues. Correct form: -Dmy.key=this is my value"
}
if (element.length() >= 2 && element.startsWith("'") && element.endsWith("'")) {
echo "[$STEP_NAME WARNING] It looks like $element is quoted but it should not be quoted."
}
}

View File

@ -49,6 +49,8 @@ void call(Map parameters = [:], stepName, metadataFile, List credentialInfo, fai
String defaultConfigArgs = getCustomDefaultConfigsArg()
String customConfigArg = getCustomConfigArg(script)
echo "PIPER_parametersJSON: ${groovy.json.JsonOutput.toJson(stepParameters)}"
// get context configuration
Map config = readJSON(text: sh(returnStdout: true, script: "./piper getConfig --contextConfig --stepMetadata '.pipeline/tmp/${metadataFile}'${defaultConfigArgs}${customConfigArg}"))
echo "Context Config: ${config}"