Fixes squash merge request for Single Job Mode #369

anuragagarwal561994 · 2023-01-23T22:53:39Z

GitLab has option to configure squash and merge option at three places:

project level, can either be enforced or defaulted to on / off
while create merge request
while merging the merge request

When we are accepting a merge request, if the option to squash enforced is enabled at a project level, we need to pass squash: true while accepting the merge request in the API call or else we get error on GitLab:

Currently the MR is stuck in loop and marge bot crashes. This MR tries to address and resolve the following issues:

#326
#297
#333
#152

anuragagarwal561994 · 2023-01-23T23:25:37Z

@snim2 can you help review these changes. Thanks

snim2 · 2023-01-24T09:11:39Z

@snim2 can you help review these changes. Thanks

I'll have a look for you later today.

anuragagarwal561994 · 2023-01-24T13:58:40Z

@snim2 thanks, we have tested the main scenario in our test project and it is working. We are checking remaining scenarios.

Also I needed to add test case for the same, but would require some help to understand how it has been setup.

snim2 · 2023-01-24T18:18:17Z

pylintrc

@@ -30,7 +30,7 @@ max-line-length=110
 [DESIGN]
 max-args=10
 max-attributes=15
-max-public-methods=35
+max-public-methods=40


This is possibly something that the project owners will want to look at, but IMO it's fine in this repo

Although this seems to be reverted in the next commit?

Yes so there were 2 approaches in which this could have done. Earlier I was merging the project config with the MR config and then taking a decision.

But then I thought that there is only one case we need to address which is when at project level we have sqaush_option: always, so we are not taking any decision on behalf of the user or gitlab, instead we are respecting user's configuration and only sending squash: true when the project level configuration is set to always because in that case even gitlab won't allow the user to change the configuration at MR level.

For the earlier case I had to add a method to merge_request class which was exceeding the number of methods, but in this case we don't have to deal with this class at all, hence reverted.

snim2 · 2023-01-24T18:18:58Z

marge/single_merge_job.py

-            auto_squash = None
-            if self.is_auto_squash_enabled(merge_request):
-                auto_squash = True
+            auto_squash = True if target_project.squash_enforced else None


Why not True if target_project.squash_enforced else False?

Again from my previous comment, a user is allowed to choose if he / she wants squash at 3 different places.

in the project settings

in the merge request settings

while merging the merge request

Out of all the case only one case we had to solve for when squash is set to required at project level. In this case we are supposed to pass this extra parameter squash: True which more or less actually seems like a bug to me from gitlab side, remaining cases we are not sending this parameter and letting gitlab take the decision based on it's business logic based on user's input.

Hence a distinction between False & None. It has been sometime since I have worked in python, so not sure if this the best way to write it. But this is intentional, I can add a comment over this line to make it more clear.

snim2 · 2023-01-24T18:20:33Z

I don't think I have maintainer rights here, but this looks OK to me!

theipster · 2023-01-24T20:10:19Z

I've also tested this fix just now on one of our private GitLab repos (GitLab SaaS), and I can confirm this fix works quite nicely. ✅

anuragagarwal561994 · 2023-01-24T20:23:47Z

Thanks @theipster, @snim2 would require some help with the test cases, I haven't had experience writing test cases in python, I am trying still. There are few things I need to take care I believe:

in test_job.py, I am mocking project which is making the condition

    auto_squash = (
            self._project.squash_enforced or
            merge_request.squash
        )

as always True, becuase squash_enforced is mocked. This is defeating the purpose for test case test_ensure_mergeable_mr_squash_and_trailers

Is there a better way this can be handled.

I want to add test case for the same by changing the project level always, should I update the mock in this test case. No examples I guess to know how it is done.
I wanted to add more cases in test_merge_request.py but I am getting confused with the flow, a little help I would be able to write the tests then.

theipster · 2023-01-29T05:17:57Z

I'm neither an expert in Python nor marge-bot, but I can try and give some suggestions. 🙂

Firstly, this may help: I consider the MR-level squash to mean "I want to squash", i.e. this is my choice, regardless of the consequences. This is in contrast to the project-level squash_enforced that means "I need to squash (otherwise the operation will fail)". Note that these are two different things, but are not mutually exclusive.

So, in terms of your questions:

in test_job.py, I am mocking project which is making the condition
auto_squash = (
        self._project.squash_enforced or
        merge_request.squash
    )
as always True, becuase squash_enforced is mocked. This is defeating the purpose for test case test_ensure_mergeable_mr_squash_and_trailers

This is not defeating the purpose. With the introduction of this project-level setting, this simply means there are now additional scenarios to consider:

#	Scenario	Project `squash_enforced`	MR `squash`
1	"I need to squash" and so "I want to squash"	`True`	`True`
2	"I need to squash" but "I don't want to squash" (and suffer the consequence later)	`True`	`False`
3	"I don't need to squash" but "I want to squash" anyway	`False`	`True`
4	"I don't need to squash" and so "I don't want to squash"	`False`	`False`

Scenarios 1 and 3 (MR squash is True) are already covered by the existing test_ensure_mergeable_mr_squash_and_trailers test case (maybe rename it to test_ensure_mergeable_mr_squash_wanted_and_trailers to make this clearer).

For scenario 2, you should add a new test case (e.g. test_ensure_mergeable_mr_squash_needed_and_trailers): configure the mock project squash_option to be always, configure the mock MR squash to be False, and then assert that CannotMerge is raised.

Scenario 4 is the "happy path" where we don't expect any exceptions. You can add a new test case for this if you like. This scenario is not specific to squashing, and I guess it was simply forgotten.

I wanted to add more cases in test_merge_request.py but I am getting confused with the flow, a little help I would be able to write the tests then.

These existing test_accept_* tests follow an approach called behaviour verification, whereby instead of asserting on the output (i.e. return values), it instead asserts on what actions (i.e. function calls) were triggered. This behaviour verification pattern can be useful when it's painful to control the inner behaviour.

So, in our case, instead of asserting on the exact HTTP response, it simply asserts that a HTTP request was made with certain parameters. This is because it's painful to control the HTTP response.

In terms of how to what you need to do:

Firstly, fix the existing tests. The existing tests expect the HTTP request to be made with a dict of 3 entries, but you've now added an extra params['squash'] = None (default) to the dict, so the existing tests will fail. You'll need to update the assertions to reflect the new dict.
Now, add an extra test for each of the possible options that the auto_squash parameter can accept (I'm guessing True, False and None), and set the asserted dict respectively.

anuragagarwal561994 · 2023-02-02T20:56:27Z

@theipster thanks for this detailed explanation. I am currently busy with my work but would surely like to contribute and complete this PR by the end of this month with proper test cases.

See https://stackoverflow.com/a/73932581. It seems like importlib-metadata is only required for tests, not production code.

…g `squash`

…mplify mocking) the only API calls made in marge-bot are `/projects` (with `simple` undefined) and `/projects/:id`, and both responses always contain the `squash_option` field to limit the scope of validation outside `Project`, convert `squash_option` into an enum

also rename test to reflect wanting vs needing to squash

theipster · 2023-02-12T04:42:53Z

@anuragagarwal561994 I had some spare time and so I decided to roll up my sleeves and have a go myself - I hope you don't mind!

Please see anuragagarwal561994#1, which adds some tests and also fixes some pip dependencies (which are unfortunately necessary for running the test suite).

anuragagarwal561994 · 2023-02-16T00:57:40Z

@theipster thanks a lot for doing this, I will review the changes from my end. No mind of course, I am not getting time to sit for this honestly :)

Add tests

anuragagarwal561994 · 2023-02-16T01:38:50Z

@theipster I have found the changes to be appropriate and I have merged the same.

@snim2 I guess there is a dependent change by @theipster which is included in this MR as well
#370

I suppose we can review and merge the same first and then we can re-review and merge this one.

anuragagarwal561994 · 2023-02-23T22:32:50Z

@snim2 did you get time to review and merge the same?

snim2 · 2023-02-24T14:03:29Z

@anuragagarwal561994 I don't have rights to merge in this repo :)

anuragagarwal561994 · 2023-02-24T14:04:51Z

@snim2 who can help with this?

snim2 · 2023-02-24T14:06:13Z

@snim2 who can help with this?

Someone from Smarkets, I guess, but you'd have to look back through the old PRs and see who merged them to find a username

