feat: print all resources #78

mlavacca · 2023-10-19T16:12:03Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

When reading the resources from a file, it is now possible to display all the resources, not only the Gateway API ones. Besides, the PROVIDER.md file has been properly formatted.

Which issue(s) this PR fixes:

Fixes #73

Does this PR introduce a user-facing change?:

By setting the flag `--all-resources`, it is now possible to print all the resources, not only the Gateway API ones. This flag is available only when reading the resources from a file.

k8s-ci-robot · 2023-10-19T16:12:05Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

levikobi

Thank you @mlavacca!
I have some questions regarding a few things but overall it's seems like another great contribution to me.

PROVIDER.md

cmd/print.go

pkg/i2gw/provider.go

mlavacca · 2024-05-02T13:40:27Z

I've revamped this PR after a long time of inactivity. I drastically changed the initial approach after all the changes that have been merged. The PR is 95% complete, there is some documentation missing and some minor cleanup, but I'd like to have the idea validated before adding the last bits.

Please, folks, take another look :)

/cc @LiorLieberman @levikobi

cmd/print.go

LiorLieberman · 2024-05-07T07:44:59Z

pkg/i2gw/providers/apisix/resource_reader.go

@@ -39,7 +38,7 @@ func (r *resourceReader) readResourcesFromCluster(ctx context.Context) (*storage
 	// read apisix related resources from cluster.
 	storage := newResourcesStorage()

-	ingresses, err := common.ReadIngressesFromCluster(ctx, r.conf.Client, ApisixIngressClass)
+	ingresses, err := i2gw.ReadIngressesFromCluster(ctx, r.conf.Client, ApisixIngressClass)


I needed to move the resource-reader files to the i2gw package to avoid the import cycle as with this change we read resources not only from the provider packages but from i2gw as well, as we want to read resources that do not have to be converted.

LiorLieberman · 2024-05-09T11:45:28Z

pkg/i2gw/ingress2gateway.go

+		if _, ok := FilteredResources[o.GetObjectKind().GroupVersionKind().GroupKind()]; ok {
+			continue


Trying to think if there is a different way to do it. I less like the fact that every provider needs to register its resources and that we depends on it.. I have not thought on anything smart yet.
But.. we should have all the resources the provider cares about (which are the resources that we need to filter out from the file) in its storage.. Maybe we can leverage that?

I guess we can add a GetCRDs() method to the provider interface. We have access to all the providers in i2gw package, so we can do the registration in this package. Instead of having each one registering his own CRDs

I followed @levikobi's suggestion and added a GetCRDs method to the provider interface. Let me know if it looks better now. In that case, I'll add documentation about the new method for the providers to implement.

cmd/print.go

levikobi · 2024-05-10T07:09:37Z

pkg/i2gw/ingress2gateway.go

+		if _, ok := FilteredResources[o.GetObjectKind().GroupVersionKind().GroupKind()]; ok {
+			continue


I guess we can add a GetCRDs() method to the provider interface. We have access to all the providers in i2gw package, so we can do the registration in this package. Instead of having each one registering his own CRDs

levikobi

@mlavacca you moved some logic from common to i2gw, can you please explain why?

mlavacca · 2024-06-05T08:32:33Z

@mlavacca you moved some logic from common to i2gw, can you please explain why?

I needed to move the resource-reader files to the i2gw package to avoid the import cycle as with this change we read resources not only from the provider packages but from i2gw as well, as we want to read resources that do not have to be converted.

Signed-off-by: Mattia Lavacca <[email protected]>

k8s-ci-robot · 2024-06-13T21:12:03Z

PR needs rebase.

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-sigs/prow repository.

LiorLieberman

Thanks @mlavacca, left some comments.
the move of logic from common to i2gw a bit bothers me because the function moved are actually common functions. Maybe we can come up with different package structure that will still accommodate the changes you introduced?

LiorLieberman · 2024-06-15T18:45:31Z

pkg/i2gw/ingress2gateway.go

@@ -30,7 +32,7 @@ import (
 	gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 )

-func ToGatewayAPIResources(ctx context.Context, namespace string, inputFile string, providers []string, providerSpecificFlags map[string]map[string]string) ([]GatewayResources, error) {
+func GetResources(ctx context.Context, namespace string, inputFile string, allresources bool, providers []string, providerSpecificFlags map[string]map[string]string) ([]client.Object, error) {


Can we leave some version of the old name (ToGatewayAPIResources) and return []client.Object, []client.Object, error ? GetResources does not really reflect what the function is doing

or []GatewayResources, []client.Object, error

LiorLieberman · 2024-06-15T18:47:50Z

pkg/i2gw/ingress2gateway.go

+	return append(gatewayResourcesToObjects(gatewayResources), genericResources...), nil
+}
+
+func gatewayResourcesToObjects(gatewayResources []GatewayResources) []client.Object {


Can we add a small function comment, also mention the order it returns the resources

LiorLieberman · 2024-06-15T18:50:29Z

pkg/i2gw/ingress2gateway.go

+	for _, p := range providerByName {
+		for _, crd := range p.GetCRDs() {
+			FilteredResources[crd] = struct{}{}
+		}
+	}


haven't thought much about it, but what are your thoughts on this approach VS every provider registers the resources he cares about in init().

I am not sure what I prefer, slight preference to what you currently have but wanted to discuss this

LiorLieberman · 2024-06-15T18:53:28Z

pkg/i2gw/providers/apisix/apisix.go

+func (p *Provider) GetCRDs() []schema.GroupKind {
+	return nil
+}


similar comment below on how we handle, provider == kong but ingress is gce. The implemented approach here would not print the gce ingress (although it hasn't been converted)

LiorLieberman · 2024-06-15T18:57:24Z

pkg/i2gw/resources.go

+		ingressClassGK: {},
+		ingressGK:      {},
+	}


re mentioning the comment about not differentiating between ingress of different providers when running with --all-resources

LiorLieberman · 2024-06-15T18:58:12Z

pkg/i2gw/utils.go

+	networkingv1beta1 "k8s.io/api/networking/v1beta1"
+)
+
+func GetIngressClass(ingress networkingv1.Ingress) string {


we already have similar function in common, why not using it?

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2023

k8s-ci-robot requested review from levikobi and Xunzhuo October 19, 2023 16:12

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 19, 2023

mlavacca had a problem deploying to release October 19, 2023 16:12 — with GitHub Actions Failure

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 20, 2023

mlavacca temporarily deployed to release October 20, 2023 10:46 — with GitHub Actions Inactive

mlavacca force-pushed the print-all-resources branch from 72189f5 to f8d7498 Compare October 20, 2023 10:48

mlavacca temporarily deployed to release October 20, 2023 10:49 — with GitHub Actions Inactive

mlavacca force-pushed the print-all-resources branch from f8d7498 to 247fa79 Compare October 20, 2023 13:45

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2023

mlavacca temporarily deployed to release October 20, 2023 13:46 — with GitHub Actions Inactive

mlavacca force-pushed the print-all-resources branch from 247fa79 to 2f0df00 Compare October 20, 2023 13:48

mlavacca temporarily deployed to release October 20, 2023 13:49 — with GitHub Actions Inactive

mlavacca force-pushed the print-all-resources branch from 2f0df00 to b5c5996 Compare October 20, 2023 14:28

mlavacca temporarily deployed to release October 20, 2023 14:28 — with GitHub Actions Inactive

mlavacca force-pushed the print-all-resources branch from b5c5996 to 2028440 Compare October 20, 2023 14:30

mlavacca temporarily deployed to release October 20, 2023 14:30 — with GitHub Actions Inactive

mlavacca force-pushed the print-all-resources branch from 2028440 to 304eac6 Compare October 20, 2023 14:31

mlavacca temporarily deployed to release October 20, 2023 14:31 — with GitHub Actions Inactive

mlavacca marked this pull request as ready for review October 20, 2023 14:35

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 Oct 20, 2023

mlavacca force-pushed the print-all-resources branch from 304eac6 to c431794 Compare October 20, 2023 15:14

mlavacca temporarily deployed to release October 20, 2023 15:14 — with GitHub Actions Inactive

levikobi reviewed Oct 22, 2023

View reviewed changes

mlavacca temporarily deployed to release October 23, 2023 08:12 — with GitHub Actions Inactive

mlavacca temporarily deployed to release May 2, 2024 13:31 — with GitHub Actions Inactive

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 2, 2024

mlavacca force-pushed the print-all-resources branch from 67f72e7 to b74031a Compare May 2, 2024 13:37

mlavacca temporarily deployed to release May 2, 2024 13:37 — with GitHub Actions Inactive

k8s-ci-robot requested review from levikobi and LiorLieberman May 2, 2024 13:40

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

mlavacca force-pushed the print-all-resources branch from b74031a to a540b97 Compare May 6, 2024 09:32

mlavacca temporarily deployed to release May 6, 2024 09:33 — with GitHub Actions Inactive

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

LiorLieberman reviewed May 9, 2024

View reviewed changes

levikobi reviewed May 10, 2024

View reviewed changes

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

mlavacca force-pushed the print-all-resources branch from a540b97 to 5de027b Compare June 5, 2024 08:31

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

mlavacca temporarily deployed to release June 5, 2024 08:31 — with GitHub Actions Inactive

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

mlavacca mentioned this pull request Jun 13, 2024

Support Gateway API extensions (output providers) #125

Open

mlavacca added 2 commits June 13, 2024 10:15

feat: --all-resources enables all resources fetch

f0b9fc2

address review comments

25cff88

Signed-off-by: Mattia Lavacca <[email protected]>

mlavacca force-pushed the print-all-resources branch from 5de027b to 25cff88 Compare June 13, 2024 08:23

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

mlavacca temporarily deployed to release June 13, 2024 08:23 — with GitHub Actions Inactive

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

LiorLieberman reviewed Jun 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: print all resources #78

feat: print all resources #78

mlavacca commented Oct 19, 2023 •

edited

Loading

k8s-ci-robot commented Oct 19, 2023

levikobi left a comment

mlavacca commented May 2, 2024

LiorLieberman May 7, 2024

mlavacca Jun 5, 2024

LiorLieberman May 9, 2024

levikobi May 10, 2024

mlavacca Jun 5, 2024

levikobi May 10, 2024

levikobi left a comment

mlavacca commented Jun 5, 2024

k8s-ci-robot commented Jun 13, 2024

LiorLieberman left a comment

LiorLieberman Jun 15, 2024

LiorLieberman Jun 15, 2024

LiorLieberman Jun 15, 2024

LiorLieberman Jun 15, 2024

LiorLieberman Jun 15, 2024

LiorLieberman Jun 15, 2024

LiorLieberman Jun 15, 2024

		if _, ok := FilteredResources[o.GetObjectKind().GroupVersionKind().GroupKind()]; ok {
		continue

feat: print all resources #78

Are you sure you want to change the base?

feat: print all resources #78

Conversation

mlavacca commented Oct 19, 2023 • edited Loading

k8s-ci-robot commented Oct 19, 2023

levikobi left a comment

Choose a reason for hiding this comment

mlavacca commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levikobi left a comment

Choose a reason for hiding this comment

mlavacca commented Jun 5, 2024

k8s-ci-robot commented Jun 13, 2024

LiorLieberman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlavacca commented Oct 19, 2023 •

edited

Loading