Fix missing default column name and age for CRDs #941
Conversation
| - [bar, foo, $skip, $duration] | ||
| - schemaID: timestamps.cattle.io.dates | ||
| expected: | ||
| - [the-date, $duration, $duration, $duration, $timestamp, $duration, $timestamp] |
There was a problem hiding this comment.
The $timestamp ones are because the type is a string for those columns
|
Seems like this also results in some issues where sorting doesn't work on fields from CRDs 🤔 |
| return nil, err | ||
| } | ||
|
|
||
| fieldSet := make(map[string]struct{}) |
There was a problem hiding this comment.
We want to avoid duplicated fields here.
We don't support multiple columns pointing to the same json path. Adding support will require more work from both UI and backend -> We need to have an ID for a field and its json path (or get rid of the json path altogether, but that brings its own set of complications).
|
Ok, it works with the UI change here: rancher/dashboard#16073 |
tomleb
force-pushed
the
columns-fix
branch
2 times, most recently
from
9e1fe71 to
6a3abac
Compare
| Name: "Name", | ||
| Type: "string", | ||
| Format: "name", | ||
| // Not setting description here because we don't set it above, might want to revisit |
There was a problem hiding this comment.
"above" doesn't really make sense here - assume this code was moved around
| - --wait=true | ||
| - fleet | ||
| - /home/shell/helm/fleet-108.0.0-up0.14.0-rc.1.tgz | ||
| # conditions: |
There was a problem hiding this comment.
What's with all the commented code?
ericpromislow
left a comment
ericpromislow
left a comment
There was a problem hiding this comment.
Just some minor things on comments. I ran tests, including new tests with old code (they failed as expected). There's a lot here, but I didn't find a reason not to merge it.
Does this issue moving out to 2.14.0 have any impact on your decision against making the "bigger change" that you reference above? |
3 participants
Issue rancher/rancher#52774
Unfortunately, we cannot get rid of those columns yet for two reasons:
Agecolumn. So instead of showing a pretty printed date, we getInvalid Datecolumn shown. Fixing that involves a bigger change (sending a timestamp instead of duration to the UI which I'm not willing to make from the little time we have before 2.13.1.So the fix here is adding the missing default columns and calling it a day for now. This PR adds a bunch of tests to try and cover as much as possible so that next time we do make changes, we'll know about regressions and what changes.
Note: While working on this, I discovered the filtering/sorting for fields for CRDs with column is broken. UI has created the following: rancher/dashboard#16071. Since this isn't a regression introduced by this PR, we should still review and merge it.