Add support for JSON & YAML output types #26

levikobi · 2023-06-19T14:27:56Z

What type of PR is this?

/kind feature
What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #22

Does this PR introduce a user-facing change?:

It adds support for specifying the output format - YAML or JSON, using an --output / -o flag,
when using the print subcommand.

LiorLieberman

Thanks @levikobi !

Overall it looks good to me!

As discussed offline, I am good with having this one without any tests now as we need to refactor the code a bit to be more extensible and testable.

Can you make sure you open a followup issue to for this work ?

go.mod

README.md

cmd/print.go

levikobi · 2023-06-21T04:58:07Z

Thanks for the review @LiorLieberman !
I've added the changes you requested, and opened a followup issue at #28

cmd/print_test.go

LiorLieberman · 2023-06-21T07:01:54Z

Aside from one small nit, looks good to me. Thanks!

/lgtm
/assign @robscott

LiorLieberman · 2023-06-21T14:32:07Z

Since you add more tests also need to change the test target in the makefile to include cmd/ tests

robscott

Thanks @levikobi!

go.mod

robscott · 2023-06-21T19:35:22Z

pkg/i2gw/ingress2gateway.go

+	if len(errors) > 0 {
+		fmt.Printf("# Encountered %d errors\n", len(errors))
+		for _, err := range errors {
+			fmt.Printf("# %s", err)
+		}
+	}


Maybe errors should also be output in JSON/YAML form?

Interesting thought. Which fields would you suggest adding to the JSON/YAML, in addition to an "errors" array? If you have an example, it would be great to see one.

No strong preference what it looks like, just if a user asks for json or yaml output it seems ideal to provide that format, even if the output is just a list of errors. Though I can be convinced against this, I can't remember what kubectl does here.

The default output format right now is YAML, which means that whether or not I specify an output format, I will never get a plain error message (always YAML/JSON). Personally, I can't think of any CLI tool that displays error messages in this way, especially in the Kubernetes ecosystem, but I may be wrong.

Overall, I'm not so sure about it. However, if you are up for it, then there's no problem.

yeah that's fair, I guess most CLI tools don't output error messages in yaml or JSON

cmd/print.go

robscott · 2023-06-21T19:36:43Z

cmd/print.go

+	case "json":
+		return &printers.JSONPrinter{}
+	default:
+		return nil


Any reason not to default to yaml? What would happen if we had a nil resource printer?

If I had defaulted to YAML and the users entered "i2gw print -o xml" for example, they would have still received a YAML format without any indication of what was wrong. In this way, if they don't specify anything, the default is YAML (outputFormat=""). Moreover, if they specify an incorrect format, they will receive a comprehensible error message.

Regarding your second point, I wonder if we should return an error when the output is not supported to make this function more intuitive. Or maybe just indicating in the function comment that a nil value may be returned..

I wonder if we should return an error when the output is not supported to make this function more intuitive

Yeah I think that's a good idea, fine with leaving that for a follow up though.

cmd/print.go

robscott · 2023-06-21T22:06:24Z

@LiorLieberman set up presubmits for this repo, let's see if they work:

/retest

levikobi · 2023-06-22T05:43:52Z

Thanks @levikobi!

Thank you for the review @robscott 🙂

LiorLieberman · 2023-06-22T07:30:30Z

/retest

LiorLieberman · 2023-06-22T07:36:02Z

Since we added the verify now, there are a bunch of errors from code that is already in master.

pkg/i2gw/ingress2gateway.go:68:27: G601: Implicit memory aliasing in for loop. (gosec)
		err := printer.PrintObj(&gateway, os.Stdout)
		                        ^
pkg/i2gw/ingress2gateway.go:75:27: G601: Implicit memory aliasing in for loop. (gosec)
		err := printer.PrintObj(&httpRoute, os.Stdout)
		                        ^
pkg/i2gw/ingress2gateway.go:56:6: var-naming: func ingresses2GatewaysAndHttpRoutes should be ingresses2GatewaysAndHTTPRoutes (revive)
func ingresses2GatewaysAndHttpRoutes(ingresses []networkingv1.Ingress) ([]gatewayv1beta1.HTTPRoute, []gatewayv1beta1.Gateway, []error) {
     ^
pkg/i2gw/aggregator_test.go:39:3: var-naming: struct field expectHttpRoutes should be expectHTTPRoutes (revive)
		expectHttpRoutes []gatewayv1beta1.HTTPRoute
		^
pkg/i2gw/aggregator_test.go:326:9: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
					if e != tc.expectErrors[i] {
					   ^
pkg/i2gw/aggregator.go:336:2: missing cases in switch of type v1.PathType: PathTypeImplementationSpecific (exhaustive)
	switch *ip.path.PathType {
	^
pkg/i2gw/aggregator.go:153:14: shadow: declaration of "errors" shadows declaration at line 134 (govet)
		httpRoute, errors := rg.toHTTPRoute()
		           ^
pkg/i2gw/aggregator.go:155:3: ineffectual assignment to errors (ineffassign)
		errors = append(errors, errors...)

levikobi · 2023-06-22T08:17:41Z

@LiorLieberman @robscott I've opened a new PR for pull-ingress2gateway-verify fixes at #29
When it will be merged I'll rebase this one with it

Co-authored-by: Rob Scott <[email protected]>

LiorLieberman · 2023-06-23T06:49:30Z

/retest

LiorLieberman · 2023-06-23T08:05:38Z

/lgtm

robscott · 2023-06-27T17:48:16Z

Thanks @levikobi!

/approve

k8s-ci-robot · 2023-06-27T17:48:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: levikobi, robscott

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:

~~OWNERS~~ [robscott]

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

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 19, 2023

k8s-ci-robot requested review from bowei and robscott June 19, 2023 14:28

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 19, 2023

levikobi force-pushed the input-format-options branch from 5792a68 to 44a32bc Compare June 19, 2023 14:29

levikobi mentioned this pull request Jun 19, 2023

Add support for JSON & YAML output types #23

Closed

LiorLieberman reviewed Jun 20, 2023

View reviewed changes

go.mod Show resolved Hide resolved

README.md Show resolved Hide resolved

cmd/print.go Outdated Show resolved Hide resolved

LiorLieberman reviewed Jun 20, 2023

View reviewed changes

cmd/print.go Outdated Show resolved Hide resolved

LiorLieberman reviewed Jun 21, 2023

View reviewed changes

cmd/print_test.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned robscott and LiorLieberman Jun 21, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023

LiorLieberman removed their assignment Jun 21, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023

robscott reviewed Jun 21, 2023

View reviewed changes

LiorLieberman mentioned this pull request Jun 22, 2023

Fix pull-ingress2gateway-verify presubmit #29

Merged

levikobi and others added 5 commits June 22, 2023 21:13

Add support for JSON & YAML output types

89001d4

Add comments and licensing information

ab7287d

Add unit test for output format flag

697f5a5

Add cmd to test target in Makefile

e7fa7e2

Apply suggestions from code review

7e2f1e7

Co-authored-by: Rob Scott <[email protected]>

levikobi force-pushed the input-format-options branch from 61f3b3a to 81ab218 Compare June 22, 2023 18:17

Add error output to getResourcePrinter function

86d6f8a

levikobi force-pushed the input-format-options branch from 81ab218 to 86d6f8a Compare June 23, 2023 05:28

k8s-ci-robot assigned LiorLieberman Jun 23, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2023

k8s-ci-robot merged commit 205b1c2 into kubernetes-sigs:main Jun 27, 2023

levikobi deleted the input-format-options branch June 27, 2023 18:08

levikobi mentioned this pull request Jul 26, 2023

REQUEST: New membership for levikobi kubernetes/org#4348

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for JSON & YAML output types #26

Add support for JSON & YAML output types #26

levikobi commented Jun 19, 2023 •

edited

Loading

LiorLieberman left a comment

levikobi commented Jun 21, 2023

LiorLieberman commented Jun 21, 2023

LiorLieberman commented Jun 21, 2023

robscott left a comment

robscott Jun 21, 2023

levikobi Jun 26, 2023

robscott Jun 26, 2023

levikobi Jun 27, 2023

robscott Jun 27, 2023

robscott Jun 21, 2023

levikobi Jun 22, 2023

robscott Jun 27, 2023

robscott commented Jun 21, 2023

levikobi commented Jun 22, 2023

LiorLieberman commented Jun 22, 2023

LiorLieberman commented Jun 22, 2023

levikobi commented Jun 22, 2023

LiorLieberman commented Jun 23, 2023

LiorLieberman commented Jun 23, 2023

robscott commented Jun 27, 2023

k8s-ci-robot commented Jun 27, 2023

Add support for JSON & YAML output types #26

Add support for JSON & YAML output types #26

Conversation

levikobi commented Jun 19, 2023 • edited Loading

LiorLieberman left a comment

Choose a reason for hiding this comment

levikobi commented Jun 21, 2023

LiorLieberman commented Jun 21, 2023

LiorLieberman commented Jun 21, 2023

robscott 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

Choose a reason for hiding this comment

robscott commented Jun 21, 2023

levikobi commented Jun 22, 2023

LiorLieberman commented Jun 22, 2023

LiorLieberman commented Jun 22, 2023

levikobi commented Jun 22, 2023

LiorLieberman commented Jun 23, 2023

LiorLieberman commented Jun 23, 2023

robscott commented Jun 27, 2023

k8s-ci-robot commented Jun 27, 2023

levikobi commented Jun 19, 2023 •

edited

Loading