Repository navigation
Big Pipeline Update - #18
Conversation
Johannes-Schneider
left a comment
Johannes-Schneider
left a comment
There was a problem hiding this comment.
Just from the looks of this PR, it seems like you spent significant manual effort for getting the pipeline and entire maven setup to somehow work...
Additionally, there seems to be a lot of clean up contained in the added modules.
IMO, this is not a good approach.
If we start investing so much clean up / setup effort for just a few modules, merging the rest of the code base in here is going to be a nightmare.
So I'd suggest to stick with the approach we aligned on:
- Remove all the internal modules in our existing repo first
- (We can still update the pipeline as suggested in this PR)
- Once that is done, simply force push everything into this repo
- THEN start cleaning up and fixing things
- (Crossing all fingers that we don't have to migrate changes coming from
v4)
| - name: Compile | ||
| # we need to use install here because otherwise the code checks below don't work | ||
| # they seem to require "mvn dependency:resolve" to work, why is unclear honestly... | ||
| # Problem: With caching the maven repo this might? maybe? produce race conditions / side effects between different executions |
There was a problem hiding this comment.
I'm also worried about this...
As by the documentation:
The format of the used cache key is
setup-java-${{ platform }}-${{ packageManager }}-${{ fileHash }}, where the hash is based on the following files [...]
- maven: **/pom.xml
Therefore, it seems like the cache key would always be the exact same for different execution instances (which is bad) 😔
I'm wondering about the entire setup in general: The path $HOME/.m2/repository doesn't seem to be different for different runs... So we would always share the same .m2.
Therefore, I feel like we might need to come up with our own "caching" strategy to avoid leaking state from one job run into an other one?
There was a problem hiding this comment.
yeah, this is first in the ToDo list to come up with a solution for.
For now it's kinda fine, because the cache doesn't properly work anyway. The cache key is indeed the same, as I could tell from the logs, but the content is still lost between jobs (even in the same workflow). Part of the reason why I had to merge everything into one job...
| print(f"warnings high: {findings['1']}, allowed is {threshold_high}") | ||
|
|
||
| if findings['3'] > threshold_low or findings['2'] > threshold_normal or findings['1'] > threshold_high: | ||
| if findings['1'] > threshold_high: # or findings['2'] > threshold_normal or findings['3'] > threshold_low: |
There was a problem hiding this comment.
I assume you commented the rest of the condition for testing, right?
If so, we should probably revert this before merging.
There was a problem hiding this comment.
no, we have around 80 normal findings which we would all have to fix or increase the threshold otherwise. to me it looked like minor stuff not worth failing over.
| - name: Load Maven Repository | ||
| uses: actions/cache/restore@v3 | ||
| with: | ||
| path: ~/.m2/repository | ||
| key: maven |
There was a problem hiding this comment.
[question]
Is this step going to create the cache (for the very first pipeline run of a new branch, for example)?
And, if so, is it a problem that we are using it inside a matrix (i.e. have multiple executions of it running in parallel)?
Edit:
I saw there is an extra actions/cache/save action, so I assume that this will not automatically create a cache for the first pipeline run 👍
| echo "There are local changes in the following files:" | ||
| echo $CHANGED_FILES | ||
| echo "Printing the git diff:" | ||
| git --no-pager diff |
There was a problem hiding this comment.
Ufff... this might be getting HUGE if we decided to change some service specification (in one of the sample modules, for example), right.
So it this really a good idea? 😅
There was a problem hiding this comment.
sure, but why not? I don't think you will get flamed for too many log output lines that quickly.. and even if we do, worst case the run is aborted, which is fine since it already failed anyway..
|
|
||
| - name: Test | ||
| run: | | ||
| mvn $MVN_CLI_ARGS test jacoco:report |
There was a problem hiding this comment.
This is NOT compiling the code as we restored the (previously built) target dirs, right?
There was a problem hiding this comment.
Actually, when checking the output of the last successful run (at this point in time), I can see that the code is indeed still getting compiled:
(From the Test stage)
[INFO] --- maven-compiler-plugin:3.11.0:compile (default-compile) @ fluent-result ---
[INFO] Changes detected - recompiling the module! :source
[INFO] Compiling 35 source files with javac [debug deprecation release 8] to target/classes
...
[INFO] --- maven-compiler-plugin:3.11.0:testCompile (default-testCompile) @ fluent-result ---
[INFO] Changes detected - recompiling the module! :dependency
[INFO] Compiling 2 source files with javac [debug deprecation release 8] to target/test-classes
There was a problem hiding this comment.
It looks like this can be fixed rather easily by directly invoking the surefire plugin (mvn surefire:test ...) instead of just mvn test - verified manually on my local machine 👍
There was a problem hiding this comment.
uff, good that you checked that. It was working at some point, I swear 😄
My best guess is that it stopped working when I switched to the zip file approach. It can also be fixed by simply touching all files after unzipping, which I added.
However, I now see that man test runs a bunch of other plugins again as well eg. the enforcer plugin. Those take some time and already run during install, so I would go with surefire:test
Remaining question is: Is there any plugin/goal that we don't execute, if we run install -DskipTests and surefire:test compared to install?
I think I'm just gonna compare the logs to see if I can find any diff
There was a problem hiding this comment.
Remaining question is: Is there any plugin/goal that we don't execute, if we run
install -DskipTestsandsurefire:testcompared to install?
Good question.
How about being more precise in the compilation step by actually just skipping the surefire:test phase?
(I saw that there seems to be an option in the plugin config to skip the execution, so that could be done using our regular property approach - but not sure if really achieves the desired result)
There was a problem hiding this comment.
🚨🚨🚨🚨
It looks like the most recent approach (mvn jacoco:prepare-agent surefire:test jacoco:report) produces a different test coverage result compared the "regular" approach (mvn test jcoco:report).
I compared run #168 vs. run #161:
(run 168)
coverage = 0.668060439038297
branch coverage = 0.5957746478873239
vs.
(run 161)
coverage = 0.5919857235561323
branch coverage = 0.5228365384615384
There was a problem hiding this comment.
We are already doing that. The source for the surefire option is:
<parameter>
<name>skipTests</name>
<type>boolean</type>
<since>2.4</since>
<required>false</required>
<editable>true</editable>
<description>Set this to "true" to skip running tests, but still compile them. Its use is NOT RECOMMENDED, but quite convenient on occasion.
Failsafe plugin deprecated the parameter skipTests and the parameter will be removed in Failsafe 3.0.0 as it is a source of conflicts between Failsafe and Surefire plugin.</description>
</parameter>
I was only concerned about other plugins that bind to the test maven lifecycle (and may or may not also use the same property as flag). But I checked and other than the prepare-agent from Jacoco there is nothing running in addition when comparing surefire:test with test that didn't already run during install -DskipTests, so we should be safe.
And the benefit is quite substantial, as we don't unnecessarily re-run enforcer and dependency plugins.
Johannes-Schneider
left a comment
Johannes-Schneider
left a comment
There was a problem hiding this comment.
Just a few more (minor) things. Otherwise LGTM
| - name: Generate JaCoCo Badge | ||
| id: jacoco | ||
| uses: cicirello/jacoco-badge-generator@v2 | ||
| with: | ||
| generate-branches-badge: true | ||
| jacoco-csv-file: "**/jacoco.csv" |
There was a problem hiding this comment.
I'd say let's not generate the JaCoCo Badge from this pipeline (we do that from our regular pipeline, which runs on every main build anyways, right?).
There was a problem hiding this comment.
I'm not sure if the badge is used anywhere. Instead, we use the outputs of the step to determine the coverage %, which we will enforce via thresholds in the future. I would leave it to the ticket to figure out the details on this..
| - name: Load Maven Repository | ||
| uses: actions/cache/restore@v3 | ||
| with: | ||
| path: ~/.m2/repository | ||
| key: maven |
There was a problem hiding this comment.
As we are aiming for a sanity check, let's remove external factors (such as this) to be extra sure everything is indeed working as intended (maybe we messed up something in the caching in our other pipeline and we don't notice because we made the same mistake here?).
Ultimately, since this runs only once a day, performance doesn't really matter here.
| on: | ||
| push: | ||
| branches: ["main", "poc_pipeline"] | ||
| branches: ["main"] |
There was a problem hiding this comment.
[comment - no action needed in this PR]
Do we plan on sticking with our aligned branch naming strategy?
(I.e. rename main to main/v5?)
There was a problem hiding this comment.
I was thinking about this too. But with the current timeline I figured we should leave it out for now and just use main, until there is a need for a v6.
| name: Build, Test and Verify JDK ${{ matrix.Java }} | ||
| name: Build | ||
| env: | ||
| CHANGED_FILES: "" |
There was a problem hiding this comment.
Why is this needed?
This variable should be defined in the Local Changes Check no matter what, right?
Context Information
BLI: SAP/cloud-sdk-java-backlog#121
This PR does a couple of things:
Change the pipeline to be a single job (to enable PMD + Spotbugs and improve performance)modules-bom4.20.0