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

Use the configured tool_path when loading data managers #3989

Merged
merged 4 commits into from Apr 28, 2017

Conversation

Projects
None yet
4 participants
@davebx
Copy link
Contributor

commented Apr 27, 2017

This should address #3606. I've also confirmed that my local instance can still load data managers with these changes.

@davebx davebx added this to the 17.05 milestone Apr 27, 2017

@@ -7,23 +7,37 @@
from tool_shed.util import xml_util

log = logging.getLogger( __name__ )
# <data_managers tool_path="/var/galaxy/data_managers">

This comment has been minimized.

Copy link
@martenson

martenson Apr 27, 2017

Member

this is probably a dev remnant?

This comment has been minimized.

Copy link
@davebx

davebx Apr 27, 2017

Author Contributor

That is exactly what it is. I even reminded myself to remove it before pushing.

data_managers_path = root.get( 'tool_path', None )
return data_managers_path


This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 27, 2017

Member

Can you remove one blank line to make flake8 happy?

if tree:
root = tree.getroot()
data_managers_path = root.get( 'tool_path', None )
return data_managers_path

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 27, 2017

Member

data_managers_path is undefined if bool(tree) is False

@@ -57,7 +57,7 @@ def load_from_xml( self, xml_filename, store_tool_path=True, replace_existing=Fa
tool_path = '.'
self.tool_path = tool_path
for data_manager_elem in root.findall( 'data_manager' ):
self.load_manager_from_elem( data_manager_elem, replace_existing=replace_existing )
self.load_manager_from_elem( data_manager_elem, tool_path=tool_path, replace_existing=replace_existing )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 27, 2017

Member

This should be tool_path=self.tool_path because tool_path is undefined if store_tool_path is False.

tree, error_message = xml_util.parse_xml( self.app.config.shed_data_manager_config_file )
if tree:
root = tree.getroot()
return root.get( 'tool_path', None )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 28, 2017

Member

Thanks @davebx! Maybe also add:

        return None

for the case when tree is None.

This comment has been minimized.

Copy link
@davebx

davebx Apr 28, 2017

Author Contributor

It was my understanding that if nothing specific is returned from a python function, the interpreter sets the return value to None, right?

This comment has been minimized.

Copy link
@davebx

davebx Apr 28, 2017

Author Contributor

Then again, the Zen of Python, koan #2 states: Explicit is better than implicit.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 28, 2017

Member

You're right, didn't think of that, but I surely agree with the title of your last commit, explicit is indeed better than implicit!

@davebx

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2017

That one API test seems to fail every now and then for reasons unrelated to the code, don't know what's going on there.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

Right, this one is failing in other PRs as well.

@mvdbeek mvdbeek merged commit 2b09fe1 into galaxyproject:dev Apr 28, 2017

4 of 5 checks passed

api test Build finished. 275 tests run, 0 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 148 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@mvdbeek

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

We may want to target 17.01 as well, right?

@davebx davebx deleted the davebx:fix_managers branch Apr 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.