ci: getGitMatchingGroup step #535

v1v · 2020-05-14T15:57:11Z

What does this PR do?

Given the changeset and a regex pattern with group let's see if all the files in the changeset got in common the regex and gather the group match

For instance:

Given the changeset for the PR elastic/beats#17043
Then the module should be zookeeper
with the pattern 'metricbeat\\/module\\/([^\\/]+)\\/.*'
and exclude pattern (.*\\/docs|.*\\.asciidoc)

CHANGELOG.next.asciidoc
metricbeat/docs/modules/zookeeper.asciidoc
metricbeat/docs/modules/zookeeper/connection.asciidoc
metricbeat/docs/modules_list.asciidoc
metricbeat/module/zookeeper/_meta/docs.asciidoc
metricbeat/module/zookeeper/connection/_meta/docs.asciidoc
metricbeat/module/zookeeper/connection/_meta/fields.yml
metricbeat/module/zookeeper/connection/connection.go
metricbeat/module/zookeeper/fields.go
metricbeat/module/zookeeper/mntr/_meta/docs.asciidoc
metricbeat/module/zookeeper/server/_meta/docs.asciidoc

Why is it important?

isGitRegionMatch returns a boolean, so we need soemthing more specific to look for groups in the given regex. Then the pipelines could use it in favor of filtering what stages should or should not run.

Related issues

Caused by https://github.com/elastic/observability-robots/issues/113

Actions

Implement
UTs
ITs
PR on beats to validate if it works as expected :] See [ci] speed builds for module specific changes beats#18592

apmmachine · 2020-05-14T16:03:08Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request ci: getGitMatchingGroup step #535 updated]
Start Time: 2020-05-18T10:41:25.130+0000
Duration: 5 min 16 sec (315529)

Test stats 🧪

Test	Results
Failed	0
Passed	540
Skipped	11
Total	551

v1v · 2020-05-15T17:16:11Z

src/test/resources/jobs/getBeatsModule.dsl

+                                        from: 'b0de59d0ec1e2ae52103a238a2e9cb5b0d7fd9b8',
+                                        to: '641fd600836abafa51def05260d63fab6eed4707',


Changeset from #524

v1v · 2020-05-15T17:16:26Z

src/test/resources/jobs/getBeatsModule.dsl

+                                        from: '347185bd7e2b402ba8f6befa5ef4428ad417fbbc',
+                                        to: '4d9fc25d258622c767ec4d38df38520647cc7dda')


ChangeSet from #516

v1v · 2020-05-15T17:18:35Z

src/test/groovy/GetBeatsModuleStepTests.groovy

+  def realData = '''CHANGELOG.next.asciidoc
+metricbeat/docs/modules/zookeeper.asciidoc
+metricbeat/docs/modules/zookeeper/connection.asciidoc
+metricbeat/docs/modules_list.asciidoc
+metricbeat/module/zookeeper/_meta/docs.asciidoc
+metricbeat/module/zookeeper/connection/_meta/docs.asciidoc
+metricbeat/module/zookeeper/connection/_meta/fields.yml
+metricbeat/module/zookeeper/connection/connection.go
+metricbeat/module/zookeeper/fields.go
+metricbeat/module/zookeeper/mntr/_meta/docs.asciidoc
+metricbeat/module/zookeeper/server/_meta/docs.asciidoc'''.stripMargin().stripIndent()


Changeset from elastic/beats#17043

v1v · 2020-05-15T17:20:04Z

vars/getBeatsModule.groovy

@@ -0,0 +1,80 @@
+// Licensed to Elasticsearch B.V. under one or more contributor


Question:

I think this step can be called something more generic, but I don't have any sensitive naming off the top of my head, any suggestions?

I did not see this comment :D #535 (comment)

kuisathaverat · 2020-05-15T17:22:33Z

I am afraid the number of calls to isGitRegionMatch will be the same, but this will help a lot to decrease the time to build on beats that support MODULE.

kuisathaverat · 2020-05-15T17:30:23Z

src/test/groovy/GetBeatsModuleStepTests.groovy

+  void test_multiple_without_match() throws Exception {
+    def script = loadScript(scriptName)
+    def changeset = '''foo/bar/file.txt
+bar/foo/subfolder'''.stripMargin().stripIndent()
+    helper.registerAllowedMethod('readFile', [String.class], { return changeset })
+    def module = script.call(pattern: '([^\\/]+)\\/.*')
+    assertEquals('', module)
+    assertTrue(assertMethodCallContainsPattern('log', 'getBeatsModule: not found'))
+    assertJobStatusSuccess()
+  }


I think the regexp return 2 matches

as long as they don't have a folder name in common there is no match.

kuisathaverat · 2020-05-15T17:34:52Z

src/test/groovy/GetBeatsModuleStepTests.groovy

+    def module = script.call(pattern: '([^\\/]+)\\/.*', from: 'something')
+    printCallStack()
+    assertEquals('foo', module)
+    assertTrue(assertMethodCallContainsPattern('sh', 'something...bar'))


bar confused me until I realized that it is GIT_BASE_COMIT

kuisathaverat · 2020-05-15T17:39:09Z

vars/getBeatsModule.groovy

+  - When exact match then all the files should match those patterns then it
+    returns the region otherwise and empty string.
+
+  def module = getBeatsModule(pattern: '([^\\/]+)\\/.*')


the step really searches for a pattern in the git diff and return the match group, what about to rename it to getGitDiffMatch or similar.

getGitMatchingGroup?

Done -> 02bb896

ycombinator · 2020-05-15T18:04:29Z

vars/getBeatsModule.groovy

+    if (!isExcluded(line, exclude)) {
+      def matches = line =~ pattern
+      def auxModule = matches.collect { it[1] }[0] ?: ''
+      modules[auxModule] = auxModule


I'm not a regular groovy developer so I may be off here, but shouldn't we only execute this line if auxModule != ''?

That's indeed a good point, I initially coded with d0a1bf6#diff-056e15bb189379e1f510e3402220ddb5R63.

There are three different output in this particular getGroup method:

if no matches then returns ''

if a match for all the change set then returns module_name

if multiple matches then return ''

Therefore '' is a valid output to be returned and that's the reason of no using auxModule != '', in other words:

Given the changeset foo/bar/file.txt
And the pattern ^unknown.txt
the output for getGroup should be ''

For instance, https://github.com/elastic/apm-pipeline-library/pull/535/files#diff-57498ad4c893cd122338bfbdeb7d0aa0R131-R141

Another example that I did not code as an unit test is:

Given the changeset foo/bar/file.txt and bar/foo/file.txt
And the pattern ^unknown.txt
the output for getGroup should be ''

Let me know if what I said it's not clear

Okay, I see why it's working the way it is implemented now.

src/test/resources/jobs/getGitMatchingGroup.dsl

wip ci: getRegionFromPattern step

c5d4f61

v1v self-assigned this May 14, 2020

v1v added the automation label May 14, 2020

botelastic bot added the groovy label May 14, 2020

v1v added 3 commits May 14, 2020 17:10

ci: minor changes

cc52240

Implemented

d0a1bf6

rename

feba9cc

v1v changed the title ~~wip ci: getRegionFromPattern step~~ ci: getBeatsModule step May 15, 2020

v1v and others added 4 commits May 15, 2020 15:35

Merge branch 'master' into feature/getRegionFromPattern

bece6a2

add exclude parameter

427739c

fix: serialisation issue

8972e1b

test: ITs

c2cca9e

botelastic bot added the master label May 15, 2020

v1v added the enhancement New feature or request label May 15, 2020

v1v marked this pull request as ready for review May 15, 2020 17:15

v1v requested review from a team and ycombinator May 15, 2020 17:15

v1v commented May 15, 2020

View reviewed changes

kuisathaverat reviewed May 15, 2020

View reviewed changes

ycombinator reviewed May 15, 2020

View reviewed changes

v1v force-pushed the feature/getRegionFromPattern branch from 58eba4a to 02bb896 Compare May 18, 2020 07:02

v1v requested review from mdelapenya and kuisathaverat May 18, 2020 07:03

v1v changed the title ~~ci: getBeatsModule step~~ ci: getGitMatchingGroup step May 18, 2020

rename

2476123

v1v force-pushed the feature/getRegionFromPattern branch from 02bb896 to 2476123 Compare May 18, 2020 08:08

kuisathaverat approved these changes May 18, 2020

View reviewed changes

v1v commented May 18, 2020

View reviewed changes

src/test/resources/jobs/getGitMatchingGroup.dsl Outdated Show resolved Hide resolved

v1v commented May 18, 2020

View reviewed changes

src/test/resources/jobs/getGitMatchingGroup.dsl Outdated Show resolved Hide resolved

v1v commented May 18, 2020

View reviewed changes

src/test/resources/jobs/getGitMatchingGroup.dsl Outdated Show resolved Hide resolved

v1v commented May 18, 2020

View reviewed changes

src/test/resources/jobs/getGitMatchingGroup.dsl Outdated Show resolved Hide resolved

Apply suggestions from code review

a4824dc

v1v merged commit e5de864 into elastic:master May 18, 2020

v1v removed groovy master labels May 18, 2020

v1v mentioned this pull request Jun 30, 2021

Move openmetrics module to oss elastic/beats#26561

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: getGitMatchingGroup step #535

ci: getGitMatchingGroup step #535

v1v commented May 14, 2020 •

edited

Loading

apmmachine commented May 14, 2020 •

edited

Loading

Build stats

Test stats 🧪

v1v May 15, 2020

v1v May 15, 2020

v1v May 15, 2020

v1v May 15, 2020

kuisathaverat May 15, 2020

kuisathaverat commented May 15, 2020

kuisathaverat May 15, 2020

v1v May 18, 2020

kuisathaverat May 15, 2020

kuisathaverat May 15, 2020

mdelapenya May 15, 2020

v1v May 18, 2020

ycombinator May 15, 2020

v1v May 18, 2020

ycombinator May 18, 2020

		from: 'b0de59d0ec1e2ae52103a238a2e9cb5b0d7fd9b8',
		to: '641fd600836abafa51def05260d63fab6eed4707',

		from: '347185bd7e2b402ba8f6befa5ef4428ad417fbbc',
		to: '4d9fc25d258622c767ec4d38df38520647cc7dda')

		@@ -0,0 +1,80 @@
		// Licensed to Elasticsearch B.V. under one or more contributor

ci: getGitMatchingGroup step #535

ci: getGitMatchingGroup step #535

Conversation

v1v commented May 14, 2020 • edited Loading

What does this PR do?

Why is it important?

Related issues

Actions

apmmachine commented May 14, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuisathaverat commented May 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1v commented May 14, 2020 •

edited

Loading

apmmachine commented May 14, 2020 •

edited

Loading