anuragagarwal561994 · 2023-02-24T14:25:50Z

@nithyashree675 @qqshfox can you help with the same?

anuragagarwal561994 added 5 commits January 24, 2023 04:29

Adds squash options in project

fdf6afe

Allows auto squash when the MR is squashable

7919a9e

Adds default values breaking tests

14302c1

Prevents breaking existing test cases

c5903b8

Fixes linting

00785b2

anuragagarwal561994 force-pushed the squash-fix branch from 7e25cd2 to 00785b2 Compare January 23, 2023 23:00

Uses only project enforced

0e4e82c

anuragagarwal561994 force-pushed the squash-fix branch from e8950d3 to 0e4e82c Compare January 23, 2023 23:22

snim2 reviewed Jan 24, 2023

View reviewed changes

theipster added 6 commits February 12, 2023 02:34

Fix tests by adding pip constraint for importlib-metadata.

7552284

See https://stackoverflow.com/a/73932581. It seems like importlib-metadata is only required for tests, not production code.

reduce mocking, since squash check happens before approval check

b3b10a3

add test around ensure_mergeable_mr() function's handling of needin…

4c98f7f

…g `squash`

pytest parametrize over enum iterator of project squash options

75a2835

also rename test to reflect wanting vs needing to squash

add tests around accept() function's squash arg

d528a86

Merge pull request #1 from theipster/master

3108ecf

Add tests

alexanderadam mentioned this pull request Feb 15, 2024

Include Squash patch hiboxsystems/marge-bot#55

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes squash merge request for Single Job Mode #369

Fixes squash merge request for Single Job Mode #369

anuragagarwal561994 commented Jan 23, 2023 •

edited

Loading

anuragagarwal561994 commented Jan 23, 2023

snim2 commented Jan 24, 2023

anuragagarwal561994 commented Jan 24, 2023

snim2 Jan 24, 2023

snim2 Jan 24, 2023

anuragagarwal561994 Jan 24, 2023

snim2 Jan 24, 2023

anuragagarwal561994 Jan 24, 2023

snim2 commented Jan 24, 2023

theipster commented Jan 24, 2023

anuragagarwal561994 commented Jan 24, 2023

theipster commented Jan 29, 2023 •

edited

Loading

anuragagarwal561994 commented Feb 2, 2023

theipster commented Feb 12, 2023

anuragagarwal561994 commented Feb 16, 2023

anuragagarwal561994 commented Feb 16, 2023

anuragagarwal561994 commented Feb 23, 2023

snim2 commented Feb 24, 2023

anuragagarwal561994 commented Feb 24, 2023

snim2 commented Feb 24, 2023

anuragagarwal561994 commented Feb 24, 2023

Fixes squash merge request for Single Job Mode #369

Are you sure you want to change the base?

Fixes squash merge request for Single Job Mode #369

Conversation

anuragagarwal561994 commented Jan 23, 2023 • edited Loading

anuragagarwal561994 commented Jan 23, 2023

snim2 commented Jan 24, 2023

anuragagarwal561994 commented Jan 24, 2023

snim2 Jan 24, 2023

Choose a reason for hiding this comment

snim2 Jan 24, 2023

Choose a reason for hiding this comment

anuragagarwal561994 Jan 24, 2023

Choose a reason for hiding this comment

snim2 Jan 24, 2023

Choose a reason for hiding this comment

anuragagarwal561994 Jan 24, 2023

Choose a reason for hiding this comment

snim2 commented Jan 24, 2023

theipster commented Jan 24, 2023

anuragagarwal561994 commented Jan 24, 2023

theipster commented Jan 29, 2023 • edited Loading

anuragagarwal561994 commented Feb 2, 2023

theipster commented Feb 12, 2023

anuragagarwal561994 commented Feb 16, 2023

anuragagarwal561994 commented Feb 16, 2023

anuragagarwal561994 commented Feb 23, 2023

snim2 commented Feb 24, 2023

anuragagarwal561994 commented Feb 24, 2023

snim2 commented Feb 24, 2023

anuragagarwal561994 commented Feb 24, 2023

anuragagarwal561994 commented Jan 23, 2023 •

edited

Loading

theipster commented Jan 29, 2023 •

edited

Loading