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

Remove 'Tower' from almost everywhere.. #2853

Merged
merged 5 commits into from
Mar 18, 2024
Merged

Conversation

ewels
Copy link
Member

@ewels ewels commented Mar 18, 2024

Tried to replace "Nextflow Tower" with "Seqera Platform" everywhere I could.

Special note:

  • nf-core download --tower now uses --platform
    • The short flag is still -t as -s and -p were already taken
    • I left --tower in place for now but hid it from help text. We can remove it fully in the future.
  • I replaced mention of tower in the code as well as docs, wherever possible
  • Some stuff can't be switched yet and needs updating in the future, eg. action-tower-launch
  • I didn't bother updating the Changelog

I didn't change the secret names, such as secrets.TOWER_WORKSPACE_ID. Though I guess we could do.

Haven't done loads of testing at this point, so any help on that front would be much appreciated 🙏🏻

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

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

Project coverage is 74.79%. Comparing base (189a4af) to head (3b78154).

❗ Current head 3b78154 differs from pull request most recent head 9f9b75d. Consider uploading reports for the commit 9f9b75d to get more accurate results

Files Patch % Lines
nf_core/download.py 84.61% 2 Missing ⚠️
Additional details and impacted files

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

@ewels
Copy link
Member Author

ewels commented Mar 18, 2024

@nf-core-bot fix linting

@MatthiasZepper
Copy link
Member

Before going into detailed review here: Why now --platform ?

I thought we had long settled for the neutral --bare on Slack? (I know for sure, because I preferred archive but then went with the majority vote). Using Tower was making a bow to Seqera, but in retrospect, I think it was a poor judgement to depend on any corporate naming choices, in particular since those pipelines can still be run without Tower/Platform/Whatever future rebranding.

Copy link
Member

@MatthiasZepper MatthiasZepper left a comment

Choose a reason for hiding this comment

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

Just a typo when replacing Tower in one file and the overall discontent with the branding. --collection would be my pick.

But if other (non-Seqera affiliated people!) agree, then I will survive having it changed to --platform.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
hidden=True,
help="Download for Seqera Platform. DEPRECATED: Please use --platform instead.",
)
@click.option(
Copy link
Member

Choose a reason for hiding this comment

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

Not at all happy with using --platform, but neither with --bare.

Still would strongly prefer something neutral that suggests that multiple pipeline revisions can be contained: --collection, --package or --multi.

The strongest argument against --collection or --package was that the -c respectively -p short parameters were already taken (or in the case of -c reserved for config). But if that isn't paramount and any letter within the word will do, then collection also contains a -t. That will of course be nice to preserve backwards compatibility.

requirements.txt Outdated Show resolved Hide resolved
@ewels
Copy link
Member Author

ewels commented Mar 18, 2024

Wow, how many times can I mis-spell the name of my employer? 😆 Thanks @MatthiasZepper!

I don't have particularly strong opinions about the CLI flag, other than it should be obvious and easy for find for folks who want to use it. Personally I don't think that any of --collection, --package or --multi really fulfil that - I would assume that each did something different just from the flag name.

Do we know of any other practical uses for this flag apart from running Seqera Platform?

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.

Think the discussion is outside the scope of this PR. This is just a style change.

I think you should open an issue or reopen the old issue if you'd like to continue to discuss @MatthiasZepper.

@ewels ewels merged commit b9699d2 into nf-core:dev Mar 18, 2024
33 checks passed
@ewels
Copy link
Member Author

ewels commented Mar 18, 2024

Thanks all!

@ewels ewels deleted the update_tower_url branch March 18, 2024 23:13
@MatthiasZepper
Copy link
Member

Do we know of any other practical uses for this flag apart from running Seqera Platform?

You can nextflow run such a download with a file:/absolute/path/to/pipeline.git. If one wishes to keep multiple pipeline revisions in a convenient, space-saving multi-revision collection/package, then those downloads are perfectly suitable and easier to handle than the classic multi-revision download, that creates subfolders for each revision.

@MatthiasZepper
Copy link
Member

Think the discussion is outside the scope of this PR. This is just a style change.

Just a style change? I wish I had known that when I wanted to change some CLI arguments of the nf-core download command a year ago. Oh boy, that was an uphill battle, discussing every single change ad nauseam and in the end settling with a half-way compromise.

I think you should open an issue or reopen the old issue if you'd like to continue to discuss @MatthiasZepper.

No, I won't. I am obviously a pushover, but I do also not what to stubbornly escalate this into a Wikipedia-style edit war. Merged is merged.

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.

None yet

4 participants