Skip to content

Condense testing and pre-commit workflow into single workflow#4545

Merged
edmundmiller merged 1 commit intonf-core:masterfrom
adamrtalbot:add_pre-commit_to_testing_workflow
Dec 7, 2023
Merged

Condense testing and pre-commit workflow into single workflow#4545
edmundmiller merged 1 commit intonf-core:masterfrom
adamrtalbot:add_pre-commit_to_testing_workflow

Conversation

@adamrtalbot
Copy link
Contributor

@adamrtalbot adamrtalbot commented Dec 6, 2023

My war on workflow bloat continues.

@adamrtalbot adamrtalbot requested review from a team, edmundmiller, maxulysse and sateeshperi and removed request for a team December 6, 2023 11:12
@edmundmiller
Copy link
Contributor

I think I'm actively working against you here 😆

Let's see if you can catch me up.

  1. Could we use Reuseable workflows? My main issue is the 608 line yaml gorrilla in the room.
  2. Why do we want them all in one workflow?

@adamrtalbot
Copy link
Contributor Author

We know we saturate our Github Action runners at every opportunity, we need to be more frugal with spinning up a new machine. Each new workflow requires a whole new runner, which needs to hit the APIs more times, which uses quota, money etc. By putting it in the same workflow, it can share variables, secrets, input triggers, etc. which means fewer things to maintain and go wrong. Generally, people create a new workflow when they want to create a new job.

This is a single step workflow, which does a related thing to the test workflow (lint). My rule of thumb is if it uses the same trigger they should be in the same workflow. We could even put this in the same job as the linting to make this more efficient.

It's only 608 lines because it duplicates all code across pytest and nf-test and it has 1 million exceptions for conda in both. Once you remove that it's only ~230 lines which is pretty low for a monorepo like this.

edmundmiller added a commit to nf-core/ops that referenced this pull request Dec 7, 2023
nf-core/modules#4545 (comment)

Co-authored-by: adamrtalbot <adamrtalbot@users.noreply.github.com>
Copy link
Contributor

@edmundmiller edmundmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the detailed explaination, I'm caught up now!

Documented it here nf-core/ops@a76929c for other to stumple on.

@edmundmiller edmundmiller added this pull request to the merge queue Dec 7, 2023
Merged via the queue into nf-core:master with commit 9b1b7a7 Dec 7, 2023
@adamrtalbot adamrtalbot deleted the add_pre-commit_to_testing_workflow branch December 7, 2023 14:13
@adamrtalbot
Copy link
Contributor Author

Let's take it to it's natural conclusion: #4554

edmundmiller added a commit to nf-core/ops that referenced this pull request Jul 25, 2025
nf-core/modules#4545 (comment)

Co-authored-by: adamrtalbot <adamrtalbot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments