fix(modules/gitlab_runner): Use correct argument to list all runners #7790

mikewadsten · 2023-12-29T01:28:18Z

SUMMARY

python-gitlab 4.0.0 removed support for the as_list=False parameter. This functionality is now available as iterator=True.

Without this change, the module actually only retrieves the first 20 results, which can lead to non-idempotent behavior, such as registering a runner again.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

gitlab_runner

ADDITIONAL INFORMATION

I am using community.general 8.1.0, python-gitlab 4.2.0 and a self-hosted instance of GitLab,
and was trying to register a new instance runner. Initial registration was successful, but if I run the playbook again,
a new runner (with the same description value) would be registered.

When I ran again using --check, I get AttributeError: 'bool' object has no attribute '_attrs'
(which is a separate bug that I will write up), but module_stderr also includes:

UserWarning: Calling a `list()` method without specifying `get_all=True` or `iterator=True` will return a maximum of 20 items. Your query returned 20 of 46 items. See https://python-gitlab.readthedocs.io/en/v4.2.0/api-usage.html#pagination for more details. If this was done intentionally, then this warning can be supressed by adding the argument `get_all=False` to the `list()` call.

The python-gitlab docs on pagination call this out:

Prior to python-gitlab 3.6.0 the argument as_list was used instead of iterator. as_list=False is the equivalent of iterator=True.

python-gitlab 4.0.0 removed support for the `as_list=False` parameter. This functionality is now available as `iterator=True`. Without this change, the module actually only retrieves the first 20 results, which can lead to non-idempotent behavior, such as registering a runner again.

ansibullbot · 2023-12-29T01:33:05Z

cc @Lunik @SamyCoenen @Shaps @lgatellier @marwatk @metanovii @nejch @scodeman @sh0shin @suukit @waheedi @zanssa
click here for bot help

mikewadsten · 2023-12-29T01:48:07Z

Hmm. It occurs to me that this change would cause issues if using python-gitlab older than 3.6. I know the new runner registration workflow stuff (#7199) requires >= 4.0.0, but the module will work on older versions as long as registration_token is provided.

Should I change this to pass as_list=False, iterator=True, so that we get the same behavior on all versions?

felixfontein · 2023-12-29T08:39:34Z

See also #7690. I don't know whether passing both as_list=False, iterator=True is a good idea; in that issue @nejch suggested as_list=False, all=True, so I guess it also makes sense.

mikewadsten · 2023-12-29T15:41:43Z

Thanks, I tried to search for this issue but somehow missed 7690. This PR is indeed a fix for that.

The only issue I would take with using as_list=False, all=True as opposed to as_list=False, iterator=True, is that on new-ish versions of python-gitlab (which will ignore the as_list parameter entirely), that will retrieve all runners and store them into a list before returning anything (see https://github.com/python-gitlab/python-gitlab/blob/b8824a6c2f06903dc8975aaf5f8807a7b1245a6a/gitlab/client.py#L924 for example), whereas iterator=True will do lazy pagination.

But it looks like 3.6.0 through 3.15.0 don't let you pass both (see https://github.com/python-gitlab/python-gitlab/blob/2fbd1bf73c15959f4345befd03a28df882b461bd/gitlab/client.py#L900).

So I guess the options are:

Use as_list=False, all=True for maximum succinctness and compatibility, at the expense of potentially-wasted time and memory with python-gitlab 3.6.0+ (due to eager pagination), depending on the number of existing runners.
Look at the python-gitlab version (as the module already does to check compatibility with the new registration workflow), and use as_list=False on < 3.6.0, iterator=True on >= 3.6.0.

My preference would be for option 2. Thoughts?

plugins/modules/gitlab_runner.py

felixfontein · 2024-01-26T22:40:24Z

@nejch does the PR look good to you?

nejch · 2024-01-27T17:58:07Z

@nejch does the PR look good to you?

Sorry for the delay @felixfontein. LGTM, I would maybe even provide per_page in the Gitlab instantiation but that can be changed later as it's not user facing in this case. 👍

…-collections#7790) I did not change every instance of all=True or all=False, only those which could obviously benefit from simplifying: * Code using `all=True` but then searching for any items that match a condition (no need to collect the full list). * Code that basically reimplements `all=True` with manual pagination. (These could be changed to `all=True`, but `list_all_kwargs` also sets per_page to 100, to gather data faster.)

ansibullbot · 2024-01-27T21:18:44Z

cc @LennertMertens @markuman @stgrace
click here for bot help

mikewadsten · 2024-01-27T21:21:11Z

@felixfontein @nejch I've implemented the requested changes (using list_all_kwargs where it would be useful).

There are a few instances that I left alone. For instance, gitlab_group.py:

if not force and len(group.projects.list(all=False)) >= 1:

This is just a quick check for whether any groups still exist. No need to ask for "all" items, when the GitLab API defaults pages to size 20, so a single call here will be sufficient.

But I did refactor a few "all=True then look for a single match" to next((...), None), which a) is semantically equivalent, b) is more efficient (not collecting everything before looking), and c) is a pattern used in other modules already.

