afids / afids-validator

Validator for the anatomical fiducial placement protocol
https://validator.afids.io
GNU General Public License v3.0
2 stars 4 forks source link

BUG: Actions Linter #122

Closed kaitj closed 3 years ago

kaitj commented 3 years ago

The problem

The current linter within the workflow is triggered by the unit test and will automatically lint and fix any known issues. However, this is does not work with PRs from upstream repos. This is a known issue due to GITHUB_TOKEN only having read-only access on forked branches: https://github.com/wearerequired/lint-action/issues/13

Some possible workarounds I can think of involve running a linter where the workflow will fail if it does not pass. Can think of 2 ways of doing this:

  1. As an additional linter, triggering only when the current linter fails (the added benefit is any pushes directly onto this repo can be automatically fixed): Should be something like

    linting:
    steps:
    - name: black ...
    ...
    ...
    
    - name: other linter
    if: $ {{ failure() }}
    run: | 
      some lint command

    Not sure what the status of the workflow would be though if this runs.

  2. Replacing the existing linter and rely on users to fix errors.

Any thoughts @ostanley @tkkuehn ?

Environment

_PR template was adopted from appium_

kaitj commented 3 years ago

Github introduced a pull_request_target event that should be able to run actions on upstream repos. ~It would mean the current linter would be making changes to another user's repo and I'm not sure if we want that.~.

Testing it on a forked repo, looks like linter will run using the new event. It is unclear whether the error message below was left in from a previous version or if it actually fails to push commits to a forked branch.

image

If it throws an error on a forked repo and can't push fixes, we can leave it to the user to make the necessary changes.

kaitj commented 3 years ago

In a somewhat related note, do we want to include a pre-commit hook to enforce the linting? It may be redundant given this already lints incoming PRs.

Pre-commit hook has been added in #124