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

Pipeline template: Fix download test CI #2727

Merged
merged 3 commits into from Feb 6, 2024

Conversation

MatthiasZepper
Copy link
Member

@MatthiasZepper MatthiasZepper commented Feb 2, 2024

When writing the CI test that tested nf-core download on the current pipeline, I intended to run the test for any push on a branch underlying an active PR to dev. This required that the CI action would dynamically determine the branch name, for which I naively queried /refs/heads/.

However, I now learned that resolving the branch name is a bit more complex than I initially thought, since there may be a HEAD, FETCH_HEAD, ORIG_HEAD, MERGE_HEAD and CHERRY_PICK_HEAD and sadly branch determination already failed in practice (cc @sofstam ) because a MERGE_HEAD existed in addition to HEAD.

Since it is quite difficult to determine the current branch reliably (except maybe by running git?) and we are exclusively running the CI test for merge-PRs to master, I decided to hardcode the dev branch instead. This works on PRs to master, but it is still possible to manually dispatch the action on any other branch if needed.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@MatthiasZepper MatthiasZepper self-assigned this Feb 2, 2024
@MatthiasZepper MatthiasZepper added the bug Something isn't working label Feb 2, 2024
@MatthiasZepper MatthiasZepper marked this pull request as ready for review February 2, 2024 15:42
@MatthiasZepper MatthiasZepper force-pushed the download_test_branch_fix branch 2 times, most recently from 81e24ae to 6280bc9 Compare February 5, 2024 11:01
@MatthiasZepper MatthiasZepper marked this pull request as draft February 5, 2024 11:07
@MatthiasZepper
Copy link
Member Author

Downgrading this to draft again.

The branch issue seems to be resolved by setting dev as default unless explicitly chosen otherwise when manually dispatching the workflow.

However, there is another issue with Jinja templating that needs fixing as well:


                                          ,--./,-.
          ___     __   __   __   ___     /,-._.--~\
    |\ | |__  __ /  ` /  \ |__) |__         }  {
    | \| |       \__, \__/ |  \ |___     \`-._,-`-,
                                          `._,._,'

    nf-core/tools version 2.13.dev0 - https://nf-co.re/


                                                                                
 Usage: nf-core download [OPTIONS] <pipeline name>                              
                                                                                
 Try 'nf-core download -h' for help                                             
╭─ Error ──────────────────────────────────────────────────────────────────────╮
│ Got unexpected extra arguments (raw %} matthiaszepper/testpipeline)          │
╰──────────────────────────────────────────────────────────────────────────────╯
                                                                                
Error: Process completed with exit code 2.

When investigating the cause of this error message in a Testpipeline CI run, I stumbled over two code fragments ({% raw %} and an {% endraw %}), which I had surely not inserted into "my" action.

Having no knowledge about their origin, I presumed that those would be odd hallucinations from Copilot and kicked them out in the course of this PR again. Upon a closer look, it turned out that @mashehu added them on purpose, but I am not aware of the rationale.

In either case, we must find a solution that does not interfere with the commands, but also settled the issue that prompted their inclusion in the first place.

@mashehu
Copy link
Contributor

mashehu commented Feb 5, 2024

The raw, endraw are jinja escape statements: https://jinja.palletsprojects.com/en/3.0.x/templates/#escaping
Otherwise jinja will be confused by the github ${{ characters

@MatthiasZepper MatthiasZepper force-pushed the download_test_branch_fix branch 2 times, most recently from 1925e80 to 5e26891 Compare February 5, 2024 16:42
@MatthiasZepper
Copy link
Member Author

MatthiasZepper commented Feb 5, 2024

I see. Then everything was just a big misunderstanding from my side. The escape statements did interfere, but only because I lazily copied the file across from my local tools repo to the testpipeline folder for testing instead of using tools, so the Jinja escape statements were not removed in the process.

Github Copilot is also exonerated. I was totally surprised that those escape sequences suddenly showed up in the file, even though I could not remember seeing them shortly before, and an automatic Copilot change seemed the only plausible explanation. In reality, that change popped up because I rebased my feature branch onto dev due to merge conflicts in the Changelog and failed to notice that also the action itself was changed in the process.

I have now dropped the two commits in which I deleted them from this branch, so the history is clean again.

@MatthiasZepper MatthiasZepper marked this pull request as ready for review February 5, 2024 16:57
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (979883a) 73.47% compared to head (de73d39) 73.61%.

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mashehu mashehu merged commit 3bc83a5 into nf-core:dev Feb 6, 2024
34 checks passed
@MatthiasZepper MatthiasZepper deleted the download_test_branch_fix branch February 6, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants