-
Notifications
You must be signed in to change notification settings - Fork 512
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
Improve utilities for gwctl describe and add more info to gateway descriptions #2999
Improve utilities for gwctl describe and add more info to gateway descriptions #2999
Conversation
f46ed0e
to
f45d005
Compare
/cc @mlavacca |
b9e0944
to
a366c36
Compare
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.
Apart from a very minor nit in a comment, it looks good to me. I unfortunately do not feel 100% confident in reviewing this code as I lack some context. I'd love to hear some other opinions as well here :)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauravkghildiyal, mlavacca The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ated with a set of Gateways
a366c36
to
b04e809
Compare
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.
Thanks for all the work on this @gauravkghildiyal!
eventList := &corev1.EventList{} | ||
options := &client.ListOptions{ | ||
FieldSelector: fields.AndSelectors( | ||
fields.OneTermEqualSelector("involvedObject.kind", "Gateway"), |
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.
Wouldn't you also need group here? Otherwise you might end up with some unexpected data related to other Gateway resources like Istio Gateways.
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.
Great catch!
Indeed we would need something of that sorts...the right choice seems to be the UID (which is what kubectl uses as well). Reason being, there's no neat way to use the API to only query the APIGroup (and not the APIVersion) which would have caused problems with gatewayv1 vs gatewayv1beta1. Anyways, UID doesn't cause any such issues.
Thanks a lot for the great find
for _, gatewayNode := range resourceModel.Gateways { | ||
eventList := &corev1.EventList{} | ||
options := &client.ListOptions{ | ||
FieldSelector: fields.AndSelectors( |
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.
This seems like it would get painful, would it be better to require that Gateway controllers set specific labels on Events they're publishing?
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.
Anything specific in your mind about the pain points? I guess some should get addressed as described in the other comment https://github.com/kubernetes-sigs/gateway-api/pull/2999/files#r1585597856 and we may not need it for the time being.
gwctl/pkg/common/clients.go
Outdated
// extractorFunc are used to build an index for Event objects. This is | ||
// required to properly mock things like the following when listing Events | ||
// associated with a resource: | ||
// | ||
// fields.OneTermEqualSelector("involvedObject.kind", "Gateway") |
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.
If I'm understanding correctly, this is more for just test mocks or something right? It looks like you're using this to list events. Maybe adjust the description to note that this is the best way to be able to list events that involve a specific resource?
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.
You are absolutely right. Yes the comment could use some improvement, thanks for the suggestion. Hope it's somewhat more meaningful now.
Thanks @gauravkghildiyal! /lgtm |
What this PR does / why we need it:
gwctl describe
views and TablesExample:
Notes for reviewer:
Reviewing individual commits in order might be easier
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: