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

frontend: CustomResourceInstancesList: Fix to work with multi cluster #2518

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

illume
Copy link
Collaborator

@illume illume commented Nov 4, 2024

Before there was an error, now they list and there is a cluster column.

(for single cluster mode it's the same as before)

image

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Nov 4, 2024
@illume illume added bug Something isn't working frontend Issues related to the frontend multi Multi cluster aggregated view labels Nov 4, 2024
@illume illume requested review from sniok and a team November 7, 2024 16:33
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't understand what the issue was from the commit description.

Comment on lines +13 to +17
const dataClassCrds = crds.map(crd => {
const crdClass = crd.makeCRClass();
const data = crdClass.useList({ cluster: crd.cluster });
return { data, crdClass, crd };
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep a useMemo for at least generating the classes here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I had tried that, but generating the classes doesn't take very much time.

If it's a problem for you, and not a nit... I can have a bit more of a think.

@illume
Copy link
Collaborator Author

illume commented Nov 8, 2024

Also, I don't understand what the issue was from the commit description.

The issue is the view completely failed to render.

Copy link
Contributor

@sniok sniok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working frontend Issues related to the frontend lgtm This PR has been approved by a maintainer multi Multi cluster aggregated view size:S This PR changes 10-29 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants