Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

pspacek
Copy link
Contributor

@pspacek pspacek commented Oct 18, 2022

Fixes: #2325

@lmilbaum
Copy link
Contributor

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

Copy link
Member

@nejch 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 Outdated Show resolved Hide resolved
gitlab/client.py Outdated Show resolved Hide resolved
@nejch
Copy link
Member

nejch commented Oct 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 :)

@nejch
Copy link
Member

nejch commented Dec 4, 2022

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!

@pspacek
Copy link
Contributor Author

pspacek commented Dec 5, 2022

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

Comment on lines +379 to +389
def response_callback(
response: requests.models.Response,
) -> requests.models.Response:
"""We need a callback that adds a resource lock reason only on first call"""
nonlocal retried

if not retried:
response.reason = "Resource lock"

retried = True
return response
Copy link
Member

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 because responses doesn't support mocking Response.reason as @pspacek already found out in #2325 (comment).

@nejch 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
Copy link
Member

@nejch 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 in 25446cf to finish the PR so will need someone else to do the merge.

Thanks again @pspacek!

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2022

Codecov Report

Merging #2326 (1a37bab) into main (c7cf0d1) will increase coverage by 0.00%.
The diff coverage is 90.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%) ⬆️

@nejch
Copy link
Member

nejch commented Dec 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.

@lmilbaum lmilbaum self-requested a review December 19, 2022 20:02
tests/unit/test_gitlab_http_methods.py Show resolved Hide resolved
gitlab/client.py Show resolved Hide resolved
tests/unit/test_gitlab_http_methods.py Show resolved Hide resolved
Copy link
Contributor

@lmilbaum lmilbaum left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] auto-retry when HTTP 409 Conflict: Resource lock is received
6 participants