LIME-2030: Setting up GHAs#21
Conversation
a6f04d9 to
6489316
Compare
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
42dbd5c to
5755291
Compare
| [[ "${{ needs.type-check.result }}" != "success" ]] && failed+=("type-check") | ||
| [[ "${{ needs.unit-tests.result }}" != "success" ]] && failed+=("unit-tests") | ||
|
|
||
| [[ "${{ needs.sonar.result }}" != "success" ]] && failed+=("sonar") |
There was a problem hiding this comment.
sonar is skipped for dependabot so this job will fail for PRs raised by it if you require a sonar success result here
There was a problem hiding this comment.
What was the reasoning behind skipping sonar? I think it would be fine to run / be required
There was a problem hiding this comment.
the main reason is bots don't have access to the sonar secret so the job always fails
There was a problem hiding this comment.
I see it running in other dependabot PRs as a required check - so not sure how that is working - e.g. here.
Or is it that we don't want to give access?
Sorry - I think potentially while its required and appears to have run/passed - since the coverage step was skipped it actually didn't.
There was a problem hiding this comment.
Although... I think it just runs without unit test coverage - there's an example of a dependabot report here.
I'll do some more experiments but not sure what approach we'd prefer if this is the case.
| # runs for ts and js by default. | ||
|
|
||
| sonar: | ||
| name: Sonar scan |
There was a problem hiding this comment.
worth checking that running the sonar scan without the coverage report on a cron won't mess with our coverage metrics on sonarcloud
There was a problem hiding this comment.
Think we will have to wait and see on this as there's not enough lines of code yet to generate a proper report. I don't expect it would since there's scenarios across the programme where unit test coverage isn't reported, but can come back to if there's any issues.
| needs: type-check | ||
| # actions: read | ||
| steps: | ||
| - name: Run Sonar scan |
There was a problem hiding this comment.
suggest moving this into a composite action (similar to the frontend) so that when we update the sonar action in the future it only needs to be done in one place
There was a problem hiding this comment.
I've been playing around with doing something like that, but more similar to how other repos do it (e.g. check-hmrc).
It adds complexity in different ways though so not sure its worth it...
Also splitting that action out seems pretty repetitive / similar to the github-actions version. I don't see much difference other than updating the SHA and would compare it to putting a wrapper on something like checkout or assume aws creds, just so we don't have to update the version in a few places.
Will look into the other point about the metrics report above though - its a good point.
8a7924a to
46c4163
Compare
46c4163 to
3fee638
Compare
befcfc7 to
04b0966
Compare
5aefa91 to
3e735fb
Compare
7bb8372 to
9f1ab61
Compare
9f1ab61 to
39ed489
Compare
39ed489 to
b5c7b37
Compare
5a6d977 to
96db2d0
Compare
nlsteers
left a comment
There was a problem hiding this comment.
looking really nice, couple of qs from me
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| sonar-token: ${{ secrets.SONAR_TOKEN }} | ||
| coverage-artifact: null |
There was a problem hiding this comment.
can this just be omitted rather than passing null here?
There was a problem hiding this comment.
Unfortunately not without changing the shared action - annoyingly it checks if its null, but it has a default value so omitting != null.
https://github.com/govuk-one-login/github-actions/blob/main/code-quality/sonarcloud/action.yml#L51
| run: echo "sha-short=${GITHUB_SHA::7}" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Build app | ||
| uses: govuk-one-login/github-actions/sam/build-application@350c58b28e56f0fd67c4a8d8c8cd4fdc6355bdf3 # my branch - change once merged |
There was a problem hiding this comment.
is this still waiting on a merge?
There was a problem hiding this comment.
This is because I want to merge this github-actions PR so we can ref the new commit sha from main.
I worry if we merge here before, it might ref a non-existant sha once the other PR is merged and the branch deleted.
| [[ "${{ needs.type-check.result }}" != "success" ]] && failed+=("type-check") | ||
| [[ "${{ needs.unit-tests.result }}" != "success" ]] && failed+=("unit-tests") | ||
|
|
||
| [[ "${{ needs.codeql.result }}" != "success" ]] && failed+=("codeql") |
There was a problem hiding this comment.
are you just checking that the codeql job ran here? it doesn't report a result directly so this is currently just a check to see if the job ran at all
There was a problem hiding this comment.
Hm yea I didn't think much about what this was doing - I think yes just checking it ran. I think it could be removed - this would then open up the possibility to rework scan-repo to be re-useable in pr-check and post-merge if they're both not required.
But I think I will leave that as a future improvement since this has taken long enough 😆
| uses: ./.github/actions/node-setup | ||
| - name: Type check | ||
| run: npm run type-check | ||
| uses: govuk-one-login/github-actions/node/install-dependencies@350c58b28e56f0fd67c4a8d8c8cd4fdc6355bdf3 # my branch - change once merged |
There was a problem hiding this comment.
useful, we should backport this into the frontend actions when your branch is merged
| - id: eslint | ||
| name: ESLint | ||
| entry: npm run lint | ||
| entry: npm run lint:pre-commit |
There was a problem hiding this comment.
what's the point of adding this over the existing 'lint' script?
There was a problem hiding this comment.
On pre-commit un-staged files were being ran against, which is not really the intended functionality for pre-commit hooks.
| @@ -0,0 +1,3 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| echo hello test word | |||
There was a problem hiding this comment.
is this supposed to be 'world'? nbd as these are temp
96db2d0 to
296cf61
Compare
2b30c72 to
500c672
Compare
500c672 to
ef0c3c4
Compare
|



Proposed changes
Issue tracking
What changed
PR check
workflow_dispatchorworkflow_call.workflow_dispatch.Cleanup
Scan repo
Other things
Other considerations
Workflows that are manually trigger-able (
workdlow_dispatch) have been tested by temporarily triggering them on PR. These need to be retested viaworkflow_dispatchonce merged into main.Post-merge workflow may cause the same cosign/rektor issue seen previously - will know on merge. If so, we can look into putting in the proper devplatform upload action fix (and bypass uploads to rektor entirely). May have to raise though which could take some time.
Future improvement if codeql and sonar are not required on PR check (or if both could be required): rework scan-repo to take an optional coverage artefact input, so we can call this workflow instead of the actions individually in pr-check and post-merge.
Outside of this repo, theres been a few changes to github-actions and the devplatform-deploy secure pipeline scripts/permissions, to allow for using new SAR/serverless applications.
Other than this, some work involved changes in identity-common-infra for pre-merge/preview stacks, and applying terraform for example to add test steps to the pipelines.