Add spanner module build override based on changeset for cicd #1545
Conversation
|
Can we give this a day or two to understand why this was disabled in the first place (@Polber)? I'm hoping we can just make a simple fix so we don't need this change |
|
@damccorm We used to build only affected modules, but this led to frequent race conditions where not all dependencies would get built - instead of adding custom logic each time this happened (which seemed to be increasing in frequency each passing week), we decided to just build all modules on each PR. Perhaps there is a "better" fix (maybe building all common modules, but only running IT's on affected modules) but I don't think it is necessarily a simple fix |
cicd/internal/op/maven.go
Outdated
| return RunMavenOnPom(pom, cmd, args...) | ||
| func RunMavenOnModule(pom string, cmd string, module []string, args ...string) error { | ||
| fullArgs := args | ||
| for _, spanner_module := range SPANNER_MODULES { |
There was a problem hiding this comment.
I don't really like this change as it stands because it has the potential to filter out real failures in non-spanner templates. For example, imagine I make a change in 2 templates - one spanner, one non-spanner (this is common in large-scale full-repo changes, or when doing something like regenerating docs as part of a PR) - the tests will not run on the non-spanner template. We also don't seem to have a clear path to fix the underlying problem, so this hack will remain in place indefinitely.
I'd like to propose a different path:
- Get rid of the whole concept of only building changed files (basically greatly simplify )
- Instead, allow individual workflows to pass in a set of modules to run against, such that you can trigger tests with
./cicd/run-unit-tests --changed-modules="v2/datastream-to-spanner,v2/sourcedb-to-spanner,v2/spanner-change-streams-to-sharded-file-sink". If helpful, this could probably even accept aliases (e.g.--changed-modules="spanner_all") - Set up a spanner-only GitHub Actions workflow which passes in these modules and only triggers on spanner changes.
- Exclude spanner from
Thoughts?
There was a problem hiding this comment.
Makes sense, I have made a number of changes on this thought process and still in the process of testing.
There was a problem hiding this comment.
Having some issues in getting the build to work in isolation -
The following command is generated -
mvn -B clean install -f pom.xml -e -Dmdep.analyze.skip -Djib.skip -DskipShade -Dspotless.check.skip -Dmaven.test.skip -T3 -pl=v2/datastream-to-spanner/,v2/spanner-common/,v2/spanner-change-streams-to-sharded-file-sink/,v2/gcs-to-sourcedb/,v2/spanner-migrations-sdk/,v2/spanner-custom-shard/,v2/sourcedb-to-spanner/
This works locally, however fails on Github actions with -
https://paste.googleplex.com/6486993152507904
I tried adding the common module as a dependency to datastream-to-spanner but that does not seem to help.
Do you have any idea on how to solve this?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1545 +/- ##
============================================
+ Coverage 40.91% 40.92% +0.01%
- Complexity 2808 2811 +3
============================================
Files 738 738
Lines 42796 42796
Branches 4577 4577
============================================
+ Hits 17509 17514 +5
+ Misses 23788 23784 -4
+ Partials 1499 1498 -1
|
|
Closing in favour of #1573 |
This is a hack to only enable module only builds for spanner modules.
Currently all modules get built which cause significant increase in the time for build and IT workflows to complete.