feat: add --output-file flag (#145) #150

ipsum-0320 · 2024-04-06T13:56:33Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
To separate the output and logs, we have introduced a new --output-file flag to allow users to dump the converted resources into a specific file, while the console only outputs log messages/warnings. The --output-file has the following requirements:

By default, --output-file is "./ingress2gateway.yaml" or "./ingress2gateway.json", depending on whether the output flag is set to "yaml" or "json".
If the user customizes the filename, the file must have a ".yaml" extension (when the output flag is "yaml"), or a ".json" extension (when the output flag is "json").

Which issue(s) this PR fixes:

Fixes #145

Does this PR introduce a user-facing change?:

Added a new `--output-file` flag to support dumping converted resources into a specific file and logs/warnings to the console.

linux-foundation-easycla · 2024-04-06T13:56:37Z

The committers listed above are authorized under a signed CLA.

✅ login: ipsum-0320 / name: ipsum-0320 (580b0fe)

k8s-ci-robot · 2024-04-06T13:56:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ipsum-0320
Once this PR has been reviewed and has the lgtm label, please assign robscott for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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 · 2024-04-06T13:56:42Z

Welcome @ipsum-0320!

It looks like this is your first PR to kubernetes-sigs/ingress2gateway 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/ingress2gateway has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-04-06T13:56:43Z

Hi @ipsum-0320. 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.

ipsum-0320 · 2024-04-06T14:32:53Z

@Xunzhuo @levikobi PTAL 😊

Xunzhuo · 2024-04-06T15:09:51Z

/ok-to-test

ipsum-0320 · 2024-04-07T02:31:30Z

@Xunzhuo @levikobi can u review the code again? Thanks! 😊

ipsum-0320 · 2024-04-08T08:55:44Z

@LiorLieberman @mlavacca Could you please review my code? The other two reviewers didn't seem to have time. I have a lot of time to contribute to the project, please feel free to give me your insights on my code 😊

levikobi · 2024-04-10T05:49:08Z

sorry for not getting to this one yet @ipsum-0320, I will get to it within the next two days

ipsum-0320 · 2024-04-12T05:38:54Z

sorry for not getting to this one yet @ipsum-0320, I will get to it within the next two days

@levikobi ok, thanks!

levikobi

Looks like this gets the job done
Thanks @ipsum-0320

/lgtm
/cc @LiorLieberman @mlavacca

levikobi · 2024-04-11T08:49:50Z

cmd/print.go

@@ -235,6 +281,9 @@ func newPrintCommand() *cobra.Command {
 	cmd.Flags().StringVarP(&pr.outputFormat, "output", "o", "yaml",
 		fmt.Sprintf(`Output format. One of: (%s)`, strings.Join(allowedFormats, ", ")))

+	cmd.Flags().StringVarP(&pr.outputFile, "output-file", "", "",
+		"Path to the  conversion result file, the tool will output the conversion result to the file. Value pattern should be *.yaml of *.json (default \"./ingress2gateway.yaml or ./ingress2gateway.json\")")


Suggested change

"Path to the conversion result file, the tool will output the conversion result to the file. Value pattern should be *.yaml of *.json (default \"./ingress2gateway.yaml or ./ingress2gateway.json\")")

"Path to the conversion result file, the tool will output the conversion result to the file. Value pattern should be *.yaml of *.json (default \"./ingress2gateway.yaml or ./ingress2gateway.json\")")

LiorLieberman · 2024-04-14T05:50:30Z

Thanks @ipsum-0320.
This PR intends to always dump the results to a file? If yes, why dont we maintain the current functionality to print the results to the console while letting people the option to specify an output-file?

/hold

ipsum-0320 · 2024-04-14T15:57:28Z

Thanks @ipsum-0320. 谢谢。 This PR intends to always dump the results to a file? If yes, why dont we maintain the current functionality to print the results to the console while letting people the option to specify an output-file? 这个 PR 打算总是将结果转储到文件中吗？如果是，为什么我们不保留当前的功能以将结果打印到控制台，同时让人们可以选择指定输出文件？

/hold /抓住

hi, @LiorLieberman . As mentioned in issue #145, if we output any other log messages to the console, such messages will also be dumped into the output file, thus contaminating the .yaml/.json file.

LiorLieberman · 2024-04-14T16:06:28Z

That I understand. I am saying why we default to a file? Why we dont keep the default to the console ?

ipsum-0320 · 2024-04-14T16:14:03Z

That I understand. I am saying why we default to a file? Why we dont keep the default to the console ?

Oh, @LiorLieberman I think if we force it to output to a file, we can completely avoid contaminating the conversion results with log messages. Of course, outputting to the console is more convenient... I'll make it optional.

k8s-ci-robot · 2024-04-15T07:03:36Z

New changes are detected. LGTM label has been removed.

ipsum-0320 · 2024-04-16T02:51:55Z

@levikobi @LiorLieberman hi guys, PTAL 😊

mlavacca · 2024-04-16T09:56:32Z

.gitignore

+
+# IDE file
+.idea/
+.DS_Store


I'd prefer not to have IDE-based fields in the .gitignore. You can add these entries in your global .gitignore to avoid committing these folders in all your local projects.

okok, got it.

mlavacca · 2024-04-16T10:01:59Z

cmd/print.go

+	if !strings.HasSuffix(pr.outputFile, ".json") && !strings.HasSuffix(pr.outputFile, ".yaml") {
+		return fmt.Errorf("output file must have be *.json or *.yaml")
+	}
+	if strings.HasSuffix(pr.outputFile, ".json") && pr.outputFormat != "json" {
+		return fmt.Errorf("output format is yaml, but output file is *.json")
+	}
+	if strings.HasSuffix(pr.outputFile, ".yaml") && pr.outputFormat != "yaml" {
+		return fmt.Errorf("output format is json, but output file is *.yaml")
+	}


I don't think we need such an enforcement/validation. I'd give users the possibility to choose the filename they prefer, even without the correct extension.

I just want to prevent confusion, maybe I can change it to output a warning instead of an error.

mlavacca · 2024-04-16T10:02:36Z

cmd/print.go

+		err := file.Close()
+		if err != nil {
+			fmt.Println("Error closing file:", err)
+			return


Suggested change

return

This return is not needed here

Yes, of course, got it.

mlavacca · 2024-04-16T10:08:41Z

cmd/print.go

+	if resourceCount == 0 {
+		err := os.Remove(pr.outputFile)
+		if err != nil {
+			fmt.Printf("Error removing file %s: %v\n", pr.outputFile, err)
+		}
+	}


Creating a file upfront and deleting it if no resources are displayed is a bit dirty in my opinion. I'd slightly refactor the outputResult function to extract the count of objects into a dedicated function. Before calling outputResult we can check if there are resources. If not, we do not need to create any file and we can just move on.

ok, got it.

ipsum-0320 · 2024-04-24T08:03:05Z

@mlavacca PTAL 😊

k8s-ci-robot · 2024-05-03T16:49:58Z

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/test-infra repository.

mlavacca · 2024-05-02T10:45:38Z

.gitignore

@@ -25,4 +25,4 @@ go.work
 /tmp

 # GolangCI-Lint cache
-/cache
+/cache


Suggested change

/cache

/cache

mlavacca · 2024-05-07T07:38:22Z

README.md

There is something weird with this table, it looks like it has been entirely reformatted.

mlavacca · 2024-05-07T07:41:01Z

cmd/print.go

+	if pr.outputFile == "" {
+		return
+	}


We need to validate that outputFile is not "" when the flag is enabled upfront. If the user provides such a flag without a value, we just exit with an error

mlavacca · 2024-05-07T07:42:26Z

cmd/print.go

+	if !strings.HasSuffix(pr.outputFile, ".json") && !strings.HasSuffix(pr.outputFile, ".yaml") {
+		fmt.Println("Warning: output file should be *.json or *.yaml")
+	} else if strings.HasSuffix(pr.outputFile, ".json") && pr.outputFormat != "json" {
+		fmt.Println("Warning: output format is yaml, but output file is *.json")
+	} else if strings.HasSuffix(pr.outputFile, ".yaml") && pr.outputFormat != "yaml" {
+		fmt.Printf("Warning: output format is json, but output file is *.yaml")
+	}
+	return
+}


I do not think we need this logic at all, we should use the file provided by the user regardless the extension in my opinion, without logging any specific message

mlavacca · 2024-05-07T07:43:24Z

cmd/print.go

@@ -235,6 +286,9 @@ func newPrintCommand() *cobra.Command {
 	cmd.Flags().StringVarP(&pr.outputFormat, "output", "o", "yaml",
 		fmt.Sprintf(`Output format. One of: (%s)`, strings.Join(allowedFormats, ", ")))

+	cmd.Flags().StringVarP(&pr.outputFile, "output-file", "", "",
+		"Path to the  conversion result file, the tool will output the conversion result to the file. Value pattern should be *.yaml of *.json. If not set, the tool will output to stdout")


we should not enforce any specific validation on the file extension. Users should be able to use whatever file they prefer

mlavacca · 2024-05-07T07:44:35Z

cmd/print.go

-			msg = fmt.Sprintf("%s in %s namespace", msg, pr.namespaceFilter)
-		}
-		fmt.Println(msg)
+func (pr *PrintRunner) output2File(gatewayResources []i2gw.GatewayResources) {


Suggested change

func (pr *PrintRunner) output2File(gatewayResources []i2gw.GatewayResources) {

func (pr *PrintRunner) outputToFile(gatewayResources []i2gw.GatewayResources) {

mlavacca · 2024-05-07T07:44:48Z

cmd/print.go

+		}
+	} else {
+		if pr.checkResourceCount(gatewayResources) {
+			pr.output2File(gatewayResources)


Suggested change

pr.output2File(gatewayResources)

pr.outputToFile(gatewayResources)

mlavacca · 2024-07-04T12:42:52Z

👋 @ipsum-0320, how are things going? If you need any help in addressing the comments and move this one forward, please let us know

k8s-triage-robot · 2024-10-02T13:22:26Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-11-01T14:08:54Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 6, 2024

k8s-ci-robot requested review from levikobi and Xunzhuo April 6, 2024 13:56

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 6, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 6, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 6, 2024

ipsum-0320 force-pushed the feat/output-flag branch 2 times, most recently from 610ff69 to 1a676d5 Compare April 6, 2024 14:15

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 6, 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 Apr 6, 2024

ipsum-0320 force-pushed the feat/output-flag branch from 1a676d5 to 33c2a26 Compare April 6, 2024 17:09

levikobi reviewed Apr 14, 2024

View reviewed changes

k8s-ci-robot requested review from LiorLieberman and mlavacca April 14, 2024 05:33

k8s-ci-robot assigned levikobi Apr 14, 2024

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2024

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

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 Apr 15, 2024

ipsum-0320 force-pushed the feat/output-flag branch from 501c969 to 5fc17e8 Compare April 16, 2024 02:48

mlavacca requested changes Apr 16, 2024

View reviewed changes

ipsum-0320 force-pushed the feat/output-flag branch from 5fc17e8 to d42332c Compare April 22, 2024 14:52

feat: add --output-file flag(kubernetes-sigs#145)

580b0fe

ipsum-0320 force-pushed the feat/output-flag branch from d42332c to 580b0fe Compare April 22, 2024 14:56

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 reviewed May 7, 2024

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 2, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 1, 2024

	"Path to the conversion result file, the tool will output the conversion result to the file. Value pattern should be .yaml of .json (default \"./ingress2gateway.yaml or ./ingress2gateway.json\")")
	"Path to the conversion result file, the tool will output the conversion result to the file. Value pattern should be .yaml of .json (default \"./ingress2gateway.yaml or ./ingress2gateway.json\")")

	func (pr *PrintRunner) output2File(gatewayResources []i2gw.GatewayResources) {
	func (pr *PrintRunner) outputToFile(gatewayResources []i2gw.GatewayResources) {

	pr.output2File(gatewayResources)
	pr.outputToFile(gatewayResources)

feat: add --output-file flag (#145) #150

Are you sure you want to change the base?

feat: add --output-file flag (#145) #150

Conversation

ipsum-0320 commented Apr 6, 2024 • edited by LiorLieberman Loading

linux-foundation-easycla bot commented Apr 6, 2024 • edited Loading

k8s-ci-robot commented Apr 6, 2024

k8s-ci-robot commented Apr 6, 2024

k8s-ci-robot commented Apr 6, 2024

ipsum-0320 commented Apr 6, 2024

Xunzhuo commented Apr 6, 2024

ipsum-0320 commented Apr 7, 2024

ipsum-0320 commented Apr 8, 2024

levikobi commented Apr 10, 2024

ipsum-0320 commented Apr 12, 2024

levikobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiorLieberman commented Apr 14, 2024

ipsum-0320 commented Apr 14, 2024

LiorLieberman commented Apr 14, 2024

ipsum-0320 commented Apr 14, 2024

k8s-ci-robot commented Apr 15, 2024

ipsum-0320 commented Apr 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipsum-0320 commented Apr 24, 2024

k8s-ci-robot commented May 3, 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

Choose a reason for hiding this comment

mlavacca commented Jul 4, 2024

k8s-triage-robot commented Oct 2, 2024

k8s-triage-robot commented Nov 1, 2024

ipsum-0320 commented Apr 6, 2024 •

edited by LiorLieberman

Loading

linux-foundation-easycla bot commented Apr 6, 2024 •

edited

Loading