Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 8, 2022
Merged

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

merged 1 commit into from
Jan 8, 2022

Conversation

JohnVillalovos
Copy link
Member

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

@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