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.

Sign up for GitHub

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

Jump to bottom

Improve cloud testing matrix strategy #1378

Merged
merged 12 commits into from
Apr 25, 2024
Merged

Improve cloud testing matrix strategy #1378

merged 12 commits into from
Apr 25, 2024

Conversation

adamrtalbot
Copy link
Contributor

@adamrtalbot adamrtalbot commented Jan 17, 2024 •
edited
Loading

  • This PR improves the cloud testing matrix strategy to use one job/step per pipeline launch. This is simpler and less error prone.
  • It also launches in a single step.
  • Uses matrix to retrieve secrets, a workaround for secrets not being allowed in a matrix.

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 17, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 1275645

+| ✅ 181 tests passed       |+
#| ❔  14 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: conf/modules.config
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/WorkflowSarek.groovy
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/sarek/sarek/.github/workflows/awstest.yml
  • template_strings - template_strings

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-04-25 09:14:49

@adamrtalbot
Copy link
Contributor Author

@nf-core-bot fix linting

@adamrtalbot
Copy link
Contributor Author

Ideally I'd like to move the if statement to the job level, so it doesn't trigger an empty job that doesn't do anything. But baby steps...

@adamrtalbot
Copy link
Contributor Author

if statement is better now, although it still runs empty jobs...

@adamrtalbot adamrtalbot mentioned this pull request Jan 17, 2024
Closed
10 tasks
@maxulysse
Copy link
Member

why renaming it back to .github/workflows/awstest.yml?

@adamrtalbot
Copy link
Contributor Author

adamrtalbot commented Jan 17, 2024 •
edited
Loading

why renaming it back to .github/workflows/awstest.yml?

You can't test it under an alternative filename. We have to change the filename when we merge it to main

FriederikeHanssen
FriederikeHanssen reviewed Jan 17, 2024
View reviewed changes
.github/workflows/awstest.yml Outdated
somatic:
description: "Somatic full test"
type: boolean
default: false
Copy link
Contributor

Choose a reason for hiding this comment

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

hm does this turn of that they are run automatically on release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The whole workflow is triggered by:

  • On release
  • Workflow dispatch (manual)

If manual, there is an inputs item which includes these variables. When it runs, it checks the inputs item for the matrix.test and matrix.cloud key, e.g. if matrix.test = azure, it checks inputs.azure is false, based on manual setting, then negates it. This means if it is true, the test will run. If the workflow is automatically triggered, inputs does not exist and it will not find inputs.azure, therefore the test will run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having said that, I actually have no way of testing this automatically.

Copy link
Member

Choose a reason for hiding this comment

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

then let's merge that, and we'll test

@maxulysse maxulysse merged commit 752494f into dev Apr 25, 2024
8 checks passed
@maxulysse maxulysse deleted the improve_cloud_tests_matrix branch April 25, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FriederikeHanssen FriederikeHanssen FriederikeHanssen left review comments

@maxulysse maxulysse maxulysse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adamrtalbot @maxulysse @FriederikeHanssen @nf-core-bot