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

Add DTD to Galaxy #2579

Merged
merged 8 commits into from
Jul 21, 2016
Merged

Add DTD to Galaxy #2579

merged 8 commits into from
Jul 21, 2016

Conversation

ericenns
Copy link
Contributor

This merges DTD upstream into Galaxy.

https://github.com/phac-nml/dynamic-tool-destination

WIP until all tests have been moved over and reviewed fully.

@bgruening
Copy link
Member

Great poster, great idea, I like it much and I think it is a nice addition to the dynamic tool destinations.
Thanks @ericenns!

@ericenns
Copy link
Contributor Author

ericenns commented Jul 4, 2016

I think WIP label can be removed, I have added all our tests in and they pass!

# Galaxy's history panel).


tools:
Copy link
Member

Choose a reason for hiding this comment

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

Should this be commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean should the whole file be commented out? If so yes, is that what is done with out sample config files?

@bgruening
Copy link
Member

@ericenns do you think we should keep the configuration under config or is this something really specific to the dynamic-job-destination and should be next to the job-destination to not pollute the config dir?

@ericenns
Copy link
Contributor Author

ericenns commented Jul 4, 2016

The config file is specific to dtd, one benefit to keeping it in config is that it gives visibility to it. I guess I could add it as an example to job_conf.xml.sample_advanced

@ericenns ericenns changed the title [WIP] Add DTD to Galaxy Add DTD to Galaxy Jul 5, 2016
@ericenns
Copy link
Contributor Author

ericenns commented Jul 5, 2016

Guess I have to remove WIP from the title or galaxybot will keep adding the label back.

@galaxybot galaxybot added this to the 16.07 milestone Jul 5, 2016
@jmchilton
Copy link
Member

@galaxybot test this

jmchilton added a commit to jmchilton/starforge that referenced this pull request Jul 20, 2016
@jmchilton
Copy link
Member

Really nifty - I ❤️ the tests and focus on validation - this great.

I created galaxyproject/starforge#100 so the wheel for testfixtures can be placed on wheels.galaxyproject.org.

I'm going to open a counter-PR to make the config handling more Galaxy-ish.

@jmchilton
Copy link
Member

The upstream project has a different copyright statement and license than Galaxy and it is featured prominently in README - given that I just wanted to make sure that you are explicitly granting the Galaxy project permission to redistribute this code with our license and copyright.

@ericenns
Copy link
Contributor Author

Yes I am granting the Galaxy project permission to redistribute this code with their license and copyright.

…tinations.

Moving rules also means it won't conflict with other peoples custom dynamic destinations and should prevent Galaxy from reloading the Python repeatedly.
@jmchilton
Copy link
Member

Thanks @ericenns!

I have two commits I'd like to discuss but Github isn't letting me open a PR against your repo and I don't understand why. Any chance you can review them, test them, and cherry-pick them into your branch if they look good?

They are from my branch here - https://github.com/jmchilton/galaxy/tree/add-dtd-to-galaxy.

jmchilton@ed48c6c Makes the config handling more like traditional Galaxy - navigating the Galaxy config object to find that YAML file.
jmchilton@d87957e Re-imagines how this would be used a bit - it makes this a special "type" of dynamic destination (there were already some others) so details such as the function name don't need to be exposed. It also adds some documentation to job_conf.xml.sample_advanced about how to use it.

Any problems with these changes?

@ericenns
Copy link
Contributor Author

Thanks @jmchilton, the changes look good to me. I will merge them and retest.

@ericenns
Copy link
Contributor Author

@jmchilton everything looks good, a revised test location and updated the sample config with a little more information on how to validate your config.

@ericenns
Copy link
Contributor Author

@galaxybot test this

@ericenns
Copy link
Contributor Author

Hmm looks like there is a problem with running the following command:

 bash -c '. .venv/bin/activate; python lib/galaxy/jobs/dynamic_tool_destination.py -c config/tool_destinations.yml.sample'

I am looking into it right now

@ericenns
Copy link
Contributor Author

Ok everything should be good to go now

@ericenns
Copy link
Contributor Author

@galaxybot test this

@jmchilton
Copy link
Member

Very nice @ericenns - thanks a bunch for this contribution!

@jmchilton jmchilton merged commit f67d3cb into galaxyproject:dev Jul 21, 2016
@jmchilton
Copy link
Member

And of course I merged this without waiting for my own starforge PR (galaxyproject/starforge#100) 😠.

@bgruening
Copy link
Member

🎉 This is awesome! Next GCC presentation in Galaxy main! Thanks a lot @ericenns!

jmchilton added a commit to jmchilton/starforge that referenced this pull request Jul 21, 2016
natefoo added a commit to galaxyproject/starforge that referenced this pull request Aug 4, 2016
jxtx pushed a commit to jxtx/starforge that referenced this pull request Oct 10, 2016
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.

None yet

5 participants