mikewadsten · 2024-01-27T21:22:07Z

Two things I'm not sure about:

Should I add a changelog fragment of some kind, calling out that this pagination change has been applied to more than gitlab_runner?
Should I rebase my branch, or is that unnecessary?

ansibullbot · 2024-01-27T21:51:43Z

cc @benibr
click here for bot help

felixfontein · 2024-01-28T12:16:41Z

1. Should I add a changelog fragment of some kind, calling out that this pagination change has been applied to more than `gitlab_runner`?

Yes, every changed module should be mentioned in a changelog fragment. (You can also mention them all in one.)

2. Should I rebase my branch, or is that unnecessary?

That's not necessary, unless there are conflicts. (Or you know that some of the GitLab modules have been modified and you need to rebase to get in the modifications because you need to change the corresponding code.)

plugins/module_utils/gitlab.py

…s#7790)

mikewadsten · 2024-01-30T21:04:15Z

Yes, every changed module should be mentioned in a changelog fragment. (You can also mention them all in one.)

Done. Hopefully I've formatted it properly. The Ansible development docs say a changelog fragment can have multiple sections in it, so I kept gitlab_runner under bugfixes and called out the rest of them as minor_changes.

felixfontein · 2024-02-11T12:22:35Z

Can someone please check all the new changes to this PR? Thanks :)

felixfontein · 2024-02-23T20:06:47Z

@markuman @nejch do you think this is OK to merge?

nejch

@markuman @nejch do you think this is OK to merge?

@felixfontein LGTM, I left a comment about the redundant argument there but I don't want to block this any longer. It will have no effect.

nejch · 2024-02-25T12:54:45Z

plugins/module_utils/gitlab.py

+        return {'as_list': False, 'get_all': True, 'per_page': 100}
+    else:
+        return {'as_list': False, 'all': True, 'per_page': 100}


quick note: as_list=False in older versions and iterator=True in newer versions takee precedence over all and get_all so IMO including the all / get_all arguments here is not necessary. But not a blocker.

👍 I'll merge for now so I can do a release this evening, if someone wants to create a follow-up PR to clean this up, feel free :)

patchback · 2024-02-25T18:45:34Z

Backport to stable-7: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 787fa46 on top of patchback/backports/stable-7/787fa4621763879dae26c57aaf0028941dcaaefc/pr-7790

Backporting merged PR #7790 into main

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/ansible-collections/community.general.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/stable-7/787fa4621763879dae26c57aaf0028941dcaaefc/pr-7790 upstream/stable-7

Now, cherry-pick PR fix(modules/gitlab_runner): Use correct argument to list all runners #7790 contents into that branch:
```
$ git cherry-pick -x 787fa4621763879dae26c57aaf0028941dcaaefc
```
If it'll yell at you with something like fatal: Commit 787fa4621763879dae26c57aaf0028941dcaaefc is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 787fa4621763879dae26c57aaf0028941dcaaefc
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR fix(modules/gitlab_runner): Use correct argument to list all runners #7790 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/stable-7/787fa4621763879dae26c57aaf0028941dcaaefc/pr-7790

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback · 2024-02-25T18:45:40Z

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/787fa4621763879dae26c57aaf0028941dcaaefc/pr-7790

Backported as #8032

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

