Syntax for conditionally loading toolbox items. #18004
Conversation
| @@ -408,6 +408,12 @@ def load_item( | |||
| panel_dict = self._tool_panel | |||
| if integrated_panel_dict is None: | |||
| integrated_panel_dict = self._integrated_tool_panel | |||
| load_if = item.get("if") | |||
| if load_if == "interactivetools_enable": | |||
There was a problem hiding this comment.
This seems to duplicate in the tool conf the info provided by tool_type="interactive" in the tool, not sure I like it.
There was a problem hiding this comment.
I get why you feel that way but I disagree. I feel like the toolbox says this is what should be loaded and it should be an error if you are attempting to load something that isn't configured properly. It only feels like duplicated information because we aren't serious enough about tool errors - we just ignore stuff and keep going with loading the tool panel.
There was a problem hiding this comment.
I don't think you would suggest Galaxy should not start if a tool doesn't load correctly, so "just ignore stuff and keep going with loading the tool panel" seems the right approach to me.
We could present the tool load failure errors in the admin UI instead of keeping them in the logs, but that's another discussion.
There was a problem hiding this comment.
It is what we do for every other configuration error - it is what an application should do if it is misconfigured IMO. I'm closing this.
There was a problem hiding this comment.
Yes, but these are user plugins .. say ITs are on the tool shed one day (nothing is actually stopping a "rogue" actor from uploading ITs). If an admin installs a tool and restarts the server it won't restart. That is a heavy price to pay for forcing that the application config is correct.
There was a problem hiding this comment.
We're never going to load all user defined tools into a toolbox like this and these are admin configured tools. The file I updated in this PR is literally hand crafted.
Alternative to #17998. I've always been annoyed by this message in my local dev logs - seems to be a better fix? The syntax only allows one thing right now but can be parameterized in the future in obvious ways if we decide we need more conditions.
How to test the changes?
(Select all options that apply)
License