-
Notifications
You must be signed in to change notification settings - Fork 302
Show alternative small scale home page clusters list #15571
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
Show alternative small scale home page clusters list #15571
Conversation
- we don't always have the mgmt cluster, so take this into account - i've checked all usages of `['management/byId'](MANAGEMENT.CLUSTER`
- more comments - rename of method to something more sensisble
- show alt list only if vai is on and there's more than X total clusters - list contains Y rows fetched via vai requested (capped and sorted)
837100b to
4e45ae6
Compare
751c3ce to
5803c9f
Compare
aalves08
left a comment
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.
@richard-cox didn't look at the code yet but gave it a test using your test cases as base. Here are the findings (all of this was tested in a Rancher system provisioned today - image is probably 12h old or around that time - v2.13-2e0b008dace3cc7ba6de6b74cd91f95275ac52bb-head):
-
Created 12 custom clusters
-
"Normal" settings (didn't change any setting,
vaiis on by default when rancher is provisioned:
Seems to be fine though, I would say. Sorting doesn't trigger any XHR requests
vaion, cluster threshold below 10:
met the conditions:
- alt table shown - Showing x of y message visible, age column visible
- changing sort / search does not result in API request
So I would say, looks fine
vaion, cluster threshold below 100:
I don't see any "Showing x message visible". Sorting does not trigger any XHR requests
- Vai
off:
- no age col
- no XHR requests with sorting
Overall
There's something weird with that message part... I haven't yet looked at with a standard user, but I was logged in as a base user when I ran your branch for the first time (vai on, no setting change) and the cluster list did not appear and it had an error on the console...
Could you define more clearly what is test 1, 2 and 3 on the test cases of your PR body so that I can compare better with the screenshots? not only that but the test cases are all text next to one another, so it's kinda confusing to check where one test case stops and another starts.
Also, how do I reset my settings to a "default"/"normal" value so that I could test this again?
…'t got access to any clusters - visually there's no difference
I've fixed this and it now matches the old behaviour (no list is shown). It's a bit of an edge case, as soon they gain access to a project / cluster then the cluster list will show as usual.
Done. It looks like your screenshots show this as working.
The default is just an empty string |
aalves08
left a comment
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.
As per our chat and the testing I've done, LGTM. Works as expected for standard and base users
|
/backport v2.12.3 release-2.12 |
|
Not creating port PR, there was an error running git am -3: |
…e-alt-list Show alternative small scale home page clusters list
Summary
Fixes #15570
Occurred changes and/or fixed issues
Technical notes summary
Areas or cases that should be tested
vai off
vai on - cluster count is over threshold of 10
/v3/settings/ui-performance,Showing x of ymessage visible, age column visiblevai on - cluster count is below threshold of 100
Showing xmessage visible, age column not visibleAreas which could experience regressions
no others outside of above tests
Previous Versions
1
2
3
Checklist