jupyter-server / team-compass

A repository for team discussion, syncing, and meeting notes.
https://jupyter-server-team-compass.readthedocs.io
BSD 3-Clause "New" or "Revised" License
13 stars 7 forks source link

dropping pre-commit.ci #37

Open dlqqq opened 1 year ago

dlqqq commented 1 year ago

cc @blink1073

In the Github Actions config used by Jupyter Server and the server extension cookiecutter, we are already including a pre-commit step where we are invoking pre-commit run --all-files --hook-stage=manual. This was weird to me because I had expected this to be done by pre-commit.ci.

Playing around with the hook stages seemed to demonstrate why we were running pre-commit ourselves in tandem with pre-commit.ci. The pre-commit step

  - repo: https://github.com/sirosen/check-jsonschema
    rev: 0.15.0
    hooks:
      - id: check-jsonschema
        name: "Check GitHub Workflows"
        files: ^\.github/workflows/
        types: [yaml]
        args: ["--schemafile", "https://json.schemastore.org/github-workflow"]
        stages: [manual]

makes a network request to a JSON schema registry, which is forbidden by pre-commit.ci. Hence we isolate this step in a separate hook stage (manual) and then invoke pre-commit with that hook stage in our own Github Actions CI.

Which got me thinking: can we just drop pre-commit.ci since we have to run it ourselves anyways? I'm not sure how it's being included, but I suspect it's an org-level setting where all repos created under jupyter-server have pre-commit.ci included in its pipeline.

Dropping pre-commit.ci, along with #36, actually lets us drop the "manual" hook stage for all pre-commit steps, which is a big developer win. This will ensure pre-commit never fails exclusively in CI while passing locally (when hook stage is unspecified), making development way friendlier.

The next steps are similar to those of #36. Remove pre-commit.ci from Jupyter server, the server cookiecutter extension, and all other server extensions.

blink1073 commented 1 year ago

We use the manual stage for things that cannot be fixed automatically as well, like flake8. We don't want to block pushing code just because a linter fails, but we do want to block merging it. We also use pre-commit.ci for the auto-update of the pre-commit dependencies.