X Tutup
Skip to content

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

Merged
nejch merged 1 commit intomainfrom
jlvillal/merge_request_merge_defaults
Jan 8, 2022
Merged

fix: remove default arguments for mergerequests.merge()#1818
nejch merged 1 commit intomainfrom
jlvillal/merge_request_merge_defaults

Conversation

@JohnVillalovos
Copy link
Member

@JohnVillalovos JohnVillalovos commented Jan 8, 2022

The arguments should_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 JohnVillalovos requested a review from nejch January 8, 2022 22:46
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/merge_request_merge_defaults branch from adc9c7c to d76152e Compare January 8, 2022 22:47
@JohnVillalovos
fix: remove default arguments for mergerequests.merge()
8e589c4 
The arguments `should_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 JohnVillalovos force-pushed the jlvillal/merge_request_merge_defaults branch from d76152e to 8e589c4 Compare January 8, 2022 23:08
@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2022

Codecov Report

Merging #1818 (8e589c4) into main (22a1516) will not change coverage.
The diff coverage is 50.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%> (ø)

@nejch
Copy link
Member

nejch commented Jan 8, 2022

As we see in the functional tests this is technically 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 the should_remove_source_branch param

In this case, python-gitlab used to override the project default due to its own default (which was False). But IMO this is wrong and unexpected behavior from python-gitlab, so I think we can get away with a fix here 👍 For anyone reading this after the fact, the merge() 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.

@JohnVillalovos
Copy link
Member Author

JohnVillalovos commented Jan 8, 2022

@nejch I agree. As it is very confusing if the project default is set to remove it but then we are by default not removing it.

@nejch nejch merged commit 0dba899 into main Jan 8, 2022
@nejch nejch deleted the jlvillal/merge_request_merge_defaults branch January 8, 2022 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nejch nejch Awaiting requested review from nejch

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

mergerequest.merge() implicitly sets should_remove_source_branch: false since 2.7.0

3 participants

@JohnVillalovos @codecov-commenter @nejch
X Tutup