feat: add an opt-in linter job to the main workflow

[darkgl0w]

Hello.

Following fastify/fastify-swagger#531 (review), this PR aims to :

• add an optional linter job to the main reusable workflow
• add documentation on how to enable the linter job

Checklist

• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[mcollina]

lgtm

[Fdawgs]

This could be added to the existing workflows as a job, similar to what we had for the now removed coveralls input, rather than as a separate workflow?

[Eomm]

This could be added to the existing workflows as a job, similar to what we had for the now removed coveralls input, rather than as a separate workflow?

I agree with both solutions.

Keeping the lint into a dedicated file is fine, but we should add it here on the ci* files like https://github.com/fastify/workflows/blob/main/.github/workflows/plugins-ci.yml

jobs:
  linter:
    uses: fastify/workflows/.github/workflows/linter-ci.yml@main
  test:
    runs-on: ${{ matrix.os }}

    strategy:
   ....

Note the main: if we write v2 it wont work to test it.
We will set v2 before merging.

[Fdawgs]

This could be added to the existing workflows as a job, similar to what we had for the now removed coveralls input, rather than as a separate workflow?

I agree with both solutions.

Keeping the lint into a dedicated file is fine, but we should add it here on the ci* files like https://github.com/fastify/workflows/blob/main/.github/workflows/plugins-ci.yml

jobs:
  linter:
    uses: fastify/workflows/.github/workflows/linter-ci.yml@main
  test:
    runs-on: ${{ matrix.os }}

    strategy:
   ....

Note the main: if we write v2 it wont work to test it. We will set v2 before merging.

Unfortunately we can't use reusable workflows within reusable workflows 😞

[Eomm]

Oh, fine then. Thanks for pointing out.

So I think integrating it into the main plaugin-ci-* is more "comfortable" for the plugins' configuration and we are forcing a unified and optimized standard across all the repos.

[darkgl0w]

Hmmmm 🤔

Right we can't use reusable workflows within reusable workflows 😢 🤯

I will make the linter a part of the plugins-ci workflow (note that this will certainly require some adjustments in repos where it is deployed)

Here are some thoughts :

• we should harmonize the npm scripts among the repositories with required scripts, e.g.: Track the requirements #9 (comment)
• we should call npm run ci:test (or whatever name containing at least the 'ci' reference, test:ci, ci:test, ci...) instead of npm test inside the plugins-ci workflows
• or we can split the reusable workflows in pieces and make use of the starter workflows

Here is an example of what can become a starter workflow :

name: CI workflow

on:
  push:
    paths-ignore:
      - 'docs/**'
      - '*.md'
  pull_request:
    paths-ignore:
      - 'docs/**'
      - '*.md'

jobs:
  # runs first and if errors are detected it doesn't waste resources because it will stop the dependent workflows
  linter:
    uses: darkgl0w/workflows/.github/workflows/ci-linter.yml@linter-ci

  # the test runner (currently node 10, 12, 14, 16) - when we drop a node version we remove it in one file only
  test:
    needs: linter
    uses: darkgl0w/workflows/.github/workflows/plugins-ci.yml@linter-ci

  # the test runner (only node current branch 17 - and we just need to change the version inside this workflow)
  experimental:
    needs: linter
    uses: darkgl0w/workflows/.github/workflows/plugins-ci-node-current.yml@linter-ci

https://github.com/darkgl0w/fastify-swagger/actions/runs/1738731030

cc @Eomm @Fdawgs WDYT ?

[Fdawgs]

This could be added to the existing workflows as a job, similar to what we had for the now removed coveralls input, rather than as a separate workflow?

@darkgl0w, i'd probably add it as an input which is off by default. This is so if certain repos don't need/want linting then we can disable it:

on:
  workflow_call:
    inputs:
      lint:
        description: 'Set to true to run linting scripts.'
        required: false
        type: boolean

jobs:
  lint:
    if: inputs.lint == true
     # rest of job goes here

Then it would be called like so:

name: CI workflow

