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

Add API for get single merge request, create note on merge request, get pipelines for merge request, delete for webhooks #137

Merged
merged 14 commits into from
May 30, 2020

Conversation

saber-wang
Copy link
Contributor

@saber-wang saber-wang commented Apr 14, 2020

Get single MR And Add MergeRequestDetail
Add List MR pipelines
Create Merge Request note request

@jetersen
Copy link
Collaborator

@saber-wang test failed: https://github.com/nmklotas/GitLabApiClient/pull/137/checks?check_run_id=585099995#step:9:90

What prevents on from extending MergeRequest to have the additional info.

Yes some of it might be empty when requesting multiple MRs, however I do not see a harm in that.

@saber-wang saber-wang closed this Apr 14, 2020
@jetersen
Copy link
Collaborator

no need to close the PR, you can just push your changes to the branch

@saber-wang
Copy link
Contributor Author

test failed: https://github.com/nmklotas/GitLabApiClient/pull/137/checks?check_run_id=585099995#step:9:90

I don't know why the test failed

What prevents on from extending MergeRequest to have the additional info.

Yes some of it might be empty when requesting multiple MRs, however I do not see a harm in that.

I'm just worried about misdirection

@saber-wang saber-wang reopened this Apr 14, 2020
@codecov-io
Copy link

codecov-io commented Apr 14, 2020

Codecov Report

Merging #137 into master will decrease coverage by 0.42%.
The diff coverage is 17.85%.

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
- Coverage   71.59%   71.16%   -0.43%     
==========================================
  Files         162      164       +2     
  Lines        2299     2362      +63     
==========================================
+ Hits         1646     1681      +35     
- Misses        653      681      +28     
Impacted Files Coverage Δ
...ent/Models/MergeRequests/Responses/MergeRequest.cs 86.79% <0.00%> (-1.67%) ⬇️
...ls/Notes/Requests/CreateMergeRequestNoteRequest.cs 0.00% <0.00%> (ø)
.../Models/Pipelines/Requests/PipelineQueryOptions.cs 100.00% <ø> (ø)
...abApiClient/Models/Pipelines/Responses/Pipeline.cs 0.00% <0.00%> (ø)
src/GitLabApiClient/MergeRequestsClient.cs 55.35% <11.11%> (-7.81%) ⬇️
.../Models/Pipelines/Requests/PipelineQueryBuilder.cs 68.96% <25.00%> (-8.12%) ⬇️
src/GitLabApiClient/Models/PageQuery.cs 100.00% <100.00%> (ø)
... and 2 more

@jetersen
Copy link
Collaborator

@saber-wang the test failure is pretty clear 🤔

[xUnit.net 00:03:32.31]       System.InvalidCastException : Unable to cast object of type 'GitLabApiClient.Models.MergeRequests.Responses.MergeRequest' to type 'GitLabApiClient.Models.MergeRequests.Responses.MergeRequestDetail'.
[xUnit.net 00:03:32.31]       Stack Trace:
[xUnit.net 00:03:32.31]            at FluentAssertions.Primitives.ReferenceTypeAssertions`2.Match[T](Expression`1 predicate, String because, Object[] becauseArgs)
[xUnit.net 00:03:32.31]         /home/runner/work/GitLabApiClient/GitLabApiClient/test/GitLabApiClient.Test/MergeRequestClientTest.cs(38,0): at GitLabApiClient.Test.MergeRequestClientTest.CreatedMergeRequestCanBeRetrieved()

@saber-wang saber-wang changed the title Get single MR Add more Api Apr 17, 2020
Copy link
Collaborator

@jetersen jetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit otherwise LGTM

src/GitLabApiClient/PipelineClient.cs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2020

Codecov Report

Merging #137 into master will decrease coverage by 0.35%.
The diff coverage is 5.26%.

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
- Coverage   71.19%   70.84%   -0.36%     
==========================================
  Files         169      170       +1     
  Lines        2350     2404      +54     
==========================================
+ Hits         1673     1703      +30     
- Misses        677      701      +24     
Impacted Files Coverage Δ
...ent/Models/MergeRequests/Responses/MergeRequest.cs 86.79% <0.00%> (-1.67%) ⬇️
...ls/Notes/Requests/CreateMergeRequestNoteRequest.cs 0.00% <0.00%> (ø)
...abApiClient/Models/Pipelines/Responses/Pipeline.cs 0.00% <0.00%> (ø)
src/GitLabApiClient/WebhookClient.cs 41.66% <0.00%> (-3.79%) ⬇️
src/GitLabApiClient/MergeRequestsClient.cs 51.66% <11.11%> (-8.34%) ⬇️

Comment on lines 1 to 12
using System;
using System.Collections.Generic;
using System.Text;

namespace GitLabApiClient.Models
{
public class PageQuery
{
public int? Page { get; set; }
public int? Per_Page { get; set; }
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to delete it. I'll fix it

@jetersen jetersen changed the title Add more Api Add API for get single merge request, create note on merge request, get pipelines for merge request, delete for webhooks May 30, 2020
@jetersen jetersen merged commit 15d4adf into nmklotas:master May 30, 2020
MindaugasLaganeckas pushed a commit to MindaugasLaganeckas/GitLabApiClient that referenced this pull request Mar 9, 2021
…et pipelines for merge request, delete for webhooks (nmklotas#137)

Co-Authored-By: Joseph Petersen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants