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

issue #29, #31, #32, #35 #37

Merged
merged 5 commits into from
Aug 14, 2020
Merged

issue #29, #31, #32, #35 #37

merged 5 commits into from
Aug 14, 2020

Conversation

22PoojaGaur
Copy link
Member

@22PoojaGaur 22PoojaGaur commented Jul 19, 2020

This PR is for #29, #31, #32, #35
if datapath is provided, the biotestmine from that path is moved and used for data

if datapath is provided, the biotestmine from that path is moved and used for data
@22PoojaGaur
Copy link
Member Author

@uosl

In this PR, I copy the user provided mine folder to biotestmine folder and then run the containers as usual.

The other way would have been to load user provided folder for biotestmine volumes and ~/.local/share/intermine_boot/.. (for linux) folder for other volumes. I did not take this approach as folders for the mine would be located at different places following this. Also, we archive the data from one path so keeping folders there makes more sense.

@22PoojaGaur 22PoojaGaur changed the title issue #29 issue #29 and issue #31 Jul 19, 2020
intermine_boot/intermine_docker.py Outdated Show resolved Hide resolved
intermine_boot/intermine_docker.py Outdated Show resolved Hide resolved
@heralden
Copy link
Member

Like we discussed, I agree with Ank and think we should focus on handling #30 in this PR as well (the current approach might make supporting it harder).

@22PoojaGaur 22PoojaGaur changed the title issue #29 and issue #31 issue #29, #31, #32, #35 Jul 22, 2020
@22PoojaGaur
Copy link
Member Author

@uosl @leoank

I have addressed the changes but it's not ready to test. There is some error I am resolving.

I will update here when it is ready.

@22PoojaGaur
Copy link
Member Author

@uosl @leoank This is ready for review.

Copy link
Member

@heralden heralden left a comment

Choose a reason for hiding this comment

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

This looks really good! 💯

Good idea to handle #30 separately. I will look more into the mine property file to decide how we should generate one.

I have one comment about the IM_DATA_FROM_PATH envvar.

intermine_boot/intermine_docker.py Outdated Show resolved Hide resolved
Copy link
Member

@heralden heralden left a comment

Choose a reason for hiding this comment

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

Great job! 👍

Sorry for being late to review. 😅

@heralden heralden merged commit 66f152d into intermine:master Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants