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

modules list doesn't work for custom remotes with a subproject #1721

Closed
awgymer opened this issue Aug 2, 2022 · 10 comments · Fixed by #1724
Closed

modules list doesn't work for custom remotes with a subproject #1721

awgymer opened this issue Aug 2, 2022 · 10 comments · Fixed by #1724
Assignees
Labels
bug Something isn't working

Comments

@awgymer
Copy link
Contributor

awgymer commented Aug 2, 2022

Description of the bug

Our remote repo has the format <organisation>/<subgroup>/<modules> and this works fine when installing modules from the remote.

The content of our modules.json as generated by nf-core modules install is:

{
    "name": "",
    "homePage": "",
    "repos": {
        "organisation/subgroup/modules": {
            "base_path": "modules",
            "git_url": "https://gitlab.com/organisation/subgroup/modules",
            "modules": {
                "mosdepth": {
                    "git_sha": "<sha>"
                },
                "samtools/merge": {
                    "git_sha": "<sha>"
                }
            }
        }
    }
}

I think this is mainly an unsupported part of GitLab, and looking at the code there are a lot of assumptions that repos follow the GitHub pattern of owner/name so accommodating this may be overly onerous.
Perhaps there is a workaround though?

Command used and terminal output

nf-core modules list local 

CRITICAL You 'modules.json' file is not up to date. Please remove it and rerun the command

System information

nf-core/tools version 2.5.dev0
python 3.9.7

@awgymer awgymer added the bug Something isn't working label Aug 2, 2022
@awgymer
Copy link
Contributor Author

awgymer commented Aug 2, 2022

Manually modifying the path and the repo name to remove the organisation makes this work. I don't know whether this is really desirable but I suppose in the absence of any other fix, shortening the path under modules to ensure it is only one level deep could work?

This just breaks most other functionality

@awgymer
Copy link
Contributor Author

awgymer commented Aug 2, 2022

def path_from_remote(remote_url):
    """
    Extracts the path from the remote URL
    See https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-clone.html#URLS for the possible URL patterns
    """
    # Check whether we have a https or ssh url
    if remote_url.startswith("https"):
        path = urllib.parse.urlparse(remote_url)
        path = path.path
        # Remove the intial '/'
        path = path[1:]
        path = os.path.splitext(path)[0]
    else:
        # Remove the initial `git@``
        path = remote_url.split("@")
        path = path[-1] if len(path) > 1 else path[0]
        path = urllib.parse.urlparse(path)
        path = path.path
        path = os.path.splitext(path)[0]
    return '/'.join(path.split('/')[-2:])

Changing the last line of this method to only return the last 2 parts of the path "seems" to work? Whether that has undesirable side-effects I do not know. For any GitHub repo this should return the same as before.

@ErikDanielsson
Copy link
Contributor

ErikDanielsson commented Aug 2, 2022

Thank you for reporting! I think the issue is deeper than this sadly-- as you suspected large parts of the codebase still depends on that git remotes have the same directory structure as GitHub repos. The root of the issue is that in many of the commands we try to determine the available modules in a pipeline from the directory structure rather than from the modules.json. This works when the number of directories in the git remote is predictable, but fails to work in cases such as this. We should change this however and rely more on the information in the modules.json, but it will probably take some time before it is done.

@awgymer
Copy link
Contributor Author

awgymer commented Aug 2, 2022

How robust/thorough are the CI-tests for this? If I go through and try to make fixes would a passing PR tests be a good sign or is it more involved?

@ErikDanielsson
Copy link
Contributor

ErikDanielsson commented Aug 2, 2022

Most of the CI for the modules depends on that this works in one way or another, so if you mess something up the tests are likely to fail ;) But I think a good solution to this will be rather involved, so I'll assign myself to it.

@ErikDanielsson
Copy link
Contributor

@awgymer it seems like the fix was simpler than first anticipated, I've made a PR in #1724. Do you mind trying out the fixed commands?

@awgymer
Copy link
Contributor Author

awgymer commented Aug 3, 2022

That was fast! I'll try and pull those changes into my fork and have a play later today.

@ErikDanielsson ErikDanielsson linked a pull request Aug 3, 2022 that will close this issue
4 tasks
@awgymer
Copy link
Contributor Author

awgymer commented Aug 3, 2022

Is there an existent modules repo where the basepath is not modules?

@ErikDanielsson
Copy link
Contributor

No, but there should be and there should be CI for it as well. I'll create a seperate issue for it.

@ErikDanielsson
Copy link
Contributor

Closing due to merge of #1724.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants