search

facebook / pysa-action

GitHub Action for Pysa
MIT License
16 stars 12 forks source link

Remove submodule to use live version of pyre-check #3

Closed abishekvashok closed 1 year ago

abishekvashok commented 1 year ago

Previously we used a very old state of pyre-check repository as a submodule to run Github Actions against and test the action in this repository.

Currently, pyre-check has moved forward and current python module of pyre-check fails to detect issues based on the submodule. Also, since we want the action to be upto date with pyre-check it only makes sense to use git clone during the workflows instead of a submodule fixed on a particular reference.

Fixes the build error in the workflow

Demo: https://github.com/abishekvashok/pysa-action/actions/runs/4542528482 https://github.com/abishekvashok/pysa-action/actions/runs/4542528481

The demo workflows fail as I don't have the token configured for uploading any SARIF files (it should pass in this PR and repository)

Also see this PR's workflows.

What to look in the newer workflows? See that the sapp action runs successfully as opposed to that in main and see that both actions can find issues (as opposed to main in this repository)

Main workflow for reference: https://github.com/facebook/pysa-action/actions/runs/4534111680

arthaud commented 1 year ago

Do we still need this? Could you rebase it on master?

abishekvashok commented 1 year ago

@arthaud yes we do! See the failing workflow tests as to why we need this. I've resolved the conflicts :)

The idea is that we would like to use git clone in the workflow to test the pysa action as opposed to submodule because submodule is tied to a particular instance of pyre-check repository and can become outdated and un-synced with the pyre-check build used.

We also don't want SAPP to exit silently after applying filters when there are no issues, at least when the output mode is SARIF, as SARIF format expects a valid JSON file as per the spec with some metadata about the tool even in case of no issues. For that, I've made another PR. Ideally, these two diffs will be stacked internally.