NotificationsYou must be signed in to change notification settings
Fork673
Star2.4k

fix:`mr.cancel_merge_when_pipeline_succeeds()`#2350

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nejch merged 1 commit intomainfromjlvillal/cancel_merge

Oct 31, 2022

Merged

fix:`mr.cancel_merge_when_pipeline_succeeds()`#2350

nejch merged 1 commit intomainfromjlvillal/cancel_merge

Oct 31, 2022

Conversation

Copy link

Member

JohnVillalovos commentedOct 30, 2022

Call was incorrectly using aPUT method when should have used aPOST method.
Changed return type to adict as GitLab only returns {'status': 'success'} on success. Since the function didn't work previously, this should not impact anyone.
Updated the test fixturemerge_request to add ability to create a pipeline.
Added functional test formr.cancel_merge_when_pipeline_succeeds()

Fixes:#2349

JohnVillalovos force-pushed thejlvillal/cancel_merge branch from4184511 toa07a94eCompare

October 30, 2022 19:31

Copy link

codecov-commenter commentedOct 30, 2022•
edited
Loading

Codecov Report

Merging#2350 (19ee535) intomain (f04e8ba) willincrease coverage by0.09%.
The diff coverage is100.00%.

@@            Coverage Diff             @@##             main    #2350      +/-   ##==========================================+ Coverage   95.78%   95.88%   +0.09%==========================================  Files          79       79                Lines        5249     5248       -1     ==========================================+ Hits         5028     5032       +4+ Misses        221      216       -5

Flag	Coverage Δ
api_func_v4	`83.47% <100.00%> (+0.09%)`	⬆️
cli_func_v4	`82.73% <33.33%> (+0.01%)`	⬆️
unit	`87.76% <33.33%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

Impacted Files	Coverage Δ
gitlab/v4/objects/merge_requests.py	`88.88% <100.00%> (+3.37%)`	⬆️

JohnVillalovos requested a review fromnejch

October 30, 2022 19:55

nejch requested changes

Oct 30, 2022

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

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

Thanks@JohnVillalovos nice catch with the response gotcha! I think we just need to update the docstring as well.

Interesting,GitlabMROnBuildSuccessError also sounds a bit weird to me now here, could be there are some historical reasons for discrepancies here.

gitlab/v4/objects/merge_requests.pyShow resolvedHide resolved

tests/functional/conftest.pyShow resolvedHide resolved

tests/functional/api/test_merge_requests.pyShow resolvedHide resolved

JohnVillalovos force-pushed thejlvillal/cancel_merge branch froma07a94e to19ee535Compare

October 30, 2022 22:11

JohnVillalovos requested a review fromnejch

October 30, 2022 22:12

lmilbaum reviewed

Oct 31, 2022

View reviewed changes

tests/functional/api/test_merge_requests.pyShow resolvedHide resolved

lmilbaum reviewed

Oct 31, 2022

View reviewed changes

tests/functional/conftest.pyShow resolvedHide resolved

nejch mentioned this pull request

Oct 31, 2022

Split resource fixtures and factories#2353

Open

nejch approved these changes

Oct 31, 2022

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

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

Thanks@JohnVillalovos for the quick fix and@lmilbaum for the review, let's tackle the follow-up separately :)

fix:mr.cancel_merge_when_pipeline_succeeds()

cd31cda

  * Call was incorrectly using a `PUT` method when should have used a    `POST` method.  * Changed return type to a `dict` as GitLab only returns    {'status': 'success'} on success. Since the function didn't work    previously, this should not impact anyone.  * Updated the test fixture `merge_request` to add ability to create    a pipeline.  * Added functional test for `mr.cancel_merge_when_pipeline_succeeds()`Fixes:#2349

nejch force-pushed thejlvillal/cancel_merge branch from19ee535 tocd31cdaCompare

October 31, 2022 10:12

nejchenabled auto-merge (squash)

October 31, 2022 10:16

lmilbaum approved these changes

Oct 31, 2022

View reviewed changes

nejch merged commitbd82d74 intomain

Oct 31, 2022

nejch deleted the jlvillal/cancel_merge branch

October 31, 2022 10:24

Labels

None yet

4 participants

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix:`mr.cancel_merge_when_pipeline_succeeds()`#2350

fix:`mr.cancel_merge_when_pipeline_succeeds()`#2350

Uh oh!

Conversation

JohnVillalovos commentedOct 30, 2022

Uh oh!

codecov-commenter commentedOct 30, 2022•
edited
Loading

Uh oh!

Codecov Report

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Movatterモバイル変換

fix:mr.cancel_merge_when_pipeline_succeeds()#2350

fix:mr.cancel_merge_when_pipeline_succeeds()#2350

Uh oh!

Conversation

JohnVillalovos commentedOct 30, 2022

Uh oh!

codecov-commenter commentedOct 30, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fix:`mr.cancel_merge_when_pipeline_succeeds()`#2350

fix:`mr.cancel_merge_when_pipeline_succeeds()`#2350

codecov-commenter commentedOct 30, 2022•
edited
Loading