Implement support for Gitea #264
Conversation
|
The client API is generated using the latest official API schema The tool I've used is openapi-generator and the resulting code is in a separate repo which is currently locally referenced in the project. The plan is to fix all the issues in the client API (the generator is far from being perfect) and when it's done (or at least good enough for use with GRM) publish it on NuGet and replace all the references to the public package. |
|
This pull request introduces 64 alerts when merging 68c2fc2 into 1c03234 - view on LGTM.com new alerts:
|
|
@belidzs having the IVcsProvider is still likely to be needed. @akordowski has expressed an interest in adding DI to the project for other reason, and we are likely going to need that interface to allow this to happen. For what I think you want to do, I would suggest both an interface, and an abstract base class (with virtual methods). That way, we can provide default implementations of the internal methods, but which can be overridden when required. Does that make sense? Would be good to get some thoughts from @AdmiringWorm and @akordowski here. |
|
@gep13 you're right it makes sense to use interfaces for that. I've recently started using DI in .NET Core and I really love the concept. I've already started extracting some common stuff into the abstract class, however I have a feeling that probably I should use AutoMapper and pass complex model objects to functions instead of raw properties (e.g. a Can you take a look at a6447c5 and confirm that this is the direction you had in mind regarding the inheritance? |
|
I was more thinking along the lines of:
And then only having the overrideable methods in the VcsProviderBase class. |
|
Wouldn't that be redundant? |
|
VcsProviderBase doesn't need to inherit from IVcsProvider. The base class will only contain the methods that were private in the GitHubVcsProvider, with the default implementation, that can then be overridden if/when required. |
| public virtual async Task CreateLabelsAsync(string owner, string repository) | ||
| { | ||
| if (Configuration.Labels.Any()) | ||
| { | ||
| Logger.Verbose("Removing existing labels"); | ||
| await DeleteExistingLabelsAsync(owner, repository).ConfigureAwait(false); | ||
| Logger.Verbose("Creating new standard labels"); | ||
| var newLabelTasks = new List<Task>(); | ||
| foreach (var label in Configuration.Labels) | ||
| { | ||
| newLabelTasks.Add(CreateLabelAsync(owner, repository, label)); | ||
| } | ||
|
|
||
| await Task.WhenAll(newLabelTasks).ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
| Logger.Warning("No labels defined"); | ||
| } | ||
| } | ||
|
|
||
| protected abstract Task DeleteExistingLabelsAsync(string owner, string repository); | ||
|
|
||
| protected abstract Task CreateLabelAsync(string owner, string repository, LabelConfig label); |
There was a problem hiding this comment.
This is an example of a public default implementation of an interface method which calls for two abstract protected methods.
It's completely possible that it is somehow anti-pattern, however it reduces redundant code quite a bit
We don't need to have an interface even when implementing DI. A abstract class would be just as valid instead of an interface. I don't care either way if there is an interface or an abstract base class that is being used. |
|
This pull request introduces 64 alerts when merging 293164c into 1c03234 - view on LGTM.com new alerts:
|
|
@belidzs I'm currently at the implementaion of the DI. I wouldn't put much effort into the VCsProvider before we have a stable and functioning DI implementaion. So you would avoid to fix the merge conflicts later. |
|
@akordowski thanks for the heads up, I wasn't aware that there was anything else going on so I have already done some heavy refactoring on the GitHubProvider class. I hope I'll be able to escape from the merge hell after you've finished. Can you let me know if you're done with |
|
@belidzs I will publish a draft PR the comming days so we can discuss the next step. I will inform you then. |
|
Cheers @akordowski |
|
Is it safe to start working on this again? |
|
Actually, yes. There are some pending PRs so at the end there might be some merge conflicts. But the refactorings for DI are already merged into the |
|
Any updates on this? |
|
Sorry, but no as I don't use GitReleaseManager anymore |
This is the PR to track the development of support for Gitea.
Description
It adds support specifically for Gitea by implementing GiteaProvider (an IVcsProvider descendant) but also expands the main code by adding capabilities to choose between and configure custom providers.
Related Issue
Issue #252
Motivation and Context
See linked issue
How Has This Been Tested?
It's currently manually tested against https://try.gitea.io which is a free and publicly available sandbox instance of Gitea.
Screenshots (if appropriate):
Checklist: