Too many redundant tests executed on dev to master release for pipelines

[jfy133]

Description of the bug

When doing a release procedure, and a PR from dev is opened against master, when changes are made to dev during review, we see all CI tests running duplicated.

This is because tests are triggered for both pull-request and pushes. There was an assumption previously that the pull-request trigger only happened on opening, but actually it seems to trigger on any change to the PR (in addition to the push).

This can be very wasteful for pipelines with many tests, e.g. a release PR for taxprofiler as 11 tests x 2 nextflow versions x 3 container engines = 66, which when we have all of those being triggered for both PR change and push to the PR, results in >120 tests.

[jfy133]

A similar case is for the download test too: #3399

[jfy133]

Example PR when we had many duplicate tests: nf-core/taxprofiler#563

[MatthiasZepper]

I am happy to add changes to the currently open PR #3399, but I think we should discuss first which changes we want?

For example, nf_core/pipeline-template/.github/workflows/ci.yml currently looks like this:

name: nf-core CI
#This workflow runs the pipeline with the minimal test dataset to check that it completes without any syntax errors
on:
  push:
    branches:
      - dev
  pull_request:
  release:
    types: [published]
  workflow_dispatch:

The triggers for nf_core/pipeline-template/.github/workflows/linting.yml are similar.

There was an assumption previously that the pull-request trigger only happened on opening, but actually it seems to trigger on any change to the PR (in addition to the push).

To achieve this behavior, one could either restrict the trigger to newly/re- opened PRs

  pull_request:
    types:
      - opened
      - reopened

or add a synchronized type as well and remove the on push entirely.

In that case, no CI would run, if there is not yet an open release PR from dev to main. But is there any scenario, where changes are directly pushed to dev without a PR in nf-core? It might, however, be relevant for third parties using the pipeline template as well.

[jfy133]

I would need to check the exact behaviour myself, but I think on open/reopen would be ok to me for the time being.

[mirpedrol]

Checking this again, I think @MatthiasZepper suggestion to remove push sounds good, as pull_request will run for all PRs, and we don't need to add types as it is triggered on opened, reopened and synchronized by default. What do you think?

@MatthiasZepper let me know if you would like to add these changes to your opened PR or should I do it :)

[MatthiasZepper]

I am fine with adding that to the open PR when we have a decision.

I think, the key question when dropping push is, that no CI will run, if changes are directly pushed to dev without a PR. For a released pipeline and within nf-core, I guess, that is fine because all changes go through a pull request. But think of a solo developer only using the template, whether working towards a new nf-core pipeline or just on their own? Would they want to open a PR all the time just to get CI?

[mirpedrol]

If someone is pushing to dev directly either they run the tests manually with the workflow_dispatch or they will see the CI once they open a PR to master. TBH, if the code is already pushed I doubt someone would go and look for the tests.