Update modules install to support version control #1116
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1116 +/- ##
==========================================
- Coverage 70.36% 70.02% -0.34%
==========================================
Files 36 38 +2
Lines 4545 4748 +203
==========================================
+ Hits 3198 3325 +127
- Misses 1347 1423 +76
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Really nice! This looks already pretty good.
The main suggestion I have here is to make the code robuster and surround especially the GitHub API call with a try-except and try to capture any errors that you expect.
You might also do this when reading/writing the json file, just to be on eht esafe side. But definitely for the api call.
|
Ah and one more thing we should add some logging to inform the user that e.g. a |
Co-authored-by: Kevin Menden <kevin.menden@live.com>
Co-authored-by: Kevin Menden <kevin.menden@live.com>
There was a problem hiding this comment.
@ErikDanielsson tested this a bit and I think this is really nice!
So feel free to remove the draft state of that PR.
|
Okay did it myself 😁 |
Co-authored-by: Kevin Menden <kevin.menden@live.com>
…hout interactive prompt
|
It works for me when trying it on |
|
The latest commit addresses that the directory structure of the modules repo was changed with nf-core/modules#80. Hence commits prior to this PR should not be fetched. |
nf_core/__main__.py
Outdated
| @click.option("-t", "--tool", type=str, metavar="<tool> or <tool/subtool>") | ||
| def install(ctx, pipeline_dir, tool): | ||
| @click.option("-l", "--latest", is_flag=True, default=False, help="Install the latest version of the module") | ||
| @click.option("--force", is_flag=True, default=False, help="Force installation of module if module already exists") | ||
| def install(ctx, pipeline_dir, tool, latest, force): |
There was a problem hiding this comment.
We need an option to specify the sha here, e.g. -s, --sha as discussed on slack
nf_core/modules/pipeline_modules.py
Outdated
| if not force: | ||
| log.error( | ||
| "Module directory already exists but module {} is not present in 'module.json'".format(module_dir) | ||
| ) | ||
| return False |
There was a problem hiding this comment.
Here it would be nice to ask the user whether to overwrite (we should have something similar somewhere else, I think nf-core modules create-test-yml).
Basically a simple question whether to force overwrite and if yes, set 'force' to true.
|
All options are added now, I think, but they still need thorough testing. I think we will have to refactor |
There was a problem hiding this comment.
Hi @ErikDanielsson , nice work!
Just a couple of minor comments.
I've tested around with it a bit and we need to add the option to update all modules, as we won't include the nf-core modules update command now. But I would actually like to treat this as a separate PR and get this one here merged now, so we can continue working on the features.
So I'm approving it now and then I say we get this one merged in! We might find some other issues along the way but can then still solve them before the release.
Nice work!
| if self.latest and self.sha is not None: | ||
| log.error("Cannot use '--sha' and '--latest' at the same time!") |
There was a problem hiding this comment.
we should probably exit here
Co-authored-by: Kevin Menden <kevin.menden@live.com>
…ols into update-modules-install
Updated
nf-core modules installcommand to accommodate version control for modules, as suggested in #868.--latestinstalls the latest version of the module.--forceoverwrites the installed version of the module--sha <commit sha>to install specific version of module.questionary selectfor interactive selection of versionPR checklist
CHANGELOG.mdis updateddocsis updated