(GH-181) Add Dependency Injection

[akordowski]

This PR addresses #181.

[akordowski]

@gep13 @AdmiringWorm This is the first draft PR. I made so little invasive changes as possible. But there are some problems I found on the way.

• I wanted to pass the ReleaseNotesBuilder and the ReleaseNotesExporter to the GitHubProvider over the constructor using a interface, instead of initializing them directly in the GitHubProvider. But we have here a circular dependency. Both of the classes need a IVcsProvider instance. I think we need to refactor the classes.
• To have more advantage of DI I would also like to extract the command methods in Spectre.Cli like Commands. This way we colud also cover them with unit tests.
• We should also discuss if we want a same DI initialization like the GitVersion project.

Please share also your ideas what we can make better. Thanks!

[akordowski]

I've been making some thoughts about the implementation and I think we need to do some refactoring. Currently the GitHubProvider has some tight coupling which we should break up. Here some ideas:

VcsProvider

A VcsProvider should only contain logic to communicate with the VCS Api and return only mapped GRM models. This make possible to create providers for different VCS.

class GitHubProvider : IVcsProvider
	GitHubProvider(IGitHubClient client, IMapper mapper)

VcsService

A VcsService would consume the VcsProvider and contain only business logic that is related to the VCS. The VcsService should be decoupled from ReleaseNotesBuilder and ReleaseNotesExporter (currently in the GitHubProvider). Instead it should only return a object which contains the necessary data to create release notes.

class VcsService
	VcsService(IVcsProvider provider, ILogger logger)

	Task<Release> CreateRelease(..., string releaseNotes)
	Task<ReleaseData> GetReleaseData(string milestone)


class ReleaseData
	Release Release
	IEnumerable<Issue> Issues
	string CommitsLink
	int CommitsCount

Release Notes

Currrently the ReleaseNotesBuilder and the ReleaseNotesExporter are couple with the IVcsProvider. Instead the classes should only recieve a object with the release notes data. This would be useful for the ReleaseNotesExporter, which currently reads the already created release notes. So it is not possible to export the release notes in other format.

class ReleaseNotesBuilder
	ReleaseNotesBuilder(ILogger logger, Config config)

	Task<string> BuildReleaseNotesAsync(ReleaseData releaseData, string releaseNotesTemplate, Dictionary<string, string> variables)


class ReleaseNotesExporter
	ReleaseNotesExporter(ILogger logger)

	Task ExportReleaseNotesAsync(string releaseNotes, string outputFile)

This would reduce complexity (a class/function should do only one thing) and increase testability. What dou you think?

[gep13]

@akordowski all of these suggestions seem like sensible ones to me.

[akordowski]

@gep13 Do I have your permission to implement it this way?

[gep13]

@akordowski yes, please feel free to continue, and we can review as you are making progress.

[akordowski]

@gep13 It seems as if the environment variables for the IntegrationTests aren't properly set. It throws an error on creating the GitHubClient credentials, which breaks the build. Could you take a look at it? Thank you!

[AdmiringWorm]

@akordowski the necessary environment variables are marked as secret, as such they can not be used when running them on pull requests unfortunately.

I would say, ignore the errors for missing environment variables in this case.

[akordowski]

@AdmiringWorm Wouldn't you say the pipeline is broken in this case? Because of the failing IntegrationTests the regular unit test and other checks are not executed. IMO this should be fixed.

[AdmiringWorm]

@akordowski and how do you suggest to fix it?
Remember, we can't make the environment variables available as that means making them public, which would be a security liability and can cause a users password or PAT token to be publically displayed (potentially).

[akordowski]

@AdmiringWorm At my previous PRs there were no problems with the IntegrationTests. The build always has run through. I've no idea why it breaks in this case. A possible fix could be to add a conditon in the pipeline that other test are not dependent on the integration tests.

[codecov-commenter]

Codecov Report

Merging #265 into develop will increase coverage by 31.77%.
The diff coverage is 83.52%.

@@             Coverage Diff              @@
##           develop     #265       +/-   ##
============================================
+ Coverage    45.43%   77.20%   +31.77%     
============================================
  Files           27       54       +27     
  Lines          799     1132      +333     
  Branches       135      135               
============================================
+ Hits           363      874      +511     
+ Misses         418      237      -181     
- Partials        18       21        +3     

Impacted Files	Coverage Δ
...ource/GitReleaseManager/Exceptions/ApiException.cs	25.00% <0.00%> (ø)
...GitReleaseManager/Exceptions/ForbiddenException.cs	0.00% <0.00%> (ø)
...GitReleaseManager/MappingProfiles/GitHubProfile.cs	0.00% <0.00%> (-87.50%)	⬇️
Source/GitReleaseManager/Model/IssueComment.cs	0.00% <0.00%> (ø)
Source/GitReleaseManager/Model/Label.cs	33.33% <0.00%> (-66.67%)	⬇️
Source/GitReleaseManager/Model/RateLimit.cs	0.00% <0.00%> (ø)
Source/GitReleaseManager/OctokitExtensions.cs	0.00% <ø> (ø)
...ce/GitReleaseManager/Options/AddAssetSubOptions.cs	100.00% <ø> (ø)
Source/GitReleaseManager/Options/BaseSubOptions.cs	20.00% <ø> (ø)
...rce/GitReleaseManager/Options/BaseVcsSubOptions.cs	40.00% <ø> (ø)
... and 62 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d53566...711d6ca. Read the comment docs.

[lgtm-com]

This pull request introduces 1 alert when merging e0d53c2 into 3d53566 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

[lgtm-com]

This pull request introduces 2 alerts and fixes 1 when merging 1d01178 into 3d53566 - view on LGTM.com

new alerts:

• 2 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 1f295f5 into 3d53566 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging edd9ace into 3d53566 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging c3ae4a1 into 3d53566 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 39ace71 into 3d53566 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[akordowski]

@gep13 @AdmiringWorm Oh boy, what a ride! There are still some points I would like to refactor though, but I think the PR is in a good state for review.

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 05700d0 into 3d53566 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[AdmiringWorm]

I won't be able to do a review until the weekend, but I will keep this on my radar then.

[akordowski]

No rush, as your time allows it. Thank you!

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging aff08ac into 3d53566 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[gep13]

@akordowski I am reviewing this PR just now on my Twitch stream, and have just rebased the PR onto the head of the develop branch. Got a couple other little things to look at as well, just wanted to give you a heads up...

[akordowski]

@gep13 Sounds great, thank you!

[mkevenaar]

@akordowski if you have time and want to follow along, please go to https://www.twitch.tv/gep13

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 844ecb4 into b01de39 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 711d6ca into b01de39 - view on LGTM.com

new alerts:

• 1 for Dereferenced variable may be null

fixed alerts:

• 1 for Dereferenced variable may be null

[gep13]

LGTM!

[gep13]

@akordowski huge thank you for all the work that you have done in this PR, really appreciate it!