fix: honor parameter value passed

[ghost]

Gitlab allows setting the defaults for MR to delete the source. Also
the inline help of the CLI suggest that a boolean is expected, but no
matter what value you set, it will always delete.

To mimic the existing behavior as good as possible, we check for not false
instead of true. This way the impact on the cli will be minimal.

[codecov-io]

Codecov Report

Merging #1271 (c2f8f0e) into master (9fcd962) will not change coverage.
The diff coverage is 0.00%.

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

Flag	Coverage Δ
unit	80.58% <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	65.45% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fcd962...c2f8f0e. Read the comment docs.

[max-wittig]

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

Wouldn't this also work?:

data["should_remove_source_branch"] = should_remove_source_branch

[ghost]

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

If I use the released code to execute gitlab 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 but false, 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 the Enable "Delete source branch" option by default set. That's why we ran into the issue in the first place.

[max-wittig]

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

data["should_remove_source_branch"] = should_remove_source_branch

[ghost]

Yes, it would work but reduce XXXX to only true or false (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.

[max-wittig]

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

if should_remove_source_branch is not None:
    should_remove_source_branch =  bool(distutils.util.strtobool(data["should_remove_source_branch"]))

[max-wittig]

the only problem here is when should_remove_source_branch is already a bool. Then this will crash 🤔

[max-wittig]

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

[ghost]

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

@max-wittig would you prefer the shorter solution of bool(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?

[max-wittig]

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

[nejch]

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.

[nejch]

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

With this fix, passing --should-remove-source-branch with True/true will delete it, false/False will not, and any other value will raise an error with should_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 the many 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?

[ghost]

Maybe I'm not getting your suggestion, @nejch, but data["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.

[nejch]

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 with mr.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.

[ghost]

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

[nejch]

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

[max-wittig]

@nejch Should be good now then, right?

[nejch]

@nejch Should be good now then, right?

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