search

thunderstore-io / thunderstore-ui

Thunderstore React UI
10 stars 5 forks source link

Static analysis workflow checks not working #36

Open nihaals opened 3 years ago

nihaals commented 3 years ago

Example of skipped checks

Checks

https://github.com/thunderstore-io/thunderstore-ui/blob/1ecfb85116552bbf5882380c58666b726dd0d506/.github/workflows/static-analysis.yml#L16-L17

https://github.com/thunderstore-io/thunderstore-ui/blob/1ecfb85116552bbf5882380c58666b726dd0d506/.github/workflows/static-analysis.yml#L36-L37

with-heart commented 3 years ago

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

nihaals commented 3 years ago

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).

https://github.com/thunderstore-io/thunderstore-ui/blob/1ecfb85116552bbf5882380c58666b726dd0d506/.github/workflows/static-analysis.yml#L36-L37

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.

https://github.com/thunderstore-io/thunderstore-ui/blob/1ecfb85116552bbf5882380c58666b726dd0d506/.github/workflows/static-analysis.yml#L16-L17

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

image

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.