Skip to content
This repository has been archived by the owner on Oct 28, 2024. It is now read-only.

support exact match in isGitRegionMatch #257

Merged
merged 14 commits into from
Nov 15, 2019
Merged

Conversation

v1v
Copy link
Member

@v1v v1v commented Nov 13, 2019

What does this PR do?

changeset doesn't allow complex match and exact matches.

Potentially it could be added to the jenkins plugin itself but https://issues.jenkins-ci.org/browse/JENKINS-44849 seems to be a blocker in our case, as the very first build won't have any changeset

Extend the current step to support:

  • Regexp and Glob pattern.
  • shouldMatchAll, in other words, all the patterns should match with every entry in the changeset.
  • Breaking change!! regexps param is not anymore supported but patterns is the new one

With this particular feature, we can look for any patterns that match the existing changeset. Or being more accurate to given a list of patterns whether they all match with every entry in the changeset.

Given the below changeset:

folder1/file1.txt
folder2/file2.txt

Then:

  • isGitRegionMatch(patterns: ["^folder**/*"]) -> returns true (GLOB format)
  • isGitRegionMatch(patterns: ["^folder.*"], comparator: 'regex') -> returns true (REGEXP format)
  • isGitRegionMatch(patterns: [".*/file.*", "folder/.*", "foo/bar/*.txt], comparator: 'regex') -> returns true (REGEXP format)
  • isGitRegionMatch(patterns: [".*/file.*", "folder/.*", "foo/bar/*.txt], comparator: 'regex', shouldMatchAll: true) -> returns false (REGEXP format)
  • isGitRegionMatch(patterns: ["**/file*", "folder/**", "foo/bar/*], shouldMatchAll: true) -> returns false (GLOB format)

Why is it important?

More granularity when a certain stage should be launched.

See elastic/apm-server#2891

Related issues

Closes #ISSUE

Test Cases

  • See the UTs as they do cover the above scenarios

ITs

Given the diff v1v/elasticsearch@master...b49fbe0

  • isGitRegionMatch(patterns: ["^.ci/*"])

image

  • isGitRegionMatch(patterns: ["^.foo/*"])
    image

  • isGitRegionMatch(patterns: ["^.ci", "**/Jenkinsfile"])

image

  • isGitRegionMatch(patterns: ["^.ci", "**/Jenkinsfile"], shouldMatchAll: true)

image

  • isGitRegionMatch(patterns: ["^.ci", "**/Jenkinsfile1"], shouldMatchAll: true)

image

  • isGitRegionMatch(patterns: ["^.ci.*", ".*/Jenkinsfile"], comparator: 'regex', shouldMatchAll: true)

image

  • isGitRegionMatch(patterns: ["^.ci.*", ".*/Jenkinsfile.+"], comparator: 'regex', shouldMatchAll: true)

image

@v1v v1v self-assigned this Nov 13, 2019
@v1v v1v added the automation label Nov 13, 2019
@mdelapenya
Copy link
Contributor

mdelapenya commented Nov 13, 2019

After reading the title it was very clear to me the expectation, but it was when I read the implementation when I had doubts about the name of the isExactMatch variable: it led me to think about the regular expression being partially matching the value instead of making me think about the AND/OR clause that it really means.

Is it possible to rename it to make it clear that it will use an AND or an OR to match patterns? I don't like usingOr either, but it expresses the real purpose of the variable.

Wdyt?

Apart from the above comment, LGTM! 👍 So please let's not block it if the name is accurate for everybody

@v1v v1v force-pushed the feature/ext-changeset branch from 60eab99 to cba0167 Compare November 13, 2019 15:54

*/
def call(Map params = [:]) {
if(!isUnix()){
error('isGitRegionMatch: windows is not supported yet.')
}
def regexps = params.containsKey('regexps') ? params.regexps : error('isGitRegionMatch: Missing regexps argument.')
def patterns = params.containsKey('regexps') ? params.regexps : error('isGitRegionMatch: Missing regexps argument.')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to rename regexps as patterns but it's already used I cannot warranty I'll break some compatibility :(

vars/isGitRegionMatch.groovy Outdated Show resolved Hide resolved
}

def isGlob(comparator) {
return comparator.equals('glob')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather compare with the glob match. Although I could potentially use the Comparator enum if required:

Although for this particular use case we could just leave as it is


def isGrepPatternFound(compareWith, pattern) {
log(level: 'DEBUG', text: "isGrepPatternFound: '${compareWith}' with pattern: '${pattern}'")
return sh(script: "echo '${compareWith}' | grep '${pattern}'", returnStatus: true) == 0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the grep for the given line rather than the given file content

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to move this logic with some more groovish grep pattern but so far I don't see an easy way to transform the grep in groovy

}

def isGrepPatternFoundInFile(file, pattern) {
return sh(script: "grep '${pattern}' ${file}", returnStatus: true) == 0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the grep for the given file

@v1v v1v requested a review from ElWPenn November 13, 2019 20:04
@v1v v1v marked this pull request as ready for review November 13, 2019 20:04
@v1v v1v changed the title WiP: support exact match in isGitRegionMatch support exact match in isGitRegionMatch Nov 14, 2019
@v1v v1v added the enhancement New feature or request label Nov 14, 2019
@v1v
Copy link
Member Author

v1v commented Nov 14, 2019

I need to figure out what's going on with a warning, as it does behave as expected but that particular expected to call log output might to be important to be reviewed

[Pipeline] readFile
expected to call java.lang.String.eachLine but wound up catching org.jenkinsci.plugins.workflow.cps.CpsClosure2.call; see: https://jenkins.io/redirect/pipeline-cps-method-mismatches/

@kuisathaverat
Copy link
Contributor

I am using it on elastic/beats#14484, so merge, merge, merge 😃

@v1v
Copy link
Member Author

v1v commented Nov 15, 2019

#257 (comment) was indeed a genuine failure as readFile does return a String, therefore eachLine is incorrect.

ITs

Given v1v/elasticsearch@master...ae8a6f8

  • isGitRegionMatch(patterns: ["^.ci.*", ".*/Jenkinsfile.*"], comparator: 'regex', shouldMatchAll: true)

image

  • isGitRegionMatch(patterns: ["^.ci.*", ".*/Jenkinsfile.+"], comparator: 'regex', shouldMatchAll: true)

image

  • isGitRegionMatch(patterns: ["^.ci.*", ".*/Jenkinsfile.+"], comparator: 'regex', shouldMatchAll: false)

image

  • isGitRegionMatch(patterns: ["^.ci/**"], shouldMatchAll: false)

image

@v1v v1v merged commit 89b2bd2 into elastic:master Nov 15, 2019
@v1v v1v deleted the feature/ext-changeset branch November 15, 2019 10:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants