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

Incorrect iteration over multiple selected namespaces #178

Closed
theboringstuff opened this issue Dec 12, 2023 · 2 comments · Fixed by #191
Closed

Incorrect iteration over multiple selected namespaces #178

theboringstuff opened this issue Dec 12, 2023 · 2 comments · Fixed by #191

Comments

@theboringstuff
Copy link

theboringstuff commented Dec 12, 2023

Describe the bug
When multiple namespaces are specified using -n option, e.g. -n ns1 -n ns2, KRR iterates these namespaces inconsistently, sometimes skipping some namespaces. For example, it lists deployments for ns1 only and statefulsets for ns2 only.

I suppose this code should bind namespace variable early before lambda (not late inside), e.g.

                tasks = [
                    loop.run_in_executor(
                        self.executor,
                        lambda ns=namespace: namespaced_request(
                            namespace=ns,
                            watch=False,
                            label_selector=settings.selector,
                        ),
                    )
                    for namespace in settings.namespaces
                ]

To Reproduce
Steps to reproduce the behavior:

  1. Create two namespaces, each with one Deployment and one StatefulSet
  2. Run KRR simple -n ns1, you will see 2 recommendations
  3. Run KRR simple -n ns2, you will see 2 recommendations
  4. Run KRR simple -n ns1 -n ns2. I expect to see 4 recommendations, but in fact there will be less (e.g. only 2).
@aantn
Copy link
Contributor

aantn commented Dec 14, 2023

Yep, you seem to be correct. Very nice catch.

Would you be interested in opening a PR to fix?

LeaveMyYard added a commit that referenced this issue Jan 10, 2024
@LeaveMyYard
Copy link
Contributor

Thanks @theboringstuff, good catch

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 a pull request may close this issue.

3 participants