-
Notifications
You must be signed in to change notification settings - Fork 470
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
[gwctl] Expand the output of gwctl describe gatewayclass #2930
[gwctl] Expand the output of gwctl describe gatewayclass #2930
Conversation
Welcome @yashvardhan-kukreja! |
Hi @yashvardhan-kukreja. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Thanks for the work on this @yashvardhan-kukreja! @dprotaso I think you've got the most context on the applyconfigurations you just added, does this change to vendor look correct to you? |
We had previously decided to not commit vendor changes to gwctl sub folder (similar to how the parent folder does not have vendor). I think we can still continue with that? If we do decide to make vendor changes, it would be nice to do them in an independent PR for the first time. |
/assign |
@gauravkghildiyal: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
gwctl describe gatewayclass
/remove-kind gep |
So, the vendor-ed changes are only required to call these LOC so as to help resolving the right apiVersion and Kind. I presume, for the time being, we can hardcode them as This would help us get rid of this import hence, allowing us to get rid of vendor-ed changes as well. wdyt |
Yeah there's no reason to |
48f6f5a
to
01a2aa0
Compare
Hi everyone, I got rid of vendor/ and added it to .gitignore as well |
Hi @dprotaso @gauravkghildiyal , mind taking a look at this one? Thanks! |
Hi @mlavacca, |
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.
/ok-to-test
.gitignore
Outdated
@@ -45,6 +45,7 @@ Session.vim | |||
/_tmp/ | |||
*.un~ | |||
.vagrant | |||
vendor |
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.
vendor |
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.
I added this intentionally to ensure that the contributors don't end up committing the vendor/ (it seemed like only a build-time req. anyway).
Should I still remove it from gitignore?
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 need not block the PR, but we can likely skip this update in this PR, and if required send an independent one. Reason being, I actually don't recall why vendor was never added to .gitignore (although we infact don't commit vendor) -- this may just turn into another long discussion which may block this PR even longer.
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.
Gotcha makes sense!
fc9524a
to
65da0b6
Compare
Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
65da0b6
to
bcffb8b
Compare
apologies for the ping @mlavacca : just in case you missed the above commits and messages. Thank you |
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 @yashvardhan-kukreja! (and sorry for the delay)
Mostly looks good, though one thing: The tabular structure of DirectlyAttachedPolicies
field as mentioned in https://gateway-api.sigs.k8s.io/geps/gep-2722/ is actually intentional.
DirectlyAttachedPolicies:
TYPE NAME
---- ----
TimeoutPolicy.bar.com demo-timeout-policy-on-gatewayclass
Ideally it would be nice to have it in the same PR, but if you want to send a separate PR for that, lets keep the issue open after this PR merges (and not use "Fixed" in the PR description)
gwctl/pkg/printer/gatewayclasses.go
Outdated
|
||
// views ordered with respect to https://deploy-preview-2723--kubernetes-sigs-gateway-api.netlify.app/geps/gep-2722/#:~:text=gwctl%20describe%20gatewayclass%20foo%2Dcom%2Dexternal%2Dgateway%2Dclass |
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.
nit: You can either remove this comment, or change the link to a permanent one at https://gateway-api.sigs.k8s.io/geps/gep-2722/
gwctl/pkg/printer/gatewayclasses.go
Outdated
Kind: kind, | ||
}, | ||
{ | ||
Metadata: &metav1.ObjectMeta{ |
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.
Lets follow a consistent approach as:
gateway-api/gwctl/pkg/printer/policies.go
Lines 176 to 180 in 877803e
metadata := crd.ObjectMeta.DeepCopy() | |
metadata.Labels = nil | |
metadata.Annotations = nil | |
metadata.Name = "" | |
metadata.Namespace = "" |
where we remove duplicated fields from metadata and print the rest? (This would allow for possible future expansion of the fields within Metadata to be included by default)
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.
Sure, sounds neat
.gitignore
Outdated
@@ -45,6 +45,7 @@ Session.vim | |||
/_tmp/ | |||
*.un~ | |||
.vagrant | |||
vendor |
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 need not block the PR, but we can likely skip this update in this PR, and if required send an independent one. Reason being, I actually don't recall why vendor was never added to .gitignore (although we infact don't commit vendor) -- this may just turn into another long discussion which may block this PR even longer.
@@ -286,6 +287,11 @@ func fetchGatewayClasses(ctx context.Context, k8sClients *common.K8sClients, fil | |||
return []gatewayv1.GatewayClass{}, err | |||
} | |||
|
|||
// because api-server doesn't return TypeMeta in `gatewayClass` | |||
gcApplyConfig := apisv1beta1.GatewayClass(gatewayClass.Name) |
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.
Ah nice catch! This may just be a bit more involved.
The thing is, because we construct the gatewayClass object with the v1 versioned struct (i.e. &gatewayv1.GatewayClass{}
, client-go will fetch the GatewayClass for the v1 version. This means the correct APIVersion would correspond to the v1
package (and not apisv1beta1 = v1beta1):
gcApplyConfig := apisv1.GatewayClass(gatewayClass.Name)
Having said that, if we think about it a bit more, use of ApplyConfig isn't giving any real advantage here. How about something like this:
gatewayClass.APIVersion = gatewayv1.GroupVersion.String()
gatewayClass.Kind = reflect.TypeOf(gatewayClass).Elem().Name()
(Infact, using gatewayClass.Kind = "GatewayClass" would also just have been good enough)
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.
Ahh good point, will take care of this.
/label tide/merge-method-squash |
Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
Hi @gauravkghildiyal |
Makes sense though I noticed that the same concern is with other DescribeViews such as the ones corresponding to So, I'd suggest a separate PR addressing all of these ones. What do you think? |
@@ -286,6 +285,10 @@ func fetchGatewayClasses(ctx context.Context, k8sClients *common.K8sClients, fil | |||
return []gatewayv1.GatewayClass{}, err | |||
} | |||
|
|||
// because api-server doesn't return TypeMeta in `gatewayClass` |
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.
nit: kube-apiserver does infact return the TypeMeta (it always does)...this mishap happens when that object is decoded. We can likely add kubernetes/kubernetes#80609 as reference.
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.
ohh yeah of course it does, my bad (else kubectl-like clients would never show type meta lol), I meant client-go I guess haha.
Anyway, let me change this comment to a ref to the above issue instead.
Thanks for pointing it out!
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.
Mostly looks good, though one thing: The tabular structure of DirectlyAttachedPolicies field as mentioned in https://gateway-api.sigs.k8s.io/geps/gep-2722/ is actually intentional.
Makes sense though I noticed that the same concern is with other DescribeViews such as the ones corresponding to
Gateway
andHTTPRoute
So, I'd suggest a separate PR addressing all of these ones. What do you think?
Yep right! Sorry this is lack of clarity from my side that "Expand the output of XXX" is also expecting this change.
Sure lets tackle that in a separate PR for gatewayclass
Also, while you are editing the PR description to remove the "Fixed" keyword, FYI, we do infact want that particular format for the release note:
**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, please enter a release note below:
-->
```release-note
```
This should resolve the do-not-merge/release-note-label-needed
label
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauravkghildiyal, yashvardhan-kukreja 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 |
Whoops, ptal now. |
Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
@yashvardhan-kukreja sorry one last thing. Can you please keep the thing with the "release notes quote" a self contained statement -- even a one liner helps (in this case, it could simply just be the PR title itself, or equivalent) Thanks |
Looks good @gauravkghildiyal ? |
/lgtm |
What type of PR is this?
/kind gep
What this PR does / why we need it:
This PR introduces an expansion in the output of
gwctl describe gatewayclass/gatewayclasses <name>
as per the expectation of this GEP.Checkout the example in the last section.
Note on the "null" values of Labels/Annotations**
If you notice in the above output, the "empty" value of Labels is being parsed as "null" despite the GEP expecting it to be "" (analogous to how
kubectl describe
works with empty values).The reason this happens in our situation but not with
kubectl describe
is that we're using the YAML unmarshaler directly overLabels
which is of the type*map[string]string
and it inherently deals with the zero-values of pointers (to a map in this case) by stringifying it to "null" instead of "".On the other hand,
kubectl describe
doesn't seem to rely upon the YAML Unmarshaller at all [1]. It describe resources flexibly yet in the hard way giving itself the opportunity to realise the zero-ness of custom types like pointers, maps, etc. and spit out<NONE>
for them instead.What's the solution then?
*map[string]string
tomap[string]string
(without omitempty): This would print out{}
instead ofnull
- Not sure how would one be more favorable than the other thoughGatewayClassesPrinter) PrintDescribeView
[2] to not use YAML unmarshaller[3] at all. Instead it can use the Describer interface under kubectl. A very high-level skim across kubectl insinuates that we can use the GenericResourceDescriber[4] to help describing none-native (corev1, appsv1, etc.) types. This would help produce an output identical tokubectl describe gatewayclass <NAME>
and this would've its own slight differences with the GEP, so not sure it would be worth the effort.Which issue(s) this PR fixes:
Does this PR introduce a user-facing change?:
For example:
GatewayClass manifest applied
Previous output of
gwctl describe gatewayclass gatewayclass-observed-generation-bump
(Before this PR)New output of
gwctl describe gatewayclass gatewayclass-observed-generation-bump
(After this PR)References:
[0] Cause of vendor-ed changes
[1] Not a single mention of yaml package in the imports of kubectl's describe backend
[2] PrintDescribeView method
[3] Usage of YAML Unmarshal
[4] GenericResourceDescriber