Implement basic functionality of gwctl describe namespace #2836

Devaansh-Kumar · 2024-03-02T06:46:19Z

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 command

Which issue(s) this PR fixes:
Fixes #2795

Does this PR introduce a user-facing change?:

Implemented command `gwctl describe namespace`

k8s-ci-robot · 2024-03-02T06:46:29Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

Devaansh-Kumar · 2024-03-04T17:56:56Z

@gauravkghildiyal I have implemented the command as close as I could according to your guidlines.
The output for a sample test case come out as:

However I have one question about the format of output: If kubectl apply is used instead of kubectl create to create a namespace from a yaml file, then the annotation kubectl.kubernetes.io/last-applied-configuration is set which might not look very good in the output:

Do you think it is alright to leave it as it is?

gauravkghildiyal · 2024-03-05T16:13:46Z

/assign

gauravkghildiyal

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.

gauravkghildiyal · 2024-03-06T06:10:01Z

gwctl/pkg/resourcediscovery/nodes.go

@@ -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


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 accept namespaces ...corev1.Namespace and not need a separate addNamespaceWithLabelsAnnotationsStatus

Within Discoverer.discoverNamespaces(), we would fetch all corev1.Namespace resources. Then, while iterating over the existing resources, once we figure out the namespace of the resource, we call addNamespace() and pass in the corev1.Namespace that we would have already fetched.

Ok I understood what you are asking for

gauravkghildiyal · 2024-03-06T06:12:27Z

gwctl/pkg/printer/namespace.go

+	DirectlyAttachedPolicies []policymanager.ObjRef `json:",omitempty"`
+}
+
+func (nsp *NamespacesPrinter) PrintDescribeView(resourceModel *resourcediscovery.ResourceModel) {


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)

Yes I was planning to add that in a seperate PR, but I will do it in this one PR itself now.

gauravkghildiyal · 2024-03-06T06:31:55Z

gwctl/pkg/cmd/describe/describe.go

