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: ClusterNotFoundPopup: Fix to work with multi clusters #2512

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

illume
Copy link
Collaborator

@illume illume commented Nov 3, 2024

Now when there's a wrong cluster name in the URL when using multiple clusters it also gives a cluster not found alert message for each cluster not found.

NOTE: A maximum of only two alerts will be shown.

How to test

  • Add two clusters with + between then in the URL, one should be wrong, then see cluster not found message

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Nov 3, 2024
@illume illume added bug Something isn't working frontend Issues related to the frontend multi Multi cluster aggregated view labels Nov 3, 2024
@illume illume requested review from a team and vyncent-t November 4, 2024 06:40
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.

Does this show a popup per cluster? I think this UX could be unnerving for the user. I had actually disabled the popup when in multi-cluster.
c42f7b2

Can't remember exactly but I think I had tried to add the cluster problems in the main view, to be expanded at will.

@illume
Copy link
Collaborator Author

illume commented Nov 8, 2024

I don't think completely disabling the alert is a good idea.

It's most likely every single cluster won't break at the same time.

How about:

  • There is already a note in the main view cluster list about the clusters being down.
  • I limit the amount of popups for max 2 clusters?

@illume illume requested a review from joaquimrocha November 8, 2024 10:07
@illume
Copy link
Collaborator Author

illume commented Nov 18, 2024

@joaquimrocha please respond to this comment? #2512 (comment)

@joaquimrocha
Copy link
Collaborator

I don't think completely disabling the alert is a good idea.

It's most likely every single cluster won't break at the same time.

How about:

  • There is already a note in the main view cluster list about the clusters being down.
  • I limit the amount of popups for max 2 clusters?

Okay. Let's try this. If it gets unnerving, we can act on that.

@joaquimrocha joaquimrocha force-pushed the ClusterNotFoundPopup-multi branch from 948359b to 60333ac Compare November 27, 2024 09:57
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Nov 27, 2024
@joaquimrocha
Copy link
Collaborator

Solved a conflict it had.

@illume illume force-pushed the ClusterNotFoundPopup-multi branch from 60333ac to f4f512f Compare December 27, 2024 08:15
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 27, 2024
@illume
Copy link
Collaborator Author

illume commented Dec 27, 2024

I don't think completely disabling the alert is a good idea.
It's most likely every single cluster won't break at the same time.
How about:

  • There is already a note in the main view cluster list about the clusters being down.
  • I limit the amount of popups for max 2 clusters?

Okay. Let's try this. If it gets unnerving, we can act on that.

I made it so only a maximum of only two alerts will be shown.

Now when there's a wrong cluster name in the URL when using
multiple clusters it also gives a cluster not found alert
message for each cluster not found.

NOTE: A maximum of only two alerts will be shown.

Signed-off-by: René Dudfield <renedudfield@microsoft.com>
@illume illume force-pushed the ClusterNotFoundPopup-multi branch from f4f512f to 3812f0f Compare January 3, 2025 07:51
@illume
Copy link
Collaborator Author

illume commented Jan 3, 2025

rebased against main

Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Tested. Lgtm. Thanks

@illume illume dismissed joaquimrocha’s stale review January 3, 2025 08:37

change to limit maximum number of alerts was done

@illume illume merged commit 69d4e12 into main Jan 3, 2025
18 checks passed
@illume illume deleted the ClusterNotFoundPopup-multi branch January 3, 2025 08:38
@illume illume self-assigned this Jan 6, 2025
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 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