Static analysis workflow checks not working

[nihaals]

Example of skipped checks

Checks

thunderstore-ui/.github/workflows/static-analysis.yml

Lines 16 to 17 in 1ecfb85

	name: ESLint
	if: github.event_name != 'pull_request' || (github.event_name == 'pull_request' && github.event.head.repo.fork)

thunderstore-ui/.github/workflows/static-analysis.yml

Lines 36 to 37 in 1ecfb85

	name: CodeQL
	if: github.event_name != 'pull_request' || (github.event_name == 'pull_request' && github.event.head.repo.fork && github.base_ref == 'master')

[with-heart]

I'm not entirely sure I'm following this workflow, likely due to my unfamiliarity with CodeQL. What's the goal?

[nihaals]

The actions/jobs themselves aren't too important (although knowing about code scanning is probably useful).

This workflow includes all the static analysis tools, mainly ones that output SARIF files that are uploaded to GitHub's code scanning (although I did try sourcegraph's LSIF but it was taking more work than it seemed to be worth).

thunderstore-ui/.github/workflows/static-analysis.yml

Lines 36 to 37 in 1ecfb85

	name: CodeQL
	if: github.event_name != 'pull_request' || (github.event_name == 'pull_request' && github.event.head.repo.fork && github.base_ref == 'master')

The CodeQL job essentially finds common web exploits and sends them to the code scanning tab. This isn't public but I'm not sure on the exact permission it's locked behind (I can't change repo settings but I still have access to it). My intention with the CodeQL check is "If it's a commit to this repo (not a fork), scan it. If not (therefore it must be a PR), make sure it's a PR where the source is a fork (prevents double scanning the same commit since otherwise commits to this repo would be scanned from both push and pull_request) and the target branch is master".

Scanning commits in this repo makes sense as they'll all eventually become a PR to master, so scanning them earlier with push just makes sense. Scanning PRs is slightly more complex though, but if we're merging a PR, it would be nice to know about issues before it's merged to master, not after. Filtering it to master is just because that's really the only important branch that needs this (a PR to another branch shouldn't impact prod so it's fine to scan it after it's merged). I think CodeQL will still run in the PR author's fork, it just won't show on ours.

thunderstore-ui/.github/workflows/static-analysis.yml

Lines 16 to 17 in 1ecfb85

	name: ESLint
	if: github.event_name != 'pull_request' || (github.event_name == 'pull_request' && github.event.head.repo.fork)

The ESLint job runs ESLint and sends the errors to code scanning. This gives something like:

While this is actually an example in the docs (suggesting code scanning is really for any static analysis tools), it being in the security tab and not being public makes it a bit weird. I think we should actually remove this in favour of running ESLint in standard CI, but maybe have in line annotations (example with codecov). I'm not sure if there is already a nice way of doing this but not just having a long list of errors but instead having it in the diff view is a nice way to actually see the issues. As ESLint will be mandatory (related: #33), this isn't a must, just a nice goal in the future if it's not easily doable.