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

nf-core download: Ability to add custom tags to revisions in downloaded pipelines #2938

Merged
merged 16 commits into from
May 7, 2024

Conversation

MatthiasZepper
Copy link
Member

@MatthiasZepper MatthiasZepper commented Apr 26, 2024

This PR introduces a -a / --additional-tag argument to nf-core download --platform. It must be followed by a string in a "key=value" format and can be provided multiple times. The left-hand side must refer to a valid branch, tag or commit SHA. The right-hand side must comply with the naming conventions for Git tags and may not yet exist in the repository.

The new -a / --additional-tag argument allows customizing the downloaded pipeline with additional tags that can be used to select particular revisions in the Seqera Platform interface.

For example, an accredited facility may opt to tag particular revisions according to their structured release management process: -a "3.12.0=testing" -a "3.9.0=validated" so their staff can easily ensure that the correct version of the pipeline is run in production.

I believe this feature will be useful for facilities and those who are setting up pipelines for others to run.

Additionaly, this PR also bumps the dev version from 2.13.2dev to 2.14dev to comply with SemVer. Mind that other PRs already merged on dev, e.g. #2853 by @ewels should have triggered this before because it deprecated the --tower argument:

  1. Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated.

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
Copy link
Member Author

Interesting...adding tags depends on a successfully configured git user:


ERROR    nf_core.download:download.py:1741 [red]Additional tag(s) could not be applied:[/]
Cmd('git') failed due to: exit code(128)
  cmdline: git tag -m Synonynmous tag to 3.7; added by 'nf-core download'. a.tad.outdated 3.7
  stderr: 'Committer identity unknown

*** Please tell me who you are.

Run

  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: unable to auto-detect email address (got 'runner@fv-az566-7.(none)')'

nf-core download already deletes superfluous tags...which apparently is no problem.

@MatthiasZepper MatthiasZepper force-pushed the Download-CustomTags branch 2 times, most recently from afcacf1 to 014962f Compare April 28, 2024 15:57
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 85.10638% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 75.42%. Comparing base (98abe32) to head (c95880f).
Report is 8 commits behind head on dev.

Files Patch % Lines
nf_core/download.py 86.20% 4 Missing ⚠️
nf_core/synced_repo.py 86.66% 2 Missing ⚠️
nf_core/__main__.py 66.66% 1 Missing ⚠️
Additional details and impacted files

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

tests/test_download.py Outdated Show resolved Hide resolved
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM, but I will let @mashehu have another look before approving :)

Copy link
Contributor

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

Almost there, would just like to discuss alternatives to -a before giving 👍🏻

README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
- Better error message when GITHUB_TOKEN exists but is wrong/outdated
- New `-a` / `--additional-tag` argument to add custom tags during a pipeline download ([#2938](https://github.com/nf-core/tools/pull/2938))
Copy link
Contributor

Choose a reason for hiding this comment

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

tbh not a big fan to use -a for something other then --all 🙂 how about just going for --tag?

Copy link
Member

Choose a reason for hiding this comment

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

+1 for --tag. I'm not too fussed about always providing short-form flags for every option. I think it's fine to have only long-form for the more unusual ones like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Notwithstanding, it is so forthcoming of Seqera to rename their web application just to free up the -t short-form flag for the future 😝...

Copy link
Member

Choose a reason for hiding this comment

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

👉🏻 #2938 (comment)

nf_core/download.py Outdated Show resolved Hide resolved
# "Private" method to add the additional custom tags to the repository.
def __add_additional_tags(self) -> None:
if self.additional_tags:
self.ensure_git_user_config(f"nf-core download v{nf_core.__version__}", "core@nf-co.re")
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure we want to use the nf-core-bot as the default user? I would prefer an error I think

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't need to be the nf-core bot we are using here as default, but an error for that reason is not a real option in my opinion.

After all, the downloaded bare repositories are pretty ephemeral and one would need to configure git properly to avoid. Admittedly, it is just two git commands that need to be run once and then never again, but I for example use nf-core download exclusively on Rackham, so I have Singularity available. But I am not doing development work on Rackham, so I haven't set up git there properly. The same goes for the func user that we use to prepare the deployments.

Lastly, mind that it was actually the failing CI that prompted me to include this, so if we switch back to error out, we first have to adapt our Pytest CI 🫣.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @MatthiasZepper on this one - this git action of adding a tag isn't really meaningful as a development step. This is purely an action to make running the pipeline easier, where the user behind the action is not visible. So I don't think that it matters what the account is, so I would prefer not to error.

That being said, I don't think that it needs to be a real account either? So maybe we can set it to automated@tag.com or some other fake email..?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have changed the default e-mail address. I presumed, it was a sinkhole / do not reply address, but apparently not.

nf_core/synced_repo.py Outdated Show resolved Hide resolved
tests/test_download.py Outdated Show resolved Hide resolved
tests/test_download.py Outdated Show resolved Hide resolved
nf_core/download.py Outdated Show resolved Hide resolved
@MatthiasZepper MatthiasZepper force-pushed the Download-CustomTags branch from 9135e84 to 4b0fe7b Compare May 3, 2024 16:59
nf_core/__main__.py Outdated Show resolved Hide resolved
@MatthiasZepper MatthiasZepper force-pushed the Download-CustomTags branch from 5e35492 to 5e640b7 Compare May 6, 2024 12:53
# "Private" method to add the additional custom tags to the repository.
def __add_additional_tags(self) -> None:
if self.additional_tags:
self.ensure_git_user_config(f"nf-core download v{nf_core.__version__}", "core@nf-co.re")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.ensure_git_user_config(f"nf-core download v{nf_core.__version__}", "core@nf-co.re")
self.ensure_git_user_config(f"nf-core download v{nf_core.__version__}", "test@example.com")

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I had at the same time googled a bit about suitable sinkhole addresses, and now settled on dev-null@example.com. Ultimately, I could not find any information on behalf of ICANN / IETF that one may indeed use any e-mail of that reserved domain for anything else than documentation.

@MatthiasZepper MatthiasZepper force-pushed the Download-CustomTags branch from 5c3149b to 0b8e4ad Compare May 6, 2024 13:31
@MatthiasZepper MatthiasZepper requested a review from mashehu May 6, 2024 13:43
README.md Outdated Show resolved Hide resolved
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
@MatthiasZepper MatthiasZepper force-pushed the Download-CustomTags branch from 306264d to 3277b7c Compare May 6, 2024 17:24
@MatthiasZepper MatthiasZepper merged commit fc78f07 into nf-core:dev May 7, 2024
35 checks passed
@MatthiasZepper MatthiasZepper deleted the Download-CustomTags branch May 7, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants