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

feat(client): automatically retry on HTTP 409 Resource lock#2326

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 2 commits intopython-gitlab:mainfrompspacek:http-409-lock-retry

Dec 19, 2022

Merged

feat(client): automatically retry on HTTP 409 Resource lock#2326

nejch merged 2 commits intopython-gitlab:mainfrompspacek:http-409-lock-retry

Dec 19, 2022

Conversation

Copy link

Contributor

lmilbaum commentedOct 18, 2022

Unit tests might increase the confidence that the code is working as expected. Would you mind adding unit tests?

nejch requested changes

Oct 18, 2022

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.

@pspacek thanks for the quick work here! Just a few nits.

Regarding unit tests and your question in#2325 (comment).

We have some vendored code from httmock for a similar reason (redirects). I think you might be able to reuse it here?
https://github.com/python-gitlab/python-gitlab/blob/main/tests/unit/helpers.py

See how it's used in the redirect tests:
https://github.com/python-gitlab/python-gitlab/blob/main/tests/unit/test_gitlab_http_methods.py#L310-L317

Let us know if you need help with that :) That's a shame about responses though.

docs/api-usage-advanced.rst OutdatedShow resolvedHide resolved

gitlab/client.py OutdatedShow resolvedHide resolved

Copy link

Member

nejch commentedOct 20, 2022

And@pspacek we're also ok to go ahead without the unit tests if it's too complicated and we open a follow-up issue to resolve on our end :)

Copy link

Member

Hi@pspacek! Let us know if you'd like to finish this PR, or if we should push to your branch to get this merged (either way you'll be credited as the commit author). Thanks again for the contribution!

Copy link

ContributorAuthor

pspacek commentedDec 5, 2022

@nejch I'm sorry, I have no time to work on this. Feel free to do whatever you like.

nejch force-pushed thehttp-409-lock-retry branch fromfdb1d2c to25446cfCompare

December 5, 2022 20:48

nejch reviewed

Dec 5, 2022

View reviewed changes

tests/unit/test_gitlab_http_methods.py

Comment on lines +379 to +389

		defresponse_callback(
		response:requests.models.Response,
		)->requests.models.Response:
		"""We need a callback that adds a resource lock reason only on first call"""
		nonlocalretried

		ifnotretried:
		response.reason="Resource lock"

		retried=True
		returnresponse

Copy link

Member

nejchDec 5, 2022

Choose a reason for hiding this comment

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

We're mostly doing this becauseresponses doesn't support mockingResponse.reason as@pspacek already found out in#2325 (comment).

nejch changed the title~~Draft: feat(client): automatically retry on HTTP 409 Resource lock~~feat(client): automatically retry on HTTP 409 Resource lock

Dec 5, 2022

nejch approved these changes

Dec 5, 2022

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.

This LGTM but I added a commit in25446cf to finish the PR so will need someone else to do the merge.

Thanks again@pspacek!

nejch assignedJohnVillalovos andmax-wittig

Dec 5, 2022

Copy link

codecov-commenter commentedDec 5, 2022•
edited
Loading

Codecov Report

Merging#2326 (1a37bab) intomain (c7cf0d1) willincrease coverage by0.00%.
The diff coverage is90.90%.

@@           Coverage Diff           @@##             main    #2326   +/-   ##=======================================  Coverage   96.13%   96.13%           =======================================  Files          84       85    +1       Lines        5534     5569   +35     =======================================+ Hits         5320     5354   +34- Misses        214      215    +1

Flag	Coverage Δ
api_func_v4	`82.58% <54.54%> (+<0.01%)`	⬆️
cli_func_v4	`82.83% <45.45%> (-0.06%)`	⬇️
unit	`87.48% <81.81%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
gitlab/client.py	`98.38% <90.90%> (-0.18%)`	⬇️
gitlab/v4/objects/__init__.py	`100.00% <0.00%> (ø)`
gitlab/v4/objects/resource_groups.py	`100.00% <0.00%> (ø)`
gitlab/v4/objects/projects.py	`98.86% <0.00%> (+<0.01%)`	⬆️

pspacekand others added2 commits

December 18, 2022 12:40

feat(client): automatically retry on HTTP 409 Resource lock

8cfb877

Fixes:python-gitlab#2325

chore: add test, docs, and helper for 409 retries

1a37bab

nejch force-pushed thehttp-409-lock-retry branch from25446cf to1a37babCompare

December 18, 2022 11:47

Copy link

Member

nejch commentedDec 19, 2022

Unit tests might increase the confidence that the code is working as expected. Would you mind adding unit tests?

@lmilbaum I've added a commit on top of the author's to rework and add tests (1a37bab). Maybe you can take a look and we can merge before the next release.