Move lint checks to a separate job#984
Conversation
.github/workflows/main.yml
Outdated
| - name: Check out Repository | ||
| uses: actions/checkout@v3.3.0 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Can you add a comment why we're checking out with zero fetch-depth?
There was a problem hiding this comment.
It took some digging to figure out the reason why (our above comment just says to improve reporting, but I wasn't sure what that meant). I found out it's about sonar scanning, briefly documented here:
https://github.com/SonarSource/sonarcloud-github-action
And detailed reasons here:
https://community.sonarsource.com/t/git-fetch-depth-implications/75260
We don't run sonar scans in the lint checker (sonar scan uses coverage reports so needs to be in the main CI job), so I've removed the fetch-depth: 0 here. I've also updated the above comment to mention sonar scan as the reason for it.
| run: $SBT osgiCheck | ||
|
|
||
| - name: Run scalafmt Check | ||
| if: success() || failure() |
There was a problem hiding this comment.
Nice use of success() || failure() instead of always(). If a user or concurrency: group cancels the run, steps with always() still would be run but steps with success() || failure() will be skipped.
There was a problem hiding this comment.
Yeah, using always() feels like the obvious choice, but the success() || failure() idiom is recommended in the GitHub Actions documentation for the reason you mention:
https://docs.github.com/en/actions/learn-github-actions/expressions#always
Seems like always should be used very rarely in favor of this success() || failure() idiom.
This allows lint checks and tests to run separately, making is easier to distinguish between failed tests or just minor cleanup issues. Should also allow the CI tests to finish 1-2 minutes quicker on average. All lint checks are run even if other lint checks fail, which should help to find all issues in a single CI run. DAFFODIL-2799
stevedlawrence
force-pushed
the
daffodil-2799-lint-checks-ci
branch
from
10fc85a to
5b91124
Compare
3 participants
This allows lint checks and tests to run separately, making is easier to distinguish between failed tests or just minor cleanup issues. Should also allow the CI tests to finish 1-2 minutes quicker on average.
All lint checks are run even if other lint checks fail, which should help to find all issues in a single CI run.
DAFFODIL-2799