Make CI only build/test modules with any changes #10323

nineinchnick · 2021-12-16T11:29:07Z

The gitflow-incremental-builder (GIB) Maven extension allows to only build modules that contain any changes. It compares the current git branch with the merge base and selects modules with any modified files.

I enable buildUpstream to make sure all (upstream) dependencies are built, but without running tests. I also enable buildDownstream to build and test all dependant (downstream) modules. If there are changes in the root module, everything would be built.

For example, when there would only be changes in the plugin/trino-hive-hadoop2 module, only the following would be built:

[INFO] Reactor Build Order:
[INFO] 
[INFO] trino-root                                                         [pom]
[INFO] trino-testing-services                                             [jar]
[INFO] trino-spi                                                          [jar]
[INFO] trino-client                                                       [jar]
[INFO] trino-parser                                                       [jar]
[INFO] trino-matching                                                     [jar]
[INFO] trino-plugin-toolkit                                               [jar]
[INFO] trino-array                                                        [jar]
[INFO] trino-geospatial-toolkit                                           [jar]
[INFO] trino-memory-context                                               [jar]
[INFO] trino-tpch                                                [trino-plugin]
[INFO] trino-main                                                         [jar]
[INFO] trino-testing                                                      [jar]
[INFO] trino-orc                                                          [jar]
[INFO] trino-memory                                              [trino-plugin]
[INFO] trino-benchmark                                                    [jar]
[INFO] trino-parquet                                                      [jar]
[INFO] trino-rcfile                                                       [jar]
[INFO] trino-hive                                                         [jar]
[INFO] trino-hive-hadoop2                                        [trino-plugin]
[INFO] trino-jdbc                                                         [jar]
[INFO] trino-proxy                                                        [jar]
[INFO] trino-verifier                                                     [jar]
[INFO] trino-benchto-benchmarks                                           [jar]
[INFO] trino-product-tests                                                [jar]
[INFO] trino-product-tests-launcher                                       [jar]
[INFO] trino-test-jdbc-compatibility-old-driver                           [jar]
[INFO] trino-test-jdbc-compatibility-old-server                           [jar]

where modules like trino-jdbc and trino-test-jdbc-compatibility-old-server and dependants, so their tests would be run.

Note this is only enabled in the ci profile and I'm adding this profile to most jobs. This does NOT affect Product Tests, which would always run with full coverage.

This would probably make the separate docs workflow obsolete.

This is a draft because I'll test different combinations of changes to be absolutely sure it won't reduce coverage.

martint · 2021-12-17T02:27:06Z

Note this is only enabled in the ci profile and I'm adding this profile to most jobs. This does NOT affect Product Tests, which would always run with full coverage.

At some point we're going to have to split product tests into modules so that only the relevant product tests run. Currently, product tests are monolithic -- if something changes in the cassandra connector, all product tests for hive, phoenix, etc will run.

plugin/trino-hive-hadoop2/bin/run_hive_abfs_access_key_tests.sh

kokosing · 2021-12-17T09:22:02Z

At some point we're going to have to split product tests into modules so that only the relevant product tests run.

I am not sure how that would look like. Can you please elaborate @martint ?

I was thinking about different approach. Currently all product tests environments configuration is known and stored in files, we could analyze that files and extract a list of features given environment is using. Like when you find a properties file with connector.name=hive then you know that it is testing hive. Then if you know what modules have changed, you can only run product tests that are using environments which are using such modules.

martint · 2021-12-17T19:57:39Z

I am not sure how that would look like. Can you please elaborate @martint ?

By making product tests a framework for building product tests (i.e., the test definitions, runners, etc. might go there), and then splitting them into separate modules, one for each connector (the specific instantiations of tests for each connector would go here).

martint

[INFO] trino-benchmark [jar]
[INFO] trino-proxy [jar]
[INFO] trino-verifier [jar]

In the example above, why are those two modules being built? None of those are downstream or upstream dependencies of trino-hive-hadoop2

.github/workflows/ci.yml

pom.xml

nineinchnick · 2021-12-18T14:33:47Z

[INFO] trino-benchmark [jar]
[INFO] trino-proxy [jar]
[INFO] trino-verifier [jar]

In the example above, why are those two modules being built? None of those are downstream or upstream dependencies of trino-hive-hadoop2

I believe this is because trino-verifier requires trino-jdbc and the latter uses trino-hive-hadoop2 for tests. While including direct dependents with scope test is ok, transitive dependents should only be included for direct dependents with scope compile. But we should discuss this with maintainers of GIB, I think it's out of the scope of this PR. I reported two bugs so far and they've been very responsive and friendly.

