WIP: support gitlab as git provider #96
Conversation
|
@agokrani Just wanted to check: is this still WIP or is it ready for review? |
|
@iuliaturc this is still WIP. It just require the default branch test. I will try to add it early tomorrow. |
|
@iuliaturc you can review it now. |
iuliaturc
left a comment
iuliaturc
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I'm wondering if it's worth merging the two RepoManagers into one (with some occasional if/else blocks) to avoid so much duplication.
| help="The local directory to store the repository", | ||
| ) | ||
| parser.add("--git-provider", default="github", choices=["github", "gitlab"]) | ||
| parser.add("--base-url", default=None, help="The base URL for the Git provider. This is only needed for GitLab.") |
There was a problem hiding this comment.
Could you please name the flag --gitlab-base-url to avoid confusion?
| return repo_manager | ||
|
|
||
|
|
||
| class GitLabRepoManager(DataManager): |
There was a problem hiding this comment.
This new class introduces a lot of code duplication. IIUC, the only difference compared to GitHubRepoManager is how URLs are constructed. Is that correct? In that case, we could have a single unified GitRepoManager that takes a base URL (which would be github.com for GitHub and gitlab.com for GitLab). And maybe we'd need occasional if/else for e.g. url_for_file.
There was a problem hiding this comment.
Sure! Should be possible but I don't think getting different if else would be a good idea if you are incorporating other git providers as well. I haven't looked into other providers maybe something to check before we merge them into one class?
There was a problem hiding this comment.
GitHub and GitLab are the main ones, I'm not sure if there's a third one as widely used.
Alternative to if/else clauses, we could have an abstract class GitRepoManager that is implemented by GitHubRepoManager and GitLabRepoManager. For instance:
class GitRepoManager:
@abstractclass
def get_repo_url(self, repo_id: str):
pass
@cached_property
def is_public(self) -> bool:
"""Checks whether a GitHub repository is publicly visible."""
response = requests.get(self.get_repo_url(repo_id), timeout=10)
# Note that the response will be 404 for both private and non-existent repos.
return response.status_code == 200
... other methods
class GitHubRepoManager(GitRepoManager):
def get_repo_url(self, repo_id: str):
return f"https://api.github.com/repos/{self.repo_id}"
...
class GitLabRepoManager(GitRepoManager):
def get_repo_url(self, repo_id: str):
return f"https://gitlab.com/api/v4/projects/{repo_id}"
...
Does that make sense?
3 participants
What does this PR do?
Implements GitLabManager class to support other git providers. Have also added other flags mentioned in issue #10.