-
Notifications
You must be signed in to change notification settings - Fork 41
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
ctx: Look up cloud/connected Space ingresses in parallel #554
Conversation
Similar to our recent change for disconnected Spaces, make the organization view more responsive by looking up all the cloud and connected Space ingresses in parallel. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
cmd/up/ctx/navigation.go
Outdated
if err != nil { | ||
if errors.Is(err, spaces.SpaceConnectionError) { | ||
// we found the space to be unreachable | ||
return | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RedbackThomson Unchanged in this PR, but is it intentional that we proceed to add an item if we get an error other than SpaceConnectionError
? I suspect we might NPE below (*ingress
) in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I can't think of a reason why we would want to continue, so this would be an oversight. Ideally we wouldn't want to fail silently, though, since users just wouldn't see the space but also wouldn't know why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the continue
unconditional for now, and left a TODO to add unselectable items for the unreachable spaces. I think we want to show them regardless of what kind of error we hit, but may want different wording depending on whether it's a known error (SpaceConnectionError
) or something else.
if errors.Is(err, spaces.SpaceConnectionError) { | ||
// we found the space to be unreachable | ||
continue | ||
wg.Add(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we cap the wait group capacity to prevent this from spooling up too much if an org has a lot of connected spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense. Capped at 20 for now, chosen arbitrarily but with the thinking that it will let us fetch everything in parallel for the majority of users while keeping the number of concurrent requests reasonable.
Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
Description of your changes
Similar to our recent change for disconnected Spaces, make the organization view more responsive by looking up all the cloud and connected Space ingresses in parallel.
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR, as appropriate.How has this code been tested
Manual testing with the two orgs I'm part of.