on: [push, pull_request]
jobs:
  test:
    uses: darkgl0w/workflows/.github/workflows/plugins-ci.yml@main
    with:
       lint: true

[darkgl0w]

Gotcha !
I will fire some sample run on my test repo to see if the needs requirement will cause trouble

[darkgl0w]

@Fdawgs > I ran some sample runs following your suggestions and this is what I've come up with due to the limitations of GitHub Actions workflows :

jobs:
  test:
    needs: linter
    # workaround to always run this job
    if: ${{ always() }}
    runs-on: ${{ matrix.os }}

    strategy:
      matrix:
        node-version: [10, 12, 14, 16]
        os: [macos-latest, ubuntu-latest, windows-latest]
    steps:
      # workaround to fail if linter fail
      - name: Stop on linter job failure
        if: ${{ needs.linter.result == 'failure' }}
        run: exit 1

      - uses: actions/checkout@v2

      - name: Use Node.js
        uses: actions/setup-node@v2
        with:
          node-version: ${{ matrix.node-version }}

      - name: Install Dependencies
        run: npm i --ignore-scripts

      - name: Run Tests
        run: npm test

Some thoughts :

• it works despite the limitations of GitHub Actions workflows 👍
• it will start test jobs even though the linter job fails (wasting resources until it hits the Stop on linter job failure step) 👎
• I find this a little less flexible than using workflow templates with starter workflows 🤔

for reference - workflow CI runs:
with lint: true and code linting errors
with lint: true and no code linting errors
with lint: false

[Fdawgs]

Cheers @darkgl0w, I've been busy finishing up some work pieces today, so will have a look tomorrow.

[Eomm]

it will start test jobs even though the linter job fails

Do you know why this is happening?

[darkgl0w]

Do you know why this is happening?

This is a limitations that comes with the conditional start of the linter job and its requirement for starting the test job.

In order to be able to bypass the need of the linter job by the test job when the lint input is set to false we resort to a workaround (if: ${{ always() }}) and we add an extra check that fail the test job when there are linting errors

      # workaround to fail if linter fail
      - name: Stop on linter job failure
        if: ${{ needs.linter.result == 'failure' }}
        run: exit 1

With the linter included in the main workflow behind an optional activation input that's the best we can do to reduce the waste of resources.

The other solution would be to split the jobs and to use GitHub Starter workflows. (cf. #8 (comment), this is an example of what we can achieve by combining reusable workflows and starter workflows across the org repos).

Both have their pros and their cons.

[Eomm]

Thanks for explaining.
I would keep it simple:

• the fastify org is the first citizen of this workflow
• there are not repositories without the linter

So I would go for the non-skippable lint step (this should solve the need for this Stop on linter job failure step)

We could add an input option to set the if-present arg to true in the case we don't have a lint script into the package.json
https://docs.npmjs.com/cli/v8/commands/npm-run-script#if-present

[darkgl0w]

@Eomm > applied your suggestion and with npm run lint --if-present the linter job will emit an exit 0 command only if the lint script does not exist ^^

linting issues
no linting issues
no lint script
normal run

[Fdawgs]

https://docs.npmjs.com/cli/v8/commands/npm-run-script#if-present

Had completely forgotten this existed, this makes it so much easier.

[darkgl0w]

I have found a better solution directly inside the Github actions :P
Will push in a few seconds ^^

[darkgl0w]

With this lint is actually optional and the waste of resources is minimal 😃

CI sample runs:
disabled (default) - lint error
disabled (option) - lint error
enabled (option) - lint error
enabled (option) - no lint error

[Fdawgs]

...Where's the chef's kiss emoji? 👨‍🍳
LGTM, thanks @darkgl0w.

[darkgl0w]

I think that we can push those workflows even further and choose to exclude/include some node/os versions.
Following this logic we can also define an input that allows us to customize the script name that we want to run with a fallback to npm run test

[edit]
Here is an iteration that allows to customize the script name:
https://github.com/darkgl0w/workflows/tree/exp-alt/.github/workflows/plugins-ci.yml