Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for initialize breaking the template #4855

Closed
wants to merge 3 commits into from

Conversation

edmundmiller
Copy link
Contributor

@edmundmiller edmundmiller commented Feb 6, 2024

If we're moving stuff out of the template that can break the template we need to check for changes.

Can include any other files that might break the template as well.

@edmundmiller
Copy link
Contributor Author

I have no idea how the CI for that works in tools @nf-core/infrastructure 😬 looks like I need some template files but I can't find those.

Can include any other files that might break the template as well
Need to migrate to nf-test
@adamrtalbot
Copy link
Contributor

Sorry, not sure I understand what this is for?

Is this so when you create a pipeline the modules/subworkflows components haven't messed up the default running?

@edmundmiller
Copy link
Contributor Author

Is this so when you create a pipeline the modules/subworkflows components haven't messed up the default running?

Exactly!

Scenario: PR to this breaks the template. There's no check. It breaks tools but doesn't get discovered for a week. Then @nf-core/infrastructure has to go back and figure out the murder mystery.

@adamrtalbot
Copy link
Contributor

adamrtalbot commented Feb 6, 2024

Hmmm can we approach this in a better way? It's hideously convoluted to test a different repo within another repo.

  • Firstly any subworkflow should be completely encapsulated and tested, so no errors within a subworkflow
  • The subworkflow can be inserted without touching other components.
  • When using the template, it pulls a specific version of the subworkflow, so we have to make a deliberate choice to update it.

That way, the tests can all be done on the nf-core/tools side.

Put another way, we should make it so there is no way updating a subworkflow or module breaks the template. This also applies to default modules like FASTQC and MultiQC.

@edmundmiller
Copy link
Contributor Author

no way updating a subworkflow or module breaks the template

Love it, that's the goal!

It's hideously convoluted to test a different repo within another repo.

Yeah it's a bit much to think about.

How does tools currently install/update fastqc?

@mashehu
Copy link
Contributor

mashehu commented Feb 6, 2024

How does tools currently install/update fastqc?

It's a manual step in the release process: https://github.com/nf-core/tools/blob/master/.github/RELEASE_CHECKLIST.md?plain=1#L6

@adamrtalbot
Copy link
Contributor

It just copy + pastes it. It could actually be completely out of sync and we wouldn't know until tests in tools failed: https://github.com/nf-core/tools/tree/dev/nf_core/pipeline-template/modules/nf-core/fastqc

@edmundmiller
Copy link
Contributor Author

Via @mashehu -bot nf-core/tools@8d1094c

So we'd catch it there, might just go a few months without noticing, or be something that breaks when the dependancies get bumped before release and stales it.

Maybe a cron job that runs the module and subworkflow update and bumps it in modules so it can get fixed ahead of time and not block a release?

@adamrtalbot
Copy link
Contributor

adamrtalbot commented Feb 6, 2024

(regarding copy + pasting the module)

Which is a pretty good choice - simpler is better.

@mashehu
Copy link
Contributor

mashehu commented Feb 6, 2024

Maybe a cron job that runs the module and subworkflow update and bumps it in modules so it can get fixed ahead of time and not block a release?

couldn't renovate do this?

@edmundmiller
Copy link
Contributor Author

couldn't renovate do this?

Probably, I think we debated it. But I think that's a whole thing to make a renovate datasource for nf-core/modules.

@mashehu
Copy link
Contributor

mashehu commented Feb 9, 2024

closing this, because we will use nf-core/tools#2733 instead

@mashehu mashehu closed this Feb 9, 2024
@SPPearce SPPearce deleted the initialize-test branch May 15, 2024 08:54
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.

3 participants