Codecov Report

Merging#1271 (c2f8f0e) intomaster (9fcd962) willnot change coverage.
The diff coverage is0.00%.

@@           Coverage Diff           @@##           master    #1271   +/-   ##=======================================  Coverage   80.58%   80.58%           =======================================  Files          65       65             Lines        3240     3240           =======================================  Hits         2611     2611             Misses        629      629

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

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update9fcd962...c2f8f0e. Read thecomment docs.

The existing behavior only happens, if you setshould_remove_source_branch to default be true:

Wouldn't this also work?:

data["should_remove_source_branch"]=should_remove_source_branch

@max-wittig with existing behavior I was referring to GitLab CLI behavior.

If I use the released code to executegitlab project-merge-request merge --project-id 22306244 --iid 197 --should-remove-source-branch XXXX, where XXXX can be anything,true will be sent on the API request.

My MR changes the behavior to, while XXXX is anything butfalse,true is sent on the API request. So it's as close as possible to the current behavior, while still allowing to set it to false. Currently, you can't set it to false at all.

Yes, we have theEnable "Delete source branch" option by default set. That's why we ran into the issue in the first place.

I got that, but wouldn't the mentioned line of code also work?

data["should_remove_source_branch"]=should_remove_source_branch

Yes, it would work but reduce XXXX to onlytrue orfalse (case insensitive). This will break backward compatibility a little but if you don't mind in this specific case being backward compatible, I can change it.

If you omit the flag, GitLab will use the default setting in your project (could be true or false).

And also your fix is not gonna work, only for your configuration. python-gitlab is not actively sendingshould_remove_source_branch for every request. If it's omitted, GitLab just uses the default. Whatever your projects/groups default is.

The string value should be converted to a boolean value

Something like this should work

ifshould_remove_source_branchisnotNone:should_remove_source_branch=bool(distutils.util.strtobool(data["should_remove_source_branch"]))

Might not be the cleanest solution, but that works. Any other ideas@nejch

Might not be the cleanest solution, but that works. Any other ideas@nejch

@max-wittig would you prefer the shorter solution ofbool(strtobool(str(should_remove_source_branch)))? That would also avoid the crash but I'd consider it unclean.

Or should I create a utils.py function that streamlines the code?

I prefer your current approach, but I'm unsure.@nejch any further ideas?

I prefer your current approach, but I'm unsure.@nejch any further ideas?

It also seems a bit more readable but I'm wondering if there are other custom methods that might be affected by this, in which case it might make sense to have utils function for it. I will have a quick look for others now and update here.

After testing the existing behavior, I think going for the simpler option with justdata["should_remove_source_branch"] = should_remove_source_branch makes the most sense.

With this fix, passing--should-remove-source-branch withTrue/true will delete it,false/False will not, and any other value will raise an error withshould_remove_source_branch is invalid. I tested it manually but maybe we can add some tests.

I don't think it's worth preserving existing behavior for other edge cases because it wasn't documented anywhere and the only reasonable expectation IMO is that one of the above options is passed. People who have been using it as documented (even if it had no effect) would not see any undesired change. In any case it's one of themany places where the CLI is misbehaving and we warn in the CLI reference that some options may be unstable I think :)

Do you agree @allcloud-jonathan@max-wittig?

Maybe I'm not getting your suggestion,@nejch, butdata["should_remove_source_branch"] = should_remove_source_branch would you mean this:

if should_remove_source_branch is not None:    data["should_remove_source_branch"] = should_remove_source_branch

or this:

data["should_remove_source_branch"] = should_remove_source_branch

The first one fails for the non-CLI tests. The second one additionally now makes the parameter mandatory which means I can no longer use the default setup on the repo.

I meant the first option - what API tests are failing for you? Maybe it's just some of the flaky functional tests. I've tried and all tests passed it seems - I also tried manually withmr.merge(should_remove_source_branch="false") which receives a truthy value that still gets sent 'correctly' as"false" to the API and the branch is not deleted.

@nejch can you retrigger the tests? Haven't changed that part and previously it worked...

@nejch can you retrigger the tests? Haven't changed that part and previously it worked...

Thanks @allcloud-jonathan that definitely looks like those flaky tests, running again.

@nejch Should be good now then, right?

@nejch Should be good now then, right?

Ah sorry, looks good to me too :) @allcloud-jonathan sorry this took a while.