Switch mr.merge() to use post_data (was using query_data) #1465

JohnVillalovos · 2021-05-23T17:48:10Z

MR #1121 changed
mr.merge() to use 'query_data'. This appears to have been wrong.

From the Gitlab docs they state it should be sent in a payload body
https://docs.gitlab.com/ee/api/README.html#request-payload since
mr.merge() is a PUT request.

Request Payload

API Requests can use parameters sent as query strings or as a
payload body. GET requests usually send a query string, while PUT
or POST requests usually send the payload body

codecov-commenter · 2021-05-23T17:49:04Z

Codecov Report

Merging #1465 (2aaf395) into master (09ef8d4) will increase coverage by 10.62%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1465       +/-   ##
===========================================
+ Coverage   80.24%   90.86%   +10.62%     
===========================================
  Files          73       73               
  Lines        4064     4027       -37     
===========================================
+ Hits         3261     3659      +398     
+ Misses        803      368      -435

Flag	Coverage Δ
cli_func_v4	`80.35% <ø> (?)`
py_func_v4	`79.66% <ø> (?)`
unit	`81.89% <ø> (+1.65%)`	⬆️

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

Impacted Files	Coverage Δ
gitlab/v4/objects/merge_requests.py	`81.81% <ø> (+16.36%)`	⬆️
gitlab/__main__.py	`0.00% <0.00%> (ø)`
gitlab/v4/objects/deploy_tokens.py	`100.00% <0.00%> (ø)`
gitlab/v4/objects/runners.py	`98.14% <0.00%> (+0.18%)`	⬆️
gitlab/base.py	`89.24% <0.00%> (+0.63%)`	⬆️
gitlab/exceptions.py	`99.28% <0.00%> (+1.45%)`	⬆️
gitlab/types.py	`96.42% <0.00%> (+3.83%)`	⬆️
gitlab/v4/objects/pipelines.py	`93.33% <0.00%> (+5.33%)`	⬆️
gitlab/v4/objects/groups.py	`82.22% <0.00%> (+5.81%)`	⬆️
gitlab/v4/objects/issues.py	`86.76% <0.00%> (+5.88%)`	⬆️
... and 22 more

nejch

Thanks @JohnVillalovos!

