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

Added more fields - part 6. #485

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ericpromislow
Copy link
Contributor

@ericpromislow ericpromislow commented Feb 12, 2025

Related to #48603

@ericpromislow ericpromislow requested a review from a team as a code owner February 12, 2025 00:05
@ericpromislow ericpromislow marked this pull request as draft February 12, 2025 00:08
@ericpromislow
Copy link
Contributor Author

I thought there was a problem mapping an array to a single value, but apps.deployments was searchable, as in curl -kgsL 'https://localhost:5111/v1/apps.deployments?filter=spec.template.spec.containers.image=IMAGENAME worked.

I thought apps.daemonsets didn't work, but got confused between apps.daemonsets and apps.daemonset.

Turns out that if we're working on an accessor that ends on an array, like spec.template.spec.containers.image, our code the JS form of spec.template.spec.containers.map(c => c['image']).join("|") .

This means it's vital that the UI does a partial match when trying to match on .....containers.image

@ericpromislow
Copy link
Contributor Author

From my notes, the accessor for batch.cronjob needs to be spec.jobTemplate.spec.template.spec.containers.image because for whatever reason, s.t.s.c.i is wrapped by that outer spec.jt thing. Who knows why....

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

There's a couple missing

  • event
    • involvedObject.uid
  • management.cattle.io.projects
    • spec.clusterName

@@ -171,6 +171,9 @@ displayed by `kubectl get $TYPE`. For example `secrets` have `"metadata.fields[0
corresponding to `"name"`, `"type"`, `"data"`, and `"age"`. For CRDs, these come from
[Additional printer columns](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#additional-printer-columns)

When matching on array-type fields, the array's values are stored in the database as a single field separated by or-bars (`|`s).=
So searching for those fields needs to do a partial match when a field contains more than one value.
Copy link
Member

Choose a reason for hiding this comment

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

I've created rancher/rancher#49116 to address this

@tomleb tomleb requested a review from crobby February 18, 2025 16:44
@richard-cox
Copy link
Member

@ericpromislow @crobby how are we placed getting this in for feature complete?

@ericpromislow
Copy link
Contributor Author

@ericpromislow @crobby how are we placed getting this in for feature complete?

The PartialEquals operator ~ is supported so the UI should be able to take advantage of this capability. I personally haven't tested any of this with a version of the front-end that can do this, so can't say with full authority, but it should be in the backend.

When I discovered that we map an array of values to X|Y|Z I understood why before I got my hands on this, the UI did partial matches by default (leaving quotes off the target value).

I haven't thought yet about how collapsing an array of.values to a |-separated single string affects sorting. For one thing I'm not sure we can support sorting these collapsed fields.

@richard-cox
Copy link
Member

Regarding filtering (both exact and partial) and sorting on items in arrays, that's been spun out to rancher/rancher#49116.

There's a couple of requested fields (not array based) that are missing from the PR though (#485 (review)). If we can get those in we could progress this one

@ericpromislow
Copy link
Contributor Author

Tom added

	gvkKey("", "v1", "Event"): {
			…
			{"involvedObject", "uid"},
			…
		},

in commit 4477e2 on Jan. 13. That's why it isn't showing up in the changelist.

As for

management.cattle.io.projects

    spec.clusterName

it's in this PR:

$ curl -skL 'https://localhost:5111/v1/management.cattle.io.projects?filter=spec.clusterName=local' | jq .count
2
$ curl -skL 'https://localhost:5111/v1/management.cattle.io.projects?filter=spec.clusterName=flipo' | jq '.data | length'
0

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

Visually looks good. Future worked pulled out to rancher/rancher#49116.

@ericpromislow ericpromislow force-pushed the 48603-indexed-fields-part-6 branch from f496391 to fe9eb4b Compare February 26, 2025 02:18
@richard-cox
Copy link
Member

richard-cox commented Feb 26, 2025

@ericpromislow as this one is yet to merge, would there be any objections to adding one more?

provisioning.cattle.io.clusters

  • metadata.labels."provisioning.cattle.io/management-cluster-display-name"
  • metadata.annotations."provisioning.cattle.io/management-cluster-display-name"

@ericpromislow
Copy link
Contributor Author

I added it but then realized that we should be pulling in all labels now, and there's no need to add this field.

How do I coax a cluster to have this label to verify I don't need to explicitly add this field?

@richard-cox
Copy link
Member

Interesting point, has that part all merged to rancher/rancher? I was getting failure messages on a recent version.

I made a mistake, i'll edit the original comment to avoid confusion, but it's an annotation rather than label.

@ericpromislow
Copy link
Contributor Author

@richard-cox Can we move further field requests to another issue so we can get this one merged in? Otherwise the scope creep is simple, but it's preventing this from getting into the QA->release part of the pipeline

@richard-cox
Copy link
Member

Understood. Created rancher/rancher#49288, also for v2.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants