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

fix: remove default arguments for mergerequests.merge()#1818

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/merge_request_merge_defaults

Jan 8, 2022

Merged

fix: remove default arguments for mergerequests.merge()#1818

nejch merged 1 commit intomainfromjlvillal/merge_request_merge_defaults

Jan 8, 2022

+7 −5

Conversation

Copy link

Member

JohnVillalovos commentedJan 8, 2022

The argumentsshould_remove_source_branch and
merge_when_pipeline_succeeds are optional arguments. We should not
be setting any default value for them.

https://docs.gitlab.com/ee/api/merge_requests.html#accept-mr

Closes:#1750

JohnVillalovos requested a review fromnejch

January 8, 2022 22:46

JohnVillalovos force-pushed thejlvillal/merge_request_merge_defaults branch fromadc9c7c tod76152eCompare

January 8, 2022 22:47

fix: remove default arguments for mergerequests.merge()

8e589c4

The arguments `should_remove_source_branch` and`merge_when_pipeline_succeeds` are optional arguments. We should notbe setting any default value for them.https://docs.gitlab.com/ee/api/merge_requests.html#accept-mrCloses:#1750

JohnVillalovos force-pushed thejlvillal/merge_request_merge_defaults branch fromd76152e to8e589c4Compare

January 8, 2022 23:08

Copy link

codecov-commenter commentedJan 8, 2022

Codecov Report

Merging#1818 (8e589c4) intomain (22a1516) willnot change coverage.
The diff coverage is50.00%.

@@           Coverage Diff           @@##             main    #1818   +/-   ##=======================================  Coverage   92.01%   92.01%           =======================================  Files          76       76             Lines        4799     4799           =======================================  Hits         4416     4416             Misses        383      383

Flag	Coverage Δ
cli_func_v4	`81.39% <50.00%> (-0.03%)`	⬇️
py_func_v4	`80.07% <50.00%> (ø)`
unit	`83.07% <0.00%> (ø)`

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

Impacted Files	Coverage Δ
gitlab/v4/objects/merge_requests.py	`84.78% <50.00%> (ø)`

Copy link

Member

nejch commentedJan 8, 2022

As we see in the functional tests this istechnically a breaking behavior. But only in the scenarios where
a) the project-level setting for "should remove source branch" is set to true (which I think has been the default for new projects for a while now)
b) the user does not supply theshould_remove_source_branch param

In this case,python-gitlab used to override the project default due to its own default (which wasFalse). But IMO this is wrong and unexpected behavior frompython-gitlab, so I think we can get away with afix here 👍 For anyone reading this after the fact, themerge() default was added years before the project-level setting was introduced, so as soon as GitLab introduced the project-level setting our client started to have wrong behavior and it doesn't make sense anymore.