MiguelWeezardo · 2022-02-17T11:01:04Z

@electrum, @martint Would it be possible to take another look, and merge it if everything is good?

hashhar

Some nits and some comments. Fine to merge as-is but let's ensure that we at-least track the actionable items in a follow-up issue.

Looks good to go otherwise.

.github/bin/build-matrix-from-impacted.py

hashhar · 2022-02-25T19:45:26Z

.github/bin/build-matrix-from-impacted.py

+    build(args.matrix, args.impacted, args.output)
+
+
+def build(matrix_file, impacted_file, output_file):


bump, let's create follow-up issues to track

.github/bin/build-matrix-from-impacted.py

pom.xml

hashhar · 2022-02-25T20:10:04Z

pom.xml

+                            <buildUpstreamMode>impacted</buildUpstreamMode>
+                            <!-- skip tests and checks for upstream modules since they have not been modified but are still required to be built -->
+                            <skipTestsForUpstreamModules>true</skipTestsForUpstreamModules>
+                            <argsForUpstreamModules>-Dmaven.source.skip=true -Dair.check.skip-all</argsForUpstreamModules>


skip javadocs too?

Also gib.skipIfPathMatches might be helpful to ensure that change in some files (e.g. .github/workflows?) causes gib to consider all modules as impacted.

I didn't add it because of these comments: #10323 (comment)

From the gib docs:

This property is helpful for more complex project setups in which not all submodules depend on the root module (= are not direct or indirect children of the root module).

I believe we don't have this kind of modules, right?

No we don't have such modules as far as I can see (though nothing prevents such a module being added in future).
However in current impl we end up running GIB when github/workflows/ci.yml changes for example. And GIB says that no modules changed. We'd want the reverse behaviour for safety IMO.

It should treat this file as part of the root module and build everything.

.github/bin/git-fetch-base-ref.sh

pom.xml

The gitflow-incremental-builder (GIB) Maven extension allows to only build modules that contain any changes. It compares the current git branch with the merge base and selects modules with any modified files. All (upstream) dependencies of impacted modules are built, but without running tests. All dependant (downstream) modules of impacted modules are also build and tested. If there are changes in the root module, everything would be built. Note this is only enabled in the ci profile. This does not affect product tests, which would always run with full coverage.

hashhar

Thank you.

plugin/trino-hive-hadoop2/bin/common.sh

hashhar

LGTM from me.

I don't see any remaining unresolved comments so I'll merge once CI is done.
We can keep iterating as needed.

hashhar · 2022-02-28T13:06:32Z

Thanks a lot @nineinchnick (and others who assisted and reviewed).

Let's keep an eye open for any cases where we feel tests are skipped which shouldn't be and we can iterate.

nineinchnick · 2022-02-28T16:24:08Z

I messed up, the build on master failed with:

usage: build-matrix-from-impacted.py [-h] [-m MATRIX] [-i IMPACTED]
                                     [-o OUTPUT] [-v]
build-matrix-from-impacted.py: error: argument -i/--impacted: can't open 'gib-impacted.log': [Errno 2] No such file or directory: 'gib-impacted.log'
Error: Process completed with exit code 2.

I'll open up a PR to fix this ASAP

cla-bot bot added the cla-signed label Dec 16, 2021

nineinchnick marked this pull request as draft December 16, 2021 12:19

nineinchnick force-pushed the gib branch from e9aa7c2 to a165b9a Compare December 17, 2021 08:04

MiguelWeezardo reviewed Dec 17, 2021

View reviewed changes

plugin/trino-hive-hadoop2/bin/run_hive_abfs_access_key_tests.sh Outdated Show resolved Hide resolved

hashhar added the WIP label Dec 17, 2021

nineinchnick force-pushed the gib branch from b6e504e to 2f78f9c Compare December 17, 2021 11:07

nineinchnick mentioned this pull request Dec 17, 2021

NPE when merge base is not found gitflow-incremental-builder/gitflow-incremental-builder#451

Closed

martint reviewed Dec 18, 2021

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

nineinchnick force-pushed the gib branch from 249d7e0 to f0f8d04 Compare December 18, 2021 14:25

nineinchnick force-pushed the gib branch 2 times, most recently from 0c94a39 to 642facf Compare December 18, 2021 15:00

nineinchnick mentioned this pull request Dec 18, 2021

Test modules built with changes in hive-hadoop2 nineinchnick/trino#1

Closed

nineinchnick force-pushed the gib branch 5 times, most recently from f90518b to bfec977 Compare December 18, 2021 17:51

nineinchnick mentioned this pull request Dec 18, 2021

Test modules built with changes in trino-benchmark nineinchnick/trino#2