+
+		resourceModel, err := discoverer.DiscoverResourcesForNamespace(filter)
+		if err != nil {
+			panic(err)


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

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?

Devaansh-Kumar · 2024-03-06T18:13:12Z

@gauravkghildiyal I have made the suggested changes, please review them

gauravkghildiyal · 2024-03-08T07:05:48Z

Thanks @Devaansh-Kumar!

/lgtm
/approve

k8s-ci-robot · 2024-03-08T07:05:55Z

[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:

~~gwctl/OWNERS~~ [gauravkghildiyal]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gauravkghildiyal · 2024-03-08T07:11:49Z

@Devaansh-Kumar

Please use the provided template in the PR description to unblock the automatic merge

**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

```

gauravkghildiyal · 2024-03-08T07:15:15Z

/ok-to-test

gauravkghildiyal · 2024-03-08T07:17:13Z

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)

Devaansh-Kumar · 2024-03-08T10:16:51Z

@gauravkghildiyal I have fixed the failing test in httproute_test.go. The pull-gateway-api-test is now passing.

gauravkghildiyal · 2024-03-10T04:30:11Z

Thanks!

Please take not of #2836 (comment), and also rebase

gauravkghildiyal · 2024-03-10T04:30:30Z

/label tide/merge-method-squash

…dNamespace() function to accept corev1.Namespace

Devaansh-Kumar · 2024-03-10T07:06:03Z

@gauravkghildiyal I have rebased and updated the PR message

gauravkghildiyal · 2024-03-11T03:10:30Z

/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

k8s-ci-robot requested a review from gauravkghildiyal March 2, 2024 06:46

Devaansh-Kumar marked this pull request as ready for review March 4, 2024 17:57

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2024

k8s-ci-robot assigned gauravkghildiyal Mar 5, 2024

gauravkghildiyal reviewed Mar 6, 2024

View reviewed changes

gauravkghildiyal mentioned this pull request Mar 6, 2024

Implement gwctl get gatewayclass #2847

Merged

Devaansh-Kumar requested a review from gauravkghildiyal March 6, 2024 18:13

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 8, 2024

Devaansh-Kumar force-pushed the gwctl/describe-namespace branch from b2b8357 to 5bcffe8 Compare March 8, 2024 10:06

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2024

Devaansh-Kumar force-pushed the gwctl/describe-namespace branch from 5bcffe8 to 2fce3d5 Compare March 8, 2024 10:22

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2024

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 10, 2024

Devaansh-Kumar added 3 commits March 10, 2024 12:03

Implement basic functionality of gwctl describe namespace

6839ab2

Finalized printing of gwctl describe namespace command

26119f1

Added test for gwctl describe namespace and modified ResourceModel.ad…

790a9bb

…dNamespace() function to accept corev1.Namespace

Devaansh-Kumar force-pushed the gwctl/describe-namespace branch from 2fce3d5 to cabd7eb Compare March 10, 2024 06:33

Fixed failing httproutes test and Makefile

e3bbe15

Devaansh-Kumar force-pushed the gwctl/describe-namespace branch from cabd7eb to e3bbe15 Compare March 10, 2024 06:49

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2024

k8s-ci-robot merged commit 8e7ffc3 into kubernetes-sigs:main Mar 11, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement basic functionality of gwctl describe namespace #2836

Implement basic functionality of gwctl describe namespace #2836

Devaansh-Kumar commented Mar 2, 2024 •

edited

Loading

k8s-ci-robot commented Mar 2, 2024

Devaansh-Kumar commented Mar 4, 2024

gauravkghildiyal commented Mar 5, 2024

gauravkghildiyal left a comment

gauravkghildiyal Mar 6, 2024

Devaansh-Kumar Mar 6, 2024 •

edited

Loading

gauravkghildiyal Mar 6, 2024

Devaansh-Kumar Mar 6, 2024

gauravkghildiyal Mar 6, 2024

Devaansh-Kumar Mar 6, 2024

Devaansh-Kumar commented Mar 6, 2024 •

edited

Loading

gauravkghildiyal commented Mar 8, 2024

k8s-ci-robot commented Mar 8, 2024

gauravkghildiyal commented Mar 8, 2024

gauravkghildiyal commented Mar 8, 2024

gauravkghildiyal commented Mar 8, 2024

Devaansh-Kumar commented Mar 8, 2024

gauravkghildiyal commented Mar 10, 2024

gauravkghildiyal commented Mar 10, 2024

Devaansh-Kumar commented Mar 10, 2024

gauravkghildiyal commented Mar 11, 2024

Implement basic functionality of gwctl describe namespace #2836

Implement basic functionality of gwctl describe namespace #2836

Conversation

Devaansh-Kumar commented Mar 2, 2024 • edited Loading

k8s-ci-robot commented Mar 2, 2024

Devaansh-Kumar commented Mar 4, 2024

gauravkghildiyal commented Mar 5, 2024

gauravkghildiyal left a comment

Choose a reason for hiding this comment

gauravkghildiyal Mar 6, 2024

Choose a reason for hiding this comment

Devaansh-Kumar Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

gauravkghildiyal Mar 6, 2024

Choose a reason for hiding this comment

Devaansh-Kumar Mar 6, 2024

Choose a reason for hiding this comment

gauravkghildiyal Mar 6, 2024

Choose a reason for hiding this comment

Devaansh-Kumar Mar 6, 2024

Choose a reason for hiding this comment

Devaansh-Kumar commented Mar 6, 2024 • edited Loading

gauravkghildiyal commented Mar 8, 2024

k8s-ci-robot commented Mar 8, 2024

gauravkghildiyal commented Mar 8, 2024

gauravkghildiyal commented Mar 8, 2024

gauravkghildiyal commented Mar 8, 2024

Devaansh-Kumar commented Mar 8, 2024

gauravkghildiyal commented Mar 10, 2024

gauravkghildiyal commented Mar 10, 2024

Devaansh-Kumar commented Mar 10, 2024

gauravkghildiyal commented Mar 11, 2024

Devaansh-Kumar commented Mar 2, 2024 •

edited

Loading

Devaansh-Kumar Mar 6, 2024 •

edited

Loading

Devaansh-Kumar commented Mar 6, 2024 •

edited

Loading