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

Overhaul GHA #1200

Merged
merged 29 commits into from
Sep 1, 2023
Merged

Overhaul GHA #1200

merged 29 commits into from
Sep 1, 2023

Conversation

adamrtalbot
Copy link
Contributor

@adamrtalbot adamrtalbot commented Aug 31, 2023

Streamline Github Actions a bit to make them more efficient.

  • Tidy workflows into one testing workflow
  • Tidy up AWS tests into single workflow
  • Use native cacheing of actions for Python and Nextflow instead of manual cacheing
  • Match triggers for nf-core template in ci.yml
  • Use standard ci.yml template which should help with migration to nf-test
  • Add default tag to pytest-workflow if anything changes within the repo. Always runs the basic 4 tests.

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).

@adamrtalbot adamrtalbot requested a review from maxulysse as a code owner August 31, 2023 09:12
@github-actions
Copy link

github-actions bot commented Aug 31, 2023

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 9342569

+| ✅ 142 tests passed       |+
#| ❔   8 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-09-01 11:47:36

@@ -67,7 +53,6 @@ jobs:
env:
NXF_ANSI_LOG: false
TEST_DATA_BASE: "${{ github.workspace }}/test-datasets"
SENTIEON_LICENSE_BASE64: ${{ secrets.SENTIEON_LICENSE_BASE64 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm ... Are you sure about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it from the existing workflow, what should it be?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean you copied it from the existing workflow?

Anyways, I don't think the line should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It came from pytest-workflow_release.yml, which I merged with this one. Will add it back!

@adamrtalbot
Copy link
Contributor Author

@nf-core-bot fix linting

Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

Lgtm

@maxulysse
Copy link
Member

Can you update changelog ?

@adamrtalbot
Copy link
Contributor Author

Can you update changelog ?

Yup, I was waiting until I knew what I actually had changed!

@maxulysse
Copy link
Member

too many tests are triggered, can you restore the check changes part of the actions?

@adamrtalbot
Copy link
Contributor Author

too many tests are triggered, can you restore the check changes part of the actions?

Done, but we need a way of avoiding the paths-filter hitting the API limit. Hmmmmmmm.

@adamrtalbot
Copy link
Contributor Author

@maxulysse what does this parameter do and is it the cause of our current problems?

params.singularity_pull_docker_container = false

Error:

ERROR ~ JSONObject["singularity_pull_docker_container"] not found.

@maxulysse
Copy link
Member

Good catch

@maxulysse
Copy link
Member

Good catch

never mind, we don't that in dev anymore...

@adamrtalbot
Copy link
Contributor Author

Good catch

never mind, we don't that in dev anymore...

Fixed with #1203

@maxulysse
Copy link
Member

Good catch

never mind, we don't that in dev anymore...

yeah was looking at the wrong branch...

@maxulysse
Copy link
Member

add .git/workflows/ci.yml to the .nf-core.yml in files_exist

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.

Looks great to me!

Do you have an issue for the file-paths hitting the API rate limit? Maybe we should open it with them?

Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

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

some of it is a black box to me, but from what i see it is streamlining the tests, not changing them, right?

@@ -4,10 +4,24 @@ name: nf-core AWS test

on:
workflow_dispatch:
inputs:
Copy link
Contributor

Choose a reason for hiding this comment

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

nice I like it, much cleaner to have it all here

@adamrtalbot
Copy link
Contributor Author

Looks great to me!

Do you have an issue for the file-paths hitting the API rate limit? Maybe we should open it with them?

Yes we do. I tried to fix by using an empty token ('') so it used git directly but it just thought everything was different and ran 1 zillion tests. Quite a few people have hit the limit on their Github issues page.

@adamrtalbot
Copy link
Contributor Author

adamrtalbot commented Sep 1, 2023

some of it is a black box to me, but from what i see it is streamlining the tests, not changing them, right?

yup, condensing multiple workflows to single files wherever possible. Removing older cruft. I may have introduced some problems once it's on master but it's quite hard to test before merging.

@FriederikeHanssen
Copy link
Contributor

sorry didn't mean to edit your comment but quote as reply. 🤦‍♀️ goind to get some coffee ☕

@FriederikeHanssen
Copy link
Contributor

some of it is a black box to me, but from what i see it is streamlining the tests, not changing them, right?

yup, condensing multiple workflows to single files wherever possible. Removing older cruft. I may have introduced some problems once it's on master but it's quite hard to test before merging.

ok then let's merge and be ready for a patch release

Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

LGTM.
Anything we can push to TEMPLATE as well?

@adamrtalbot
Copy link
Contributor Author

adamrtalbot commented Sep 1, 2023

LGTM.
Anything we can push to TEMPLATE as well?

Kinda, if you look it only includes a very trivial example of running nextflow run . -profile test: https://github.com/nf-core/tools/blob/e5ce6ce20304835bd40f102f038b7e1aadc888b2/nf_core/pipeline-template/.github/workflows/ci.yml

So it's a little hard to migrate the changes over. Will we be adding pytest-workflow and the change tracking to them? Maybe we should? Perhaps it's worth a ticket and discussion before adding new code?

I can merge the AWS tests but that's the smallest part of this PR.

@adamrtalbot adamrtalbot merged commit 43a8704 into dev Sep 1, 2023
16 checks passed
@adamrtalbot adamrtalbot deleted the overhaul_ci_gha branch September 1, 2023 15:02
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.

6 participants