Skip to content
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

Add : Support outputting the result of 'verify' in YAML format and only outputting the summary of the result of 'verify' #115

Merged
merged 9 commits into from
Aug 23, 2023

Conversation

chunriyeqiongsaigao
Copy link
Contributor

@chunriyeqiongsaigao chunriyeqiongsaigao commented Aug 17, 2023

  1. Added a new command-line parameter -o, which stands for --output. The value of this parameter represents the format in which the summary of the verify results should be outputted. Currently, only YAML format is supported, the value of which is yaml. If the command e2e verify -c e2e.yaml -o yaml is entered, only the summary in YAML format will be outputted to the CL.

  2. Added a new command-line parameter --summary-only. If the command e2e verify -c e2e.yaml --summary-only is entered, only the SUMMARY part of verify results will be outputted to the CL.

example:

This is the cases section of the e2e.yaml config file for verify. It provides 4 cases for e2e to veriy. And case-1, case-2, and case-4 will pass, while case-3 will fail.

2023-08-17 10-29-01 的屏幕截图

  1. A command without -o yaml : ./bin/linux/e2e verify -c ../e2e/e2e.yaml

2023-08-20 15-18-53 的屏幕截图

  1. A command with -o yaml : ./bin/linux/e2e verify -c ../e2e/e2e.yaml -o yaml

2023-08-20 15-29-36 的屏幕截图

  1. A command without --summary-only : ./bin/linux/e2e verify -c ../e2e/e2e.yaml

2023-08-20 15-18-53 的屏幕截图

  1. A command with --summary-only : ./bin/linux/e2e verify -c ../e2e/e2e.yaml --summary-only

2023-08-20 17-20-57 的屏幕截图

@wu-sheng wu-sheng added this to the 1.3.0 milestone Aug 17, 2023
@wu-sheng
Copy link
Member

What does y actually stand for?

@chunriyeqiongsaigao
Copy link
Contributor Author

chunriyeqiongsaigao commented Aug 17, 2023

What does y actually stand for?

In my opinion, the y stands for yaml.
Perhaps the naming of this parameter is not appropriate, and I expect my mentor to provide me with guidance and feedback on it later.

@fgksgf
Copy link
Member

fgksgf commented Aug 17, 2023

@chunriyeqiongsaigao
I think --output and -o would be better, just like kubectl. And we may need to support more output format in the future. In this case, we can specify -o yaml to output the result in YAML.

@wu-sheng
Copy link
Member

I am a little confused. From what I read from your demo, the -y seems to disable the diff, is that relative to the YAML? Somehow?

@fgksgf
Copy link
Member

fgksgf commented Aug 17, 2023

I am a little confused. From what I read from your demo, the -y seems to disable the diff, is that relative to the YAML? Somehow?

We need this PR to make e2e (the dev version) output the verify result in yaml format, so that another e2e (the stable version) can use the yaml output to verify if the dev version e2e works well, just like we discussed in this

@wu-sheng
Copy link
Member

OK. This seems more like summary in YAML because actually you disabled many other things.

@kezhenxu94 Are you aware of the naming thing? Any suggestion?

@fgksgf
Copy link
Member

fgksgf commented Aug 17, 2023

OK. This seems more like summary in YAML because actually you disabled many other things.

Yes

@chunriyeqiongsaigao
Copy link
Contributor Author

@chunriyeqiongsaigao I think --output and -o would be better, just like kubectl. And we may need to support more output format in the future. In this case, we can specify -o yaml to output the result in YAML.

OK, I will edit the code later. Thanks!

@wu-sheng
Copy link
Member

If we consider more formats, we should consider this in two parts.

  1. Whether output the verify diff or summary.
  2. What is the format of the final result.

@kezhenxu94
Copy link
Member

OK. This seems more like summary in YAML because actually you disabled many other things.

@kezhenxu94 Are you aware of the naming thing? Any suggestion?

