Skip to content

fix: don't ignore 'as_list=False' for getting runners as iterator #2115

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

Closed
wants to merge 1 commit into from

Conversation

koreno
Copy link

@koreno koreno commented Jun 30, 2022

The gitlab.runners.all(as_list=False) was ignoring the flag, returning a list regardless

The `gitlab.runners.all(as_list=False)` was ignoring the flag, returning a list regardless
@nejch
Copy link
Member

nejch commented Jun 30, 2022

Thanks for your contribution @koreno!

I think we could get this in, but ideally we would get rid of the custom all() methods and convert the runners/all into a proper manager so we can use runners_all.list(iterator=True) directly using the ListMixin.

We did exactly the same for the members/all endpoint and deprecated all(): #1376, so we already have some precedent on how to do it (just ignore the AllMixin changes there, not relevant here).
I've wanted to do this for a while. Would you be interested in reworking the PR for that? Otherwise we'll start with a minimal change and follow-up.

That would actually fix #593 as well.

@koreno
Copy link
Author

koreno commented Jul 3, 2022

I'm gonna have to let you take over from here, I can't really afford to dig in much further...

@koreno koreno removed their assignment Jul 3, 2022
@nejch nejch self-assigned this Jul 3, 2022
@nejch
Copy link
Member

nejch commented Jul 3, 2022

I'm gonna have to let you take over from here, I can't really afford to dig in much further...

Thanks for your work on this @koreno. to avoid double work (e.g. getting your initial fix in and then tackling #593), I'll get to this ASAP.

@nejch
Copy link
Member

nejch commented Jul 23, 2022

Thanks again @koreno, I've opened #2167 to reuse our existing list() infrastructure and fix a related issue, replacing the custom method used here. Note that the new runners_all.list() will behave slightly differently, you'll still need to get() individual runners to get more details or manipulate them.

Should be released on the 28th.

@nejch nejch closed this Jul 23, 2022
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.

3 participants