GitReleaseManager icon indicating copy to clipboard operation
GitReleaseManager copied to clipboard

Implement support for Gitea

Open belidzs opened this issue 5 years ago • 15 comments

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:

  • [ ] My code follows the code style of this project.
  • [x] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.

belidzs avatar Jul 31 '20 09:07 belidzs

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.

belidzs avatar Jul 31 '20 09:07 belidzs

This pull request introduces 64 alerts when merging 68c2fc21bc2fb948cab4db51a81ec05dd7423d48 into 1c032346739e85e2d36b1bdfbd68c26217b81189 - view on LGTM.com

new alerts:

  • 64 for Use of default ToString()

lgtm-com[bot] avatar Jul 31 '20 10:07 lgtm-com[bot]

@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 avatar Jul 31 '20 12:07 gep13

@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 Model.Issue instead of index), but I'm not sure which one is the better solution.

Can you take a look at a6447c5 and confirm that this is the direction you had in mind regarding the inheritance?

belidzs avatar Jul 31 '20 12:07 belidzs

I was more thinking along the lines of:

public class GiteaProvider : VcsProviderBase, IVcsProvider

And then only having the overrideable methods in the VcsProviderBase class.

gep13 avatar Jul 31 '20 12:07 gep13

Wouldn't that be redundant? VcsProviderBase already has to inherit from the interface since it provides a default implementation of CreateLabels()

belidzs avatar Jul 31 '20 12:07 belidzs

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.

gep13 avatar Jul 31 '20 12:07 gep13

@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.

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.

AdmiringWorm avatar Jul 31 '20 12:07 AdmiringWorm

This pull request introduces 64 alerts when merging 293164cb8087137400718026168160173ae00091 into 1c032346739e85e2d36b1bdfbd68c26217b81189 - view on LGTM.com

new alerts:

  • 64 for Use of default ToString()

lgtm-com[bot] avatar Jul 31 '20 12:07 lgtm-com[bot]

@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 avatar Jul 31 '20 13:07 akordowski

@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 GitHubProvider and IVcsProvider?

belidzs avatar Jul 31 '20 14:07 belidzs

@belidzs I will publish a draft PR the comming days so we can discuss the next step. I will inform you then.

akordowski avatar Jul 31 '20 14:07 akordowski

Cheers @akordowski

belidzs avatar Jul 31 '20 14:07 belidzs

Is it safe to start working on this again?

belidzs avatar Oct 09 '20 11:10 belidzs

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 develop branch.

akordowski avatar Oct 09 '20 12:10 akordowski