Closed

nineinchnick force-pushed the gib branch 2 times, most recently from 948cdd1 to 9a0c55b Compare December 18, 2021 19:57

nineinchnick mentioned this pull request Dec 18, 2021

Test modules built with changes in trino-postgresql nineinchnick/trino#3

Closed

nineinchnick force-pushed the gib branch 5 times, most recently from 1b17879 to f9a6f33 Compare December 19, 2021 10:29

nineinchnick force-pushed the gib branch from 0ddfbbe to b81d21e Compare February 14, 2022 08:12

MiguelWeezardo force-pushed the gib branch 3 times, most recently from 98a7ee7 to d3cf3ad Compare February 25, 2022 13:12

nineinchnick force-pushed the gib branch from d3cf3ad to 796f898 Compare February 25, 2022 13:57

hashhar reviewed Feb 25, 2022

View reviewed changes

nineinchnick added 2 commits February 28, 2022 09:56

Pass Maven test flags from CI to Hive tests

1d81b4f

nineinchnick force-pushed the gib branch from 796f898 to 0d0d409 Compare February 28, 2022 08:56

nineinchnick requested a review from hashhar February 28, 2022 09:03

nineinchnick mentioned this pull request Feb 28, 2022

Add Delta Lake connector #10897

Merged

hashhar approved these changes Feb 28, 2022

View reviewed changes

plugin/trino-hive-hadoop2/bin/common.sh Outdated Show resolved Hide resolved

nineinchnick added 7 commits February 28, 2022 10:22

Skip running Hive tests early if module is not impacted

b5433e4

Skip running JDBC compatibility tests early if module is not impacted

c56f778

Move memsql and bigquery CI jobs into matrix

9d5784d

Break up module test matrix into separate fields

70130f3

Build CI test matrix using impacted modules list

fb2e708

Avoid blocks for single run commands in the CI workflow

5f27804

Don't build whole project when building CI test matrix

f6ace6f

nineinchnick force-pushed the gib branch from 0d0d409 to f6ace6f Compare February 28, 2022 09:22

hashhar approved these changes Feb 28, 2022

View reviewed changes

hashhar merged commit bd0de0a into trinodb:master Feb 28, 2022

github-actions bot added this to the 372 milestone Feb 28, 2022

mosabua mentioned this pull request Feb 28, 2022

Add Trino 372 release notes #11094

Merged

nineinchnick mentioned this pull request Feb 28, 2022

Don't exclude anything in test matrix if impacted modules list is empty #11228

Merged

hashhar mentioned this pull request Mar 4, 2022

Selective tests execution #1988

Closed

nineinchnick mentioned this pull request Mar 13, 2022

Make matrix script process any matrix items #11446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CI only build/test modules with any changes #10323

Make CI only build/test modules with any changes #10323

nineinchnick commented Dec 16, 2021

martint commented Dec 17, 2021

kokosing commented Dec 17, 2021

martint commented Dec 17, 2021

martint left a comment

nineinchnick commented Dec 18, 2021

MiguelWeezardo commented Feb 17, 2022

hashhar left a comment •

edited

Loading

hashhar Feb 25, 2022

hashhar Feb 25, 2022

hashhar Feb 25, 2022

nineinchnick Feb 28, 2022 •

edited

Loading

hashhar Feb 28, 2022 •

edited

Loading

nineinchnick Feb 28, 2022

hashhar left a comment

hashhar left a comment

hashhar commented Feb 28, 2022

nineinchnick commented Feb 28, 2022

		build(args.matrix, args.impacted, args.output)


		def build(matrix_file, impacted_file, output_file):

Make CI only build/test modules with any changes #10323

Make CI only build/test modules with any changes #10323

Conversation

nineinchnick commented Dec 16, 2021

martint commented Dec 17, 2021

kokosing commented Dec 17, 2021

martint commented Dec 17, 2021

martint left a comment

Choose a reason for hiding this comment

nineinchnick commented Dec 18, 2021

MiguelWeezardo commented Feb 17, 2022

hashhar left a comment • edited Loading

Choose a reason for hiding this comment

hashhar Feb 25, 2022

Choose a reason for hiding this comment

hashhar Feb 25, 2022

Choose a reason for hiding this comment

hashhar Feb 25, 2022

Choose a reason for hiding this comment

nineinchnick Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

hashhar Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

nineinchnick Feb 28, 2022

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar commented Feb 28, 2022

nineinchnick commented Feb 28, 2022

hashhar left a comment •

edited

Loading

nineinchnick Feb 28, 2022 •

edited

Loading

hashhar Feb 28, 2022 •

edited

Loading