I'm not quite aware of the naming thing. @chunriyeqiongsaigao Can you ping your mentor here.

From my understanding, if this is about the output format, then the format name might not be a good command option -y, instead you might want to add an option to set the output format such as -o, and make yaml one of the possible value of that option. -o yaml

@wu-sheng
Copy link
Member

-o/--output yaml seems good, meanwhile, could we consider --output-detail Y/N(or a better name) to disable the raw diff separately?

@chunriyeqiongsaigao
Copy link
Contributor Author

I'm not quite aware of the naming thing. @chunriyeqiongsaigao Can you ping your mentor here.
OK!My mentor @fgksgf 👍👍👍

@fgksgf
Copy link
Member

fgksgf commented Aug 17, 2023

-o/--output yaml seems good, meanwhile, could we consider --output-detail Y/N(or a better name) to disable the raw diff separately?

Make sense, I will discuss with the student.

@wu-sheng
Copy link
Member

Please update PR title and descriptions to fit your latest codes.

@chunriyeqiongsaigao
Copy link
Contributor Author

Please update PR title and descriptions to fit your latest codes.

Sure, I will update the PR title and descriptions to align with the latest changes in the code. Thank you for the reminder.

@chunriyeqiongsaigao chunriyeqiongsaigao changed the title Add : Support outputting the result of 'verify' in YAML format Add : Support outputting the result of 'verify' in YAML format and outputting the result of 'verify' without 'details'(diff) Aug 20, 2023
@chunriyeqiongsaigao chunriyeqiongsaigao changed the title Add : Support outputting the result of 'verify' in YAML format and outputting the result of 'verify' without 'details'(diff) Add : Support outputting the result of 'verify' in YAML format and only outputting the summary of the result of 'verify' Aug 20, 2023
@wu-sheng
Copy link
Member

I think new command style is good. I want to confirm, is the real tests for this tool will come next?

Copy link
Member

@fgksgf fgksgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically correct 👍, but need some polish

commands/verify/verify.go Outdated Show resolved Hide resolved
commands/verify/verify.go Outdated Show resolved Hide resolved
internal/config/globalConfig.go Outdated Show resolved Hide resolved
pkg/output/output.go Outdated Show resolved Hide resolved
pkg/output/output.go Outdated Show resolved Hide resolved
pkg/output/printer.go Outdated Show resolved Hide resolved
pkg/output/printer.go Outdated Show resolved Hide resolved
@fgksgf
Copy link
Member

fgksgf commented Aug 20, 2023

I think new command style is good. I want to confirm, is the real tests for this tool will come next?

Yes, the student has made a POC.

Copy link
Member

@fgksgf fgksgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits

commands/verify/verify.go Outdated Show resolved Hide resolved
commands/verify/verify.go Outdated Show resolved Hide resolved
internal/config/globalConfig.go Outdated Show resolved Hide resolved
pkg/output/output.go Outdated Show resolved Hide resolved
pkg/output/output.go Outdated Show resolved Hide resolved
pkg/output/output.go Outdated Show resolved Hide resolved
pkg/output/output.go Outdated Show resolved Hide resolved
pkg/output/printer.go Outdated Show resolved Hide resolved
pkg/output/printer.go Show resolved Hide resolved
pkg/output/printer.go Outdated Show resolved Hide resolved
Copy link
Member

@fgksgf fgksgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, well done!

@fgksgf fgksgf merged commit 0f31b43 into apache:main Aug 23, 2023
1 check passed
@wu-sheng
Copy link
Member

@fgksgf After further tests are added, are we planning a new release?

It has been 24 commits included and over a year since the last release. v1.2.0...main

@fgksgf
Copy link
Member

fgksgf commented Aug 24, 2023

@fgksgf After further tests are added, are we planning a new release?

It has been 24 commits included and over a year since the last release. v1.2.0...main

@wu-sheng No problem, i will do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants