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

Make GitProvider better suited for the GitHub implementation #37

Open
MartinKanters opened this issue Jul 12, 2023 · 2 comments
Open

Make GitProvider better suited for the GitHub implementation #37

MartinKanters opened this issue Jul 12, 2023 · 2 comments

Comments

@MartinKanters
Copy link
Contributor

MartinKanters commented Jul 12, 2023

As I am creating the GitHub implementation, I'm finding multiple flaws in the current interfaces which makes certain things impossible.
Therefor I would like to propose some changes. I figured this could be a place for discussion.

In my opinion we can already merge the first PR (decouple from Azure DevOps) whenever you approve, and then we can follow it up with one that contains the outcome of this discussion. I think it will keep the changes clearer and the PRs smaller.


Summary

I've checked which current interface methods are compatible with GitHub and which aren't. I've added proposed solutions for the latter. At the bottom I've wrote down what I think about how to handle singular versus multiple Git implementations.

GitProvider interface

Method Compatible Notes Proposal
getAllPullRequests() Yes -
getPullRequestsNotReviewedByUser() Kind of By looking up all pull requests, followed by specific api calls per PR. n+1 Not support this in GH (the -n flag in git-pullrequest)
getPullRequestById(id) No A repository name is required here PRs should always be tied to repositories, so make it getPullRequestById(repository: String, id)
createPullRequest(repository, sourceRefName, targetRefName, title, description) ? TBD
getAllRepositories() Yes -
getRepositoryById(repository) Yes -
getAllRefs(repository, filter) Yes We only use this to get all branches. In AzDo you can only do this using the getRefs API and filtering on "heads/". This is AzDo functionality, so should be in the AzDo provider. Rename to getAllBranches(repository)
getAllPipelines() Kind of We could loop through all repositories then fetch their pipelines, n+1 requests Not support this, but introduce an optional repository: String? parameter
getPipelineRuns(pipelineId) No Pipelines are tied to repositories Not support this, but introduce an optional repository: String? parameter

GitUrlFactory interface

Method Compatible Notes Proposal
repository(name) Yes -
pipelineRun(id) No Pipelines are tied to repositories Introduce repository: String? param
pipeline(id) No Pipelines are tied to repositories. Introduce repository: String? param
pipelineDashboard() No There is no project-wide pipeline dashboard Don't support this, or perhaps introduce the repository: String? parameter (would enable users to go to the pipeline dashboard of their context-aware project for example)
pullRequest(repositoryName, id) Yes -
pullRequestCreate(repositoryName, sourceRef) Yes -

Selecting multiple providers per command or not

In general I think that for most of the list commands (autocompletion) we should combine the results of both GitProviders (if they are configured).
For actions (create PR, open URL), we should of course open the target from the right GitProvider. For this, we would need to improve the autogeneration keys to include the git provider and the repository name: ret git pr open github/users-api/136274
If at a later stage we would support multiple implementations per provider, we could extend this to github/<org>/users-api/136274.

@MartinKanters
Copy link
Contributor Author

Another improvement could be to add a GitProviderProperties class which adds information about the features that a GitProvider supports. For example whether pipelines are always tied to repositories or not (no for AzDo, yes for GitHub).

The alternative for finding this out is to try a method (for example pipelineRun(repositoryName: String?, id) without supplying a repositoryName, and if the GitProvider throws a NotSupportedException try it again with the repository (if available). Not so nice

@MartinKanters
Copy link
Contributor Author

Here is some draft work for these interface changes. It's working pretty well so far. You can see here how the GitHub implementation is going so far.

I want to complete this work before actually merging anything, but feel free of course to give opinions on the interface changes.

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

No branches or pull requests

1 participant