…7790) * fix(modules/gitlab_runner): Use correct argument to list all runners python-gitlab 4.0.0 removed support for the `as_list=False` parameter. This functionality is now available as `iterator=True`. Without this change, the module actually only retrieves the first 20 results, which can lead to non-idempotent behavior, such as registering a runner again. * Add changelog entry (#7790) * gitlab_runner: Check python-gitlab version when listing runners * gitlab: Add list_all_kwargs variable to module_utils * refactor(gitlab modules): use list_all_kwargs where it helps (#7790) I did not change every instance of all=True or all=False, only those which could obviously benefit from simplifying: * Code using `all=True` but then searching for any items that match a condition (no need to collect the full list). * Code that basically reimplements `all=True` with manual pagination. (These could be changed to `all=True`, but `list_all_kwargs` also sets per_page to 100, to gather data faster.) * gitlab_instance_variable: Use list_all_kwargs * Add new changelog entry for gitlab module changes (#7790) (cherry picked from commit 787fa46)

felixfontein · 2024-02-25T18:46:47Z

@mikewadsten thanks for your contribution!
@nejch @markuman thanks for reviewing this!

felixfontein · 2024-02-25T18:47:17Z

If someone is interested in a 7.5.x backport, you have to cherry-pick the commit manually (git cherry-pick -x ...) and create a PR for it.

…e correct argument to list all runners (#8032) fix(modules/gitlab_runner): Use correct argument to list all runners (#7790) * fix(modules/gitlab_runner): Use correct argument to list all runners python-gitlab 4.0.0 removed support for the `as_list=False` parameter. This functionality is now available as `iterator=True`. Without this change, the module actually only retrieves the first 20 results, which can lead to non-idempotent behavior, such as registering a runner again. * Add changelog entry (#7790) * gitlab_runner: Check python-gitlab version when listing runners * gitlab: Add list_all_kwargs variable to module_utils * refactor(gitlab modules): use list_all_kwargs where it helps (#7790) I did not change every instance of all=True or all=False, only those which could obviously benefit from simplifying: * Code using `all=True` but then searching for any items that match a condition (no need to collect the full list). * Code that basically reimplements `all=True` with manual pagination. (These could be changed to `all=True`, but `list_all_kwargs` also sets per_page to 100, to gather data faster.) * gitlab_instance_variable: Use list_all_kwargs * Add new changelog entry for gitlab module changes (#7790) (cherry picked from commit 787fa46) Co-authored-by: Mike Wadsten <[email protected]>

…/gitlab_runner): Use correct argument to list all runners (ansible-collections#8032) fix(modules/gitlab_runner): Use correct argument to list all runners (ansible-collections#7790) * fix(modules/gitlab_runner): Use correct argument to list all runners python-gitlab 4.0.0 removed support for the `as_list=False` parameter. This functionality is now available as `iterator=True`. Without this change, the module actually only retrieves the first 20 results, which can lead to non-idempotent behavior, such as registering a runner again. * Add changelog entry (ansible-collections#7790) * gitlab_runner: Check python-gitlab version when listing runners * gitlab: Add list_all_kwargs variable to module_utils * refactor(gitlab modules): use list_all_kwargs where it helps (ansible-collections#7790) I did not change every instance of all=True or all=False, only those which could obviously benefit from simplifying: * Code using `all=True` but then searching for any items that match a condition (no need to collect the full list). * Code that basically reimplements `all=True` with manual pagination. (These could be changed to `all=True`, but `list_all_kwargs` also sets per_page to 100, to gather data faster.) * gitlab_instance_variable: Use list_all_kwargs * Add new changelog entry for gitlab module changes (ansible-collections#7790) (cherry picked from commit 787fa46) Co-authored-by: Mike Wadsten <[email protected]>

…e correct argument to list all runners (#8311) [PR #7790/787fa462 backport][stable-8] fix(modules/gitlab_runner): Use correct argument to list all runners (#8032) fix(modules/gitlab_runner): Use correct argument to list all runners (#7790) * fix(modules/gitlab_runner): Use correct argument to list all runners python-gitlab 4.0.0 removed support for the `as_list=False` parameter. This functionality is now available as `iterator=True`. Without this change, the module actually only retrieves the first 20 results, which can lead to non-idempotent behavior, such as registering a runner again. * Add changelog entry (#7790) * gitlab_runner: Check python-gitlab version when listing runners * gitlab: Add list_all_kwargs variable to module_utils * refactor(gitlab modules): use list_all_kwargs where it helps (#7790) I did not change every instance of all=True or all=False, only those which could obviously benefit from simplifying: * Code using `all=True` but then searching for any items that match a condition (no need to collect the full list). * Code that basically reimplements `all=True` with manual pagination. (These could be changed to `all=True`, but `list_all_kwargs` also sets per_page to 100, to gather data faster.) * gitlab_instance_variable: Use list_all_kwargs * Add new changelog entry for gitlab module changes (#7790) (cherry picked from commit 787fa46) Co-authored-by: patchback[bot] <45432694+patchback[bot]@users.noreply.github.com> Co-authored-by: Mike Wadsten <[email protected]>

…nsible-collections#7790) * fix(modules/gitlab_runner): Use correct argument to list all runners python-gitlab 4.0.0 removed support for the `as_list=False` parameter. This functionality is now available as `iterator=True`. Without this change, the module actually only retrieves the first 20 results, which can lead to non-idempotent behavior, such as registering a runner again. * Add changelog entry (ansible-collections#7790) * gitlab_runner: Check python-gitlab version when listing runners * gitlab: Add list_all_kwargs variable to module_utils * refactor(gitlab modules): use list_all_kwargs where it helps (ansible-collections#7790) I did not change every instance of all=True or all=False, only those which could obviously benefit from simplifying: * Code using `all=True` but then searching for any items that match a condition (no need to collect the full list). * Code that basically reimplements `all=True` with manual pagination. (These could be changed to `all=True`, but `list_all_kwargs` also sets per_page to 100, to gather data faster.) * gitlab_instance_variable: Use list_all_kwargs * Add new changelog entry for gitlab module changes (ansible-collections#7790)

ansibullbot added bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Dec 29, 2023

Add changelog entry (ansible-collections#7790)

c284404

ansibullbot removed the small_patch Hopefully easy to review label Dec 29, 2023

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-7 backport-8 Automatically create a backport for the stable-8 branch labels Dec 29, 2023

mikewadsten mentioned this pull request Dec 29, 2023

gitlab_runner can't handle more than 20 runners #7690

Closed

1 task

ansibullbot added the has_issue label Dec 29, 2023

gitlab_runner: Check python-gitlab version when listing runners

46ceb8a

felixfontein reviewed Dec 30, 2023

View reviewed changes

plugins/modules/gitlab_runner.py Outdated Show resolved Hide resolved

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 12, 2024

gitlab: Add list_all_kwargs variable to module_utils

ae28cec

ansibullbot added gitlab module_utils module_utils and removed stale_ci CI is older than 7 days, rerun before merging labels Jan 23, 2024

nejch approved these changes Jan 27, 2024

View reviewed changes

gitlab_instance_variable: Use list_all_kwargs

c48fedf

markuman reviewed Jan 30, 2024

View reviewed changes

plugins/module_utils/gitlab.py Show resolved Hide resolved

Add new changelog entry for gitlab module changes (ansible-collection…

938f1c3

…s#7790)

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 30, 2024

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Feb 10, 2024

nejch approved these changes Feb 25, 2024

View reviewed changes

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 25, 2024

felixfontein merged commit 787fa46 into ansible-collections:main Feb 25, 2024
121 checks passed

patchback bot mentioned this pull request Feb 25, 2024

[PR #7790/787fa462 backport][stable-8] fix(modules/gitlab_runner): Use correct argument to list all runners #8032

Merged

lgatellier mentioned this pull request May 5, 2024

[PR #7790/787fa462 backport][stable-7] fix(modules/gitlab_runner): Use correct argument to list all runners #8311

Merged

lgatellier mentioned this pull request May 19, 2024

gitlab_runner deletion via state: absent doesn't work properly #7716

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(modules/gitlab_runner): Use correct argument to list all runners #7790

fix(modules/gitlab_runner): Use correct argument to list all runners #7790

mikewadsten commented Dec 29, 2023

ansibullbot commented Dec 29, 2023

mikewadsten commented Dec 29, 2023

felixfontein commented Dec 29, 2023

mikewadsten commented Dec 29, 2023

felixfontein commented Jan 26, 2024

nejch commented Jan 27, 2024

ansibullbot commented Jan 27, 2024

mikewadsten commented Jan 27, 2024

mikewadsten commented Jan 27, 2024

ansibullbot commented Jan 27, 2024

felixfontein commented Jan 28, 2024

mikewadsten commented Jan 30, 2024

felixfontein commented Feb 11, 2024

felixfontein commented Feb 23, 2024

nejch left a comment

nejch Feb 25, 2024

felixfontein Feb 25, 2024

patchback bot commented Feb 25, 2024 •

edited

Loading

patchback bot commented Feb 25, 2024 •

edited

Loading

felixfontein commented Feb 25, 2024

felixfontein commented Feb 25, 2024

fix(modules/gitlab_runner): Use correct argument to list all runners #7790

fix(modules/gitlab_runner): Use correct argument to list all runners #7790

Conversation

mikewadsten commented Dec 29, 2023

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Dec 29, 2023

mikewadsten commented Dec 29, 2023

felixfontein commented Dec 29, 2023

mikewadsten commented Dec 29, 2023

felixfontein commented Jan 26, 2024

nejch commented Jan 27, 2024

ansibullbot commented Jan 27, 2024

mikewadsten commented Jan 27, 2024

mikewadsten commented Jan 27, 2024

ansibullbot commented Jan 27, 2024

felixfontein commented Jan 28, 2024

mikewadsten commented Jan 30, 2024

felixfontein commented Feb 11, 2024

felixfontein commented Feb 23, 2024

nejch left a comment

Choose a reason for hiding this comment

nejch Feb 25, 2024

Choose a reason for hiding this comment

felixfontein Feb 25, 2024

Choose a reason for hiding this comment

patchback bot commented Feb 25, 2024 • edited Loading

Backport to stable-7: 💔 cherry-picking failed — conflicts found

Backporting merged PR #7790 into main

patchback bot commented Feb 25, 2024 • edited Loading

Backport to stable-8: 💚 backport PR created

felixfontein commented Feb 25, 2024

felixfontein commented Feb 25, 2024

patchback bot commented Feb 25, 2024 •

edited

Loading

patchback bot commented Feb 25, 2024 •

edited

Loading