The folks at pyup introduced that MR (see pyupio/pyup#384) so we should maybe check with them why that was done (/cc @ferhat-aram @rafaelpivato). I agree it should be in post data for POST/PUT though, I had the same doubts in the original issue as I couldn't reproduce the issue.

The fact they had issues with 405 Method Not Allowed and that you have a lot of wait_for_sidekiq makes me think they were more likely hitting a race condition when applying the merge, but not sure why query data would work in that case.

nejch · 2021-05-24T06:33:50Z

tools/functional/api/test_merge_requests.py

@@ -95,3 +98,116 @@ def test_merge_request_merge(project):
    with pytest.raises(gitlab.GitlabMRClosedError):
        # Two merge attempts should raise GitlabMRClosedError
        mr.merge()
+
+
+def merge_request_create_helper(


This could be turned into a fixture factory in conftest.py (scoped to functions) that we can then use in any other test as well. The convention seems to be to create a make_merge_request fixture with an inner _make_merge_request function.
https://docs.pytest.org/en/6.2.x/fixture.html#factories-as-fixtures

Although not sure all of the logic should then stay in that fixture, the asserts would belong in the test itself 🤔

We have something similar here

python-gitlab/tools/functional/conftest.py

Lines 68 to 78 in 1508eb7

@pytest.fixture(scope="session")

def check_is_alive():

"""

Return a healthcheck function fixture for the GitLab container spinup.

"""

def _check(container):

logs = ["docker", "logs", container]

return "gitlab Reconfigured!" in check_output(logs).decode()

return _check

@nejch

I have created a pytest.fixture for merge_request.

Let me know what you think! 😊

nejch · 2021-05-24T19:31:46Z

The folks at pyup introduced that MR (see pyupio/pyup#384) so we should maybe check with them why that was done (/cc @ferhat-aram @rafaelpivato). I agree it should be in post data for POST/PUT though, I had the same doubts in the original issue as I couldn't reproduce the issue.

Sorry @JohnVillalovos actually I think we can go ahead with this, since the previous PR broke behavior for other people and POST is the documented approach by GitLab. If it doesn't work with POST then it should be addressed upstream in GitLab.

Maybe if we can just have that helper as a fixture it'd be nice :)

JohnVillalovos · 2021-05-24T19:45:09Z

Maybe if we can just have that helper as a fixture it'd be nice :)

Okay. Can we do that later?

Though I'm not sure I totally understand the benefit of making it a fixture as only two functions use it and both of those are in the same file.

I can do it, but I won't have time to do it until tomorrow at the earliest. Sorry work is a bit busy today...

nejch · 2021-05-24T20:05:23Z

Maybe if we can just have that helper as a fixture it'd be nice :)

Okay. Can we do that later?

Though I'm not sure I totally understand the benefit of making it a fixture as only two functions use it and both of those are in the same file.

I can do it, but I won't have time to do it until tomorrow at the earliest. Sorry work is a bit busy today...

For example in the CLI functional tests, we're relying on test order because we don't have fixtures to create a merge request from a new branch:

python-gitlab/tools/functional/cli/test_cli_v4.py

Lines 189 to 224 in 1508eb7

    
           def test_create_branch(gitlab_cli, project): 
        
               branch = "branch1" 
        
               cmd = [ 
        
                   "project-branch", 
        
                   "create", 
        
                   "--project-id", 
        
                   project.id, 
        
                   "--branch", 
        
                   branch, 
        
                   "--ref", 
        
                   "master", 
        
               ] 
        
               ret = gitlab_cli(cmd) 
        
               assert ret.success 
        
           def test_create_merge_request(gitlab_cli, project): 
        
               branch = "branch1" 
        
               cmd = [ 
        
                   "project-merge-request", 
        
                   "create", 
        
                   "--project-id", 
        
                   project.id, 
        
                   "--source-branch", 
        
                   branch, 
        
                   "--target-branch", 
        
                   "master", 
        
                   "--title", 
        
                   "Update README", 
        
               ] 
        
               ret = gitlab_cli(cmd) 
        
               assert ret.success

There are other potential tests we currently don't do because it's not convenient (merge request deployments, merge request <resource a/b/c>) and things like this would really help. But it's a wider issue for all functional tests so definitely can wait.

Also no rush of course, my comment was only that we don't wait for feedback that I initially suggested :)

JohnVillalovos · 2021-05-24T20:46:05Z

@nejch That makes a lot of sense. Thanks for the explanation!

nejch

Thanks! Should be much more reusable later with the fixture. Just a tiny comment.

tools/functional/conftest.py

Going to switch to putting parameters from in the query string to having them in the 'data' body section. Add a functional test to make sure that we don't break anything. #1120

Functional test to show that #1452 is fixed. Added a functional test to ensure that we can use large commit message (10_000+ bytes) in mr.merge() Related to: #1452

MR #1121 changed mr.merge() to use 'query_data'. This appears to have been wrong. From the Gitlab docs they state it should be sent in a payload body https://docs.gitlab.com/ee/api/README.html#request-payload since mr.merge() is a PUT request. > Request Payload > API Requests can use parameters sent as query strings or as a > payload body. GET requests usually send a query string, while PUT > or POST requests usually send the payload body Fixes: #1452 Related to: #1120

Add a helper function to have less code duplication in the functional testing.

Added a pytest.fixture for merge_request(). Use this fixture in tools/functional/api/test_merge_requests.py

JohnVillalovos marked this pull request as draft May 23, 2021 20:28

JohnVillalovos marked this pull request as ready for review May 23, 2021 21:54

JohnVillalovos changed the title ~~chore: add a functional test for issue #1120~~ Switch mr.merge() to use post_data (was using query_data) May 23, 2021

JohnVillalovos requested review from nejch and max-wittig May 23, 2021 21:57

nejch reviewed May 24, 2021

View reviewed changes

nejch reviewed May 25, 2021

View reviewed changes

tools/functional/conftest.py Outdated Show resolved Hide resolved

JohnVillalovos added 5 commits May 25, 2021 14:54

chore: add a functional test for issue #1120

7d66115

Going to switch to putting parameters from in the query string to having them in the 'data' body section. Add a functional test to make sure that we don't break anything. #1120

chore: add functional test mr.merge() with long commit message

cd5993c

Functional test to show that #1452 is fixed. Added a functional test to ensure that we can use large commit message (10_000+ bytes) in mr.merge() Related to: #1452

chore: simplify functional tests

df9b5f9

Add a helper function to have less code duplication in the functional testing.

chore: add a merge_request() pytest fixture and use it

8be2838

Added a pytest.fixture for merge_request(). Use this fixture in tools/functional/api/test_merge_requests.py

nejch approved these changes May 25, 2021

View reviewed changes

nejch enabled auto-merge May 25, 2021 22:18

nejch merged commit 9beff0d into python-gitlab:master May 25, 2021

JohnVillalovos deleted the jlvillal/fix_1452_query_parameters branch May 26, 2021 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch mr.merge() to use post_data (was using query_data) #1465

Switch mr.merge() to use post_data (was using query_data) #1465

JohnVillalovos commented May 23, 2021 •

edited

Loading

codecov-commenter commented May 23, 2021 •

edited

Loading

nejch left a comment

nejch May 24, 2021 •

edited

Loading

JohnVillalovos May 24, 2021

nejch commented May 24, 2021 •

edited

Loading

JohnVillalovos commented May 24, 2021 •

edited

Loading

nejch commented May 24, 2021

JohnVillalovos commented May 24, 2021

nejch left a comment

	@pytest.fixture(scope="session")
	def check_is_alive():
	"""
	Return a healthcheck function fixture for the GitLab container spinup.
	"""

	def _check(container):
	logs = ["docker", "logs", container]
	return "gitlab Reconfigured!" in check_output(logs).decode()

	return _check

Switch mr.merge() to use post_data (was using query_data) #1465

Switch mr.merge() to use post_data (was using query_data) #1465

Conversation

JohnVillalovos commented May 23, 2021 • edited Loading

codecov-commenter commented May 23, 2021 • edited Loading

Codecov Report

nejch left a comment

Choose a reason for hiding this comment

nejch May 24, 2021 • edited Loading

Choose a reason for hiding this comment

JohnVillalovos May 24, 2021

Choose a reason for hiding this comment

nejch commented May 24, 2021 • edited Loading

JohnVillalovos commented May 24, 2021 • edited Loading

nejch commented May 24, 2021

JohnVillalovos commented May 24, 2021

nejch left a comment

Choose a reason for hiding this comment

JohnVillalovos commented May 23, 2021 •

edited

Loading

codecov-commenter commented May 23, 2021 •

edited

Loading

nejch May 24, 2021 •

edited

Loading

nejch commented May 24, 2021 •

edited

Loading

JohnVillalovos commented May 24, 2021 •

edited

Loading