-
Notifications
You must be signed in to change notification settings - Fork 2
[FOLIO-4126] Extract MD logic, minor other improvements #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: FOLIO-4126-maven-workflows-1
Are you sure you want to change the base?
Conversation
| run: if ! ${{ inputs.allow-snapshots-release }} && [ -s snapshots.txt ]; then exit 1; fi | ||
| - name: (Release only) Report release dependencies that are snapshots | ||
| if: ${{ inputs.is-release == 'True' && inputs.allow-snapshots-release == 'True' }} | ||
| run: if [ -s snapshots.txt ]; then exit 1; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could almost certainly also be updated into a check of hashFiles, but I'm not sure which would be clearer
| - name: Create default .dockerignore file | ||
| if: ${{ hashFiles('.dockerignore') == '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the bonus of GA showing the step 'skipped' iff the file already exists
| echo "loop-count=$i (max_startup_wait=$max_startup_wait)" | ||
| - name: Build and push Docker image | ||
| - name: Build ${{ inputs.do-docker-push == 'True' && 'and push ' || '' }}Docker image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd dynamically rename the job itself, too, but I'm wary of potentially breaking branch protection rules. It should be fine since do-docker-push=true only when we're on main or we're on a release tag, so it'd always be simply Docker build on PRs; thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand the benefit of dynamically renaming the job. Feels like too much magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent the confusion of seeing a step named "docker build and publish" having ran in a PR run, even though no publishing occurred
…to FOLIO-4126-noah
|
@dcrossleyau now that #97 is merged, I went ahead and synced this PR with the base branch to make it a little less noisy |
This PR does the following:
output;--update-snapshotsto the main "build" step, to ensure that the latest snapshots are always used (performance penalty of <1s with multiple snapshot dependencies in my testing);Moved sonar logic frombuildto a separate job; andThe two changes marked with * are done to improve parallelization, to decrease the time between CI starting and docker/publication completing. They also significantly decrease the number of parameters flying around. Here's what was extracted and why:
- Sonar does not need to block Docker/etc steps from completing. It's also pretty slow (often a minute-plus), so having it block everything is quite a pain. - Sonar relies on the compiled `class` files and Jacoco results, and for some repositories, `generated-sources` and friendsalready mergedjqmagic) requiresartifact-version.version-numberstep (which takes ~15s to resolve for some reason?), and is the only thing which directly relies on it. By moving this logic from the mainbuildstep,version-numberandbuildcan run concurrently.build-artifactswhich istarget/*(excluding API docs + jars). This is useful for debugging in general, but allows downstream steps access to all of this without needing to re-run maven. In testing this was ~2MB across a few repos, so it's quite minimal.It got a little more spaghetti, but in total this shaved about 60-90 seconds off:
Original:

New:
