-
Notifications
You must be signed in to change notification settings - Fork 491
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
Implement basic functionality of gwctl describe namespace #2836
Implement basic functionality of gwctl describe namespace #2836
Conversation
Hi @Devaansh-Kumar. 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. |
@gauravkghildiyal I have implemented the command as close as I could according to your guidlines. However I have one question about the format of output: If Do you think it is alright to leave it as it is? |
/assign |
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 is almost there Devaansh! Just a couple of suggestions and we should be good. Thanks you!
On the topic of whether to exclude last-applied-configuration
, I think leaving that should be just fine. We will always have the possibility of some line spreading across multiple lines (based on the width of the output screen) but that should normally not be reason enough to remove that field entirely.
gwctl/pkg/resourcediscovery/nodes.go
Outdated
@@ -246,11 +247,15 @@ func (b *BackendNode) ID() backendID { | |||
) | |||
} | |||
|
|||
// HTTPRouteNode models the relationships and dependencies of a Namespace. | |||
// NamespaceNode models the relationships and dependencies of a Namespace. | |||
type NamespaceNode struct { | |||
// NamespaceName identifies the Namespace. | |||
NamespaceName string |
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 replace the NamespaceName with the actual corev1.Namespace
resource. This would make this node consistent with how the other nodes are defined -- where nodes are simply wrappers over the underlying base resource. This would also mean that we don't need to define each field of the Namepace individually in the node here.
This means that now:
- We can modify the
ResourceModel.addNamespace()
to acceptnamespaces ...corev1.Namespace
and not need a separateaddNamespaceWithLabelsAnnotationsStatus
- Within
Discoverer.discoverNamespaces()
, we would fetch allcorev1.Namespace
resources. Then, while iterating over the existing resources, once we figure out the namespace of the resource, we calladdNamespace()
and pass in thecorev1.Namespace
that we would have already fetched.
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 I understood what you are asking for
DirectlyAttachedPolicies []policymanager.ObjRef `json:",omitempty"` | ||
} | ||
|
||
func (nsp *NamespacesPrinter) PrintDescribeView(resourceModel *resourcediscovery.ResourceModel) { |
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 add a test for this as well. (You can use some other tests like those for gateway and httproute for examples on how they can be written)
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.
Yes I was planning to add that in a seperate PR, but I will do it in this one PR itself now.
gwctl/pkg/cmd/describe/describe.go
Outdated
|
||
resourceModel, err := discoverer.DiscoverResourcesForNamespace(filter) | ||
if err != nil { | ||
panic(err) |
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.
Forgot to call this out. @pottekkat is working on fixing the panics that we currently have in the codebase. Lets try to ensure that the new code we're adding tries to properly write to the correct output stream (instead of panic'ing)
Ref #2825 for relevant changes
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.
Understood, I need to just replace panic(err)
with
fmt.Fprintf(os.Stderr, "relevant error message: %v\n", err)
os.Exit(1)
wherever I have made code changes right?
@gauravkghildiyal I have made the suggested changes, please review them |
Thanks @Devaansh-Kumar! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Devaansh-Kumar, gauravkghildiyal 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 |
Please use the provided template in the PR description to unblock the automatic merge
|
/ok-to-test |
Looks like there's a couple of tests as well that will need to be fixed as part of the change. :) (Shouldn't likely be very problematic) |
b2b8357
to
5bcffe8
Compare
@gauravkghildiyal I have fixed the failing test in |
5bcffe8
to
2fce3d5
Compare
Thanks! Please take not of #2836 (comment), and also rebase |
/label tide/merge-method-squash |
…dNamespace() function to accept corev1.Namespace
2fce3d5
to
cabd7eb
Compare
cabd7eb
to
e3bbe15
Compare
@gauravkghildiyal I have rebased and updated the PR message |
/lgtm |
…-sigs#2836) * Implement basic functionality of gwctl describe namespace * Finalized printing of gwctl describe namespace command * Added test for gwctl describe namespace and modified ResourceModel.addNamespace() function to accept corev1.Namespace * Fixed failing httproutes test and Makefile
What type of PR is this?
Add one of the following kinds:
/area gwctl
/kind feature
/cc @gauravkghildiyal
What this PR does / why we need it:
Implement
gwctl describe namespace
commandWhich issue(s) this PR fixes:
Fixes #2795
Does this PR introduce a user-facing change?: