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

Revise toolshed config dependency check error message in repository install manager #8720

Merged
merged 2 commits into from Sep 28, 2019

Conversation

guerler
Copy link
Contributor

@guerler guerler commented Sep 26, 2019

Follow-up to #8660. Fixes #8715.

Copy link
Member

@jmchilton jmchilton left a comment

Choose a reason for hiding this comment

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

It feels like the check should be modernized and fixed but eliminated entirely. I'm fine with this though - we don't have any testing to verify the check works clearly so if we decide to do this better later we can add that with tests.

Copy link
Member

@jdavcs jdavcs left a comment

Choose a reason for hiding this comment

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

app.tool_dependency_dir can be None. I'm afraid I can't estimate whether this check is needed here (I don't know this part of the codebase well enough), so I'll rely on @jmchilton 's opinion.

@nsoranzo
Copy link
Member

I cannot reproduce #8715, is this still needed after #8696 was merged? If yes, how to reproduce?

@nsoranzo
Copy link
Member

nsoranzo commented Sep 27, 2019

app.config.tool_dependency_dir can be None, while I don't think app.tool_dependency_dir should be None just because tool_dependency_dir is not set in galaxy.yml .

See https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/config/__init__.py#L1102 and https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/tool_util/deps/__init__.py#L83

@guerler
Copy link
Contributor Author

guerler commented Sep 27, 2019

This underlying issue seems to be fixed now by a recent change in dev. Thank you @nsoranzo! Should we still remove the check here or at least revise the message?

@nsoranzo
Copy link
Member

Should we still remove the check here or at least revise the message?

I think we can revise the message to something like "Tool dependencies installation is disabled in your configuration files."

@guerler guerler changed the title Remove toolshed config dependency check from repository install manager Revise toolshed config dependency check error message in repository install manager Sep 28, 2019
@martenson martenson merged commit ccaf426 into galaxyproject:dev Sep 28, 2019
@guerler guerler deleted the remove_dependency_check branch February 19, 2020 23:13
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.

Galaxy fails to install tools with dependencies
6 participants