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
Done: Refactor shedtools #104
Done: Refactor shedtools #104
Conversation
This is needed when running `shed_tools test` without an explicit `--test_json` argument.
Otherwise uninstalled repos would be listed, leading to `Skipped` when you actually want to install them.
This has been deprecated in python 3.
Avoids mixups during tuple unpacking (wasn't broken here, but briefly suspected it).
if all tools are skipped or in error. Skipped tools are still tested if `--test_exisiting` is passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice, I like it in principle. I'm ambivalent on the name change of install_repositories
to install_tools
, because we only install repositories, and I'm afraid that distinction is lost with these changes. Which is sort-of OK, because nothing here actually deals with tools, and we were not very consistent from the start. We will break planemo though, but it's easy to update.
@@ -176,6 +228,10 @@ def _parser(): | |||
parser.add_argument("--get_data_managers", | |||
action="store_true", | |||
help="Include the data managers in the tool list. Requires login details") | |||
parser.add_argument("--get_all_tools", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting data managers technically doesn't need an admin key, but regular users can't see the data managers, giving the wrong impression that no data managers are installed. Maybe change line 230 to Requires admin login
ephemeris/shed_tools.py
Outdated
|
||
def log_repository_install_error(repository, start, msg, log): | ||
""" | ||
Log failed tool installations. Return a dictionary wiyh information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/wiyh/with
ephemeris/shed_tools.py
Outdated
repository_list.append(complete_repo) | ||
except (LookupError, KeyError) as e: | ||
if log: | ||
log_repository_install_error(repository, start, e.message, log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/e.message/str(message)/
ephemeris/shed_tools.py
Outdated
log.debug("\tRepository %s already installed (at revision %s)" % | ||
(repository['name'], repository['changeset_revision'])) | ||
return "skipped" | ||
elif "504" in e.message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/e.message/str(message)/
ephemeris/shed_tools.py
Outdated
if args.test_existing: | ||
to_be_tested_tools.extend(install_results.skipped_repositories) | ||
|
||
install_tool_manager.test_tools( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should wrap this in if to_be_tested_tools:
, because otherwise we start testing all tools on the instance, as you fall back to self.installed_tools()
if the tool list is empty. Think if all tools were skipped but you didn't set --test_exisiting
. Or if all repos errored you start testing all tools (irrespective of what you had in your tool_list.yml), that is counterintuitive.
I think I have implemented all those changes in rhpvorderman#3 |
Rhpvorderman refactor shedtools
Thanks for all the changes @mvdbeek. Especially for fixing the error of get-tool-list listing deleted tools. I ran into this error, but did not really understand why it was happening, and then forget about it. Thanks for all the effort on this pull request!
Yes this is quite ambivalent trough ephemeris. Since we are going to break planemo anyway, I better do that now to avoid breaking it twice. Commits coming up. |
Renamed all the tools that are actually repos to repos. I kept the Also kept the user interface the same. I can rename everything in the documentation to repositories properly, and also change the information in the parser. But I do not think that is something we should do in this pull request. |
.idea/vcs.xml
Outdated
@@ -0,0 +1,6 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to put this file into .gitignore, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Did put this in global .gitignore. But apparently got committed anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed now by recommitting changes
8690273
to
c652146
Compare
After two attempts refactoring the existing file, I am going to refactor shed tools from scratch. I will use the issue #91 as a guideline.Please do not add any functionality to shed tools before the refactoring complete. I will be pushing regularly so you can check on my progress. I will also be available on gitter.Done.
The following stuff has changed:
shed-tools update
now also accepts tool lists. It will only update tools that are already installed on the galaxy and issue a warning for other tools.--skip_install_tool_dependencies
flag is no longer functional as this is the default. Same goes for the--install_resolver_dependencies
flag. These flags are still around for backwards compatibility, but do not do anything.--install_tool_dependencies
,--skip_install_resolver_dependencies
and--skip_install_repository_dependencies
where added.The following stuff has not changed:
Stuff that needs further disccussion:
+ The CLI interface has some mentions of skipping tool dependencies and installing resolver dependencies. Skipping tool dependencies and installing resolver and repository dependencies are now the default, so these options do not make sense. They are ignored indef main
but kept for backwards compatibility. (I have broken every one's scripts already once... )I hope it is much easier to work now on
shed_tools
. Should also be a lot easier to write a testing framework now using pytest #80 .Hope this somewhat redeems myself for not being at the galaxy hackathon.