feat: allow global retry_transient_errors

[javatarz]

Value set on the Gitlab object will be used as a default in case every
subsequent API call does not provide a retry_transient_errors
parameter.

This allows code to be succinct and the retry policy to be set for all
calls to the Gitlab instance.

[javatarz]

Noticed another MR that has had a failed pipeline for a while. It also did not have documentation. It made sense to make the small change from scratch and update documentation.

I have run the test locally (for my python version). I have not generated the documentation and run a visual check on the page itself yet.

Does anyone know what it would take for the pipelines to run and provide feedback? Do the tests and linters not run on the PR?

[codecov-commenter]

Codecov Report

Merging #1565 (3b1d3a4) into master (a00ec87) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1565      +/-   ##
==========================================
+ Coverage   91.16%   91.18%   +0.02%     
==========================================
  Files          74       74              
  Lines        4177     4188      +11     
==========================================
+ Hits         3808     3819      +11     
  Misses        369      369              

Flag	Coverage Δ
cli_func_v4	80.80% <100.00%> (+0.05%)	⬆️
py_func_v4	80.15% <100.00%> (+0.05%)	⬆️
unit	82.49% <100.00%> (+0.18%)	⬆️

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

Impacted Files	Coverage Δ
gitlab/client.py	86.76% <100.00%> (+0.07%)	⬆️
gitlab/config.py	93.03% <100.00%> (+0.42%)	⬆️

[nejch]

Noticed another MR that has had a failed pipeline for a while. It also did not have documentation. It made sense to make the small change from scratch and update documentation.

I have run the test locally (for my python version). I have not generated the documentation and run a visual check on the page itself yet.

Does anyone know what it would take for the pipelines to run and provide feedback? Do the tests and linters not run on the PR?

Thanks for the contribution @javatarz! I've just triggered the workflow, it needs to be approved for first-time contributors.

I'll take a look at this later tonight but yes adding a basic test would generally be good (if it makes sense here - maybe just to confirm the correct option is used depending on global/overrides) :) Thanks!

[javatarz]

I had looked for tests around this and there were none. I can see a test for http_request. I'll add a couple of tests for

1. verifying the default behaviour (when retry_transient_errors is not provided)
2. verifying how retry_transient_errors works (missing test case)
3. verifying the default value gets applied
4. verifying that the default value can be overridden

Please don't merge this MR, I'll be back in a few hours with tests!

[javatarz]

I have added the missing tests and update the documentation a bit. Here's what the documentation looks like now (after rendering)

@nejch: This one's ready for review again. It needs a nudge for running the checks on the pipeline. Tests passed locally on py39.

[nejch]

Hey @javatarz finally had a chance to look at this properly, thanks again, looks great!

This looks like a good candidate to be added to global (or per-server) config in the configuration file to keep it consistent with existing global options. You can take a look at the user agent (the last option added before yours) as inspiration if it helps: https://github.com/python-gitlab/python-gitlab/pull/1277/files

(I realize config.py could de-duplicate a bit..). We can also do it in a follow-up, just let me know. thanks!

[javatarz]

Sorry for the late response. Just had a look at config.py and it makes a lot of sense to add the retry_transient_errors to be supported through the configuration mechanism. Will push updates soon. (if you mark this as draft, you will have to trigger the pipeline again 😂)

[javatarz]

@nejch: I have added changes to allow config values to be set (both globally and per gitlab connection).

Used mypy typing in test utlity functions which isn't a pattern on the file. I cannot run mypy locally (because your fix for mypy has not been merged yet 😞). Not sure if I have broken something.

Do you mind approving the pipeline run for me once again so I can confirm if everything is ok?
Let me know if there's any other change necessary before we can get this merged. 😃

[nejch]

@nejch: I have added changes to allow config values to be set (both globally and per gitlab connection).

Used mypy typing in test utlity functions which isn't a pattern on the file. I cannot run mypy locally (because your fix for mypy has not been merged yet disappointed). Not sure if I have broken something.

Do you mind approving the pipeline run for me once again so I can confirm if everything is ok?
Let me know if there's any other change necessary before we can get this merged. smiley

Thank you @javatarz! I see mypy is not happy :) the hook fix should be ready soon. maybe you can check how it's done for the default scenario in ssl_verify with getboolean() if it helps?

[javatarz]

Can confirm, mypy runs fine well locally! Thanks @nejch!

The build has passed locally for Python 3.9. Should be fine on other versions too. Do you mind triggering the pipeline for me please? Just noticed that someone triggered it 😄

[javatarz]

Seems like the test failure is unrelated. I'm trying to run functional tests locally to confirm.

[nejch]

Seems like the test failure is unrelated. I'm trying to run functional tests locally to confirm.

The functional tests can be a little flaky sometimes, although I thought this was mostly fixed. I'm re-running now.

[javatarz]

Dang, they passed now. Should we merge this one now or should we disable that flaky test first?

[javatarz]

Are there any other steps before this change can be accepted @nejch, @JohnVillalovos?

Picked names off of the recent people to merge/commit things

[JohnVillalovos]

@javatarz Can the git commits be cleaned up? In particular the "merge" commits. It would be nicer to have a cleaner git history, IMHO.

[JohnVillalovos]

Please clean-up the commits to make it a cleaner git history. In particular removing the "merge" commits.

[javatarz]

@JohnVillalovos: can you have another look? I have squashed the commits into a single commit now.

[JohnVillalovos]

Please fix the error in the docs build.

[javatarz]

Thought that's a bug with my local setup. Wish the pipeline automatically ran after commits so I could get this feedback quicker.

@JohnVillalovos, @max-wittig, @nejch : Do you mind re-triggering the pipeline for me? Please let me know if there's any other review comments.

[JohnVillalovos]

Looks good to me. Would like at least one other person to review.

[nejch]

Thanks a lot @javatarz for adding all the tests, let's finally get this merged ;) We can follow up with a tiny docs update in https://github.com/python-gitlab/python-gitlab/blob/master/docs/cli-usage.rst#content.

[javatarz]

@nejch: is it possible to publish a new version of this artifact to pypi? I'd like to use this feature in multiple projects where I use this library. 😄

[nejch]

Thanks for the ping @javatarz! I would like to and usually we have a release every 28th but I've disabled it because we have #1520 next, and we're dropping support for 3.6 so we thought we'd release it with 3.6 EOL, along with other breaking changes in the library.

I could maybe try cherry-picking non-breaking changes into a 2.11.0 or a pre-release branch though.

[Sineaggi]

@nejch We're in a similar boat, would love to see a release cut with this particular change.