NotificationsYou must be signed in to change notification settings
Fork673
Star2.4k

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

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nejch merged 5 commits intopython-gitlab:masterfromJohnVillalovos:jlvillal/fix_1452_query_parameters

May 25, 2021

Merged

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

nejch merged 5 commits intopython-gitlab:masterfromJohnVillalovos:jlvillal/fix_1452_query_parameters

May 25, 2021

Conversation

Copy link

Member

JohnVillalovos commentedMay 23, 2021•
edited
Loading

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 PayloadAPI Requests can use parameters sent as query strings or as apayload body. GET requests usually send a query string, while PUTor POST requests usually send the payload body

Copy link

codecov-commenter commentedMay 23, 2021•
edited
Loading

Codecov Report

Merging#1465 (2aaf395) intomaster (09ef8d4) willincrease coverage by10.62%.
The diff coverage isn/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%)`	⬆️
... and22 more

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 fromnejch andmax-wittig

May 23, 2021 21:57

nejch reviewed

May 24, 2021

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks@JohnVillalovos!

The folks atpyup introduced that MR (seepyupio/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 ofwait_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.

tools/functional/api/test_merge_requests.py Outdated

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

Copy link

Member

nejchMay 24, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This could be turned into a fixture factory inconftest.py (scoped to functions) that we can then use in any other test as well. The convention seems to be to create amake_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 in1508eb7

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

	def_check(container):
	logs= ["docker","logs",container]
	return"gitlab Reconfigured!"incheck_output(logs).decode()

	return_check

Copy link

MemberAuthor

JohnVillalovosMay 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@nejch

I have created a pytest.fixture for merge_request.

Let me know what you think! 😊

Copy link

Member

nejch commentedMay 24, 2021•
edited
Loading

The folks atpyup introduced that MR (seepyupio/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 :)

Copy link

MemberAuthor

JohnVillalovos commentedMay 24, 2021•
edited
Loading

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

Copy link

Member

nejch commentedMay 24, 2021

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 in1508eb7

	deftest_create_branch(gitlab_cli,project):
	branch="branch1"

	cmd= [
	"project-branch",
	"create",
	"--project-id",
	project.id,
	"--branch",
	branch,
	"--ref",
	"master",
	]
	ret=gitlab_cli(cmd)

	assertret.success


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

	assertret.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 :)

Copy link

MemberAuthor

JohnVillalovos commentedMay 24, 2021

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

nejch reviewed

May 25, 2021

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

tools/functional/conftest.py OutdatedShow resolvedHide resolved

JohnVillalovos added5 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 tohaving them in the 'data' body section. Add a functional test to makesure 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

fix: change mr.merge() to use 'post_data'

cb6a3c6

MR#1121 changedmr.merge() to use 'query_data'. This appears to have been wrong.From the Gitlab docs they state it should be sent in a payload bodyhttps://docs.gitlab.com/ee/api/README.html#request-payload sincemr.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 bodyFixes:#1452Related to:#1120

chore: simplify functional tests

df9b5f9

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

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

8be2838

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

nejch approved these changes

May 25, 2021

View reviewed changes

nejchenabled auto-merge

May 25, 2021 22:18

nejch merged commit9beff0d intopython-gitlab:master

May 25, 2021

JohnVillalovos deleted the jlvillal/fix_1452_query_parameters branch

May 26, 2021 00:27

Labels

None yet

3 participants

Movatterモバイル変換

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

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

Uh oh!

Conversation

JohnVillalovos commentedMay 23, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commentedMay 23, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

nejchMay 24, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnVillalovosMay 24, 2021

Choose a reason for hiding this comment

Uh oh!

nejch commentedMay 24, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

JohnVillalovos commentedMay 24, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

nejch commentedMay 24, 2021

Uh oh!

JohnVillalovos commentedMay 24, 2021

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JohnVillalovos commentedMay 23, 2021•
edited
Loading

codecov-commenter commentedMay 23, 2021•
edited
Loading

nejchMay 24, 2021•
edited
Loading

nejch commentedMay 24, 2021•
edited
Loading

JohnVillalovos commentedMay 24, 2021•
edited
Loading