Skip to content

STRY0019104 - Test against GSP version of Nextflow in GitHub CI for all pipelines#26

Merged
apetkau merged 24 commits into
devfrom
update/nextflow-test
Oct 8, 2025
Merged

STRY0019104 - Test against GSP version of Nextflow in GitHub CI for all pipelines#26
apetkau merged 24 commits into
devfrom
update/nextflow-test

Conversation

@apetkau

@apetkau apetkau commented Oct 2, 2025

Copy link
Copy Markdown
Member

Adding GitHub CI tests against Nextflow 24.10.3

Also makes the following changes:

  • Updating GithubCI tests and nf-tests to fix nf-core linting issues.
  • Updating custom/dumpsoftwareversions and csvtk/concat nf-core modules to latest versions.
  • Removed unneeded input_check code from pipeline.

PR checklist

  • This comment contains a description of changes (with reason).
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • CHANGELOG.md is updated.

@github-actions

github-actions Bot commented Oct 2, 2025

Copy link
Copy Markdown

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 928bd6b

+| ✅ 141 tests passed       |+
#| ❔  20 tests were ignored |#
!| ❗   2 tests had warnings |!
Details

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.3.2
  • Run at 2025-10-06 13:37:26

@sgsutcliffe sgsutcliffe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, as far as I can tell. I am not so familiar with the types of changes you were making to the CI tests but I like the implementation of shards!

$(if [ -n "${{ inputs.tags }}" ]; then echo "--tag ${{ inputs.tags }}"; fi) \
--dry-run \
--ci \
--changed-since HEAD^) || {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is cool didn't know about this, so it is only going to run tests impacted by changes?

@apetkau apetkau Oct 7, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks. Since HEAD^ is referencing the parent commit of the HEAD commit I wanted to know more about how this worked on GitHub (since a PR can have multiple commits, so why does HEAD^ capture all changes in the PR)?

Based on what I've read, it looks like the GitHub actions checkout (https://github.com/marketplace/actions/checkout) will checkout code with a particular commit that is the squashed commit of all changes in your PR. Hence, HEAD^ will point to the commit prior to the PR, and so differences from HEAD^ (that is the --changed-since HEAD^ parameter) will properly capture all code changes in this PR. Running nf-test test ... --changed-since HEAD^ locally on a branch won't work (since it only looks at differences one commit up).

This is based on inspecting output of the GitHub actions and a copilot summary (I couldn't find good documentation on this outside of copilot):

Question: In git and github, what does the command "git fetch origin +fbae8663d8ad761d4e1191ad9359e32481e293cf:refs/remotes/pull/26/merge" do?

🔍 Breakdown of the command
git fetch origin: Fetches data from the remote repository named origin.

+fbae8663d8ad761d4e1191ad9359e32481e293cf:refs/remotes/pull/26/merge:

The + sign indicates a forced update of the reference (even if it’s not a fast-forward).
fbae866 is a commit SHA — the specific commit you want to fetch.

refs/remotes/pull/26/merge is a remote-tracking reference — typically used by GitHub to represent the result of >merging a pull request (PR #26 in this case) into the base branch.

Comment on lines +2 to +3
# This workflow is triggered on PRs to `main`/`master` branch on the repository
# It fails when someone tries to make a PR against the phac-nml `main`/`master` branch instead of `dev`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we were moving away from master?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's correct. The default checks from the nf-core templates should be for both master and main, so it should work for us still.

@apetkau
apetkau merged commit 9816571 into dev Oct 8, 2025
25 checks passed
@apetkau
apetkau deleted the update/nextflow-test branch October 8, 2025 14:53
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