-
Notifications
You must be signed in to change notification settings - Fork 18
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
Adds report file output and different stdout outputs #71
Conversation
# Conflicts: # reporting/report.go
# Conflicts: # cmd/main.go
# Conflicts: # cmd/main.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd/main.go
Outdated
@@ -76,6 +79,8 @@ func Execute() { | |||
cobra.OnInitialize(initLog) | |||
rootCmd.PersistentFlags().StringSlice(tagsFlagName, []string{"all"}, "select rules to be applied") | |||
rootCmd.PersistentFlags().String(logLevelFlagName, "info", "log level (trace, debug, info, warn, error, fatal)") | |||
rootCmd.PersistentFlags().StringSlice(reportPath, []string{""}, "path to generate report file. Available formats are: json, yaml and sarif") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rootCmd.PersistentFlags().StringSlice(reportPath, []string{""}, "path to generate report file. Available formats are: json, yaml and sarif") | |
rootCmd.PersistentFlags().StringSlice(reportPath, []string{""}, "path to generate report files. The output format will be determined by the file extension (.json, .yaml, .sarif)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/spf13/cobra/blob/main/shell_completions.md#completions-for-flags
rootCmd.PersistentFlags().StringSlice(reportPath, []string{""}, "path to generate report file. Available formats are: json, yaml and sarif") | |
rootCmd.PersistentFlags().StringSlice(reportPath, []string{""}, "path to generate report file. Available formats are: json, yaml and sarif") | |
rootCmd.MarkPersistentFlagFilename(reportPath, "json", "yaml", "sarif") | |
rootCmd.PersistentFlags().String(stdoutFormat, "yaml", "stdout output format, available formats are: json, yaml and sarif") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none of them should be mandatory, isn't that what we agreed? Report File is only if the user wants a report and the stdoutFormat has the default value set to yaml so it will print yaml by default like we talked about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you say is it will always print to the stdout
(because you can't omit it, if so, it will be yaml
as default)?
OK then, let's keep it like that.
But please consider my first suggestion, I think we need to explain that the format will be decided by the file extension,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you say is it will always print to the
stdout
(because you can't omit it, if so, it will beyaml
as default)?
exactly this
But please consider my first suggestion, I think we need to explain that the format will be decided by the file extension
I did consider it and changed it, but in this view it always stays as it was it doesn't show the updates.
Please check here that it is what you suggested:
https://github.com/Checkmarx/2ms/pull/71/files#diff-c444f711e9191b53952edb65bfd8c644419fc7695c62611dc0fb304b4fb197d6
config/config.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we need this file. Do you expect it to grow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to pass the version, so instead of doing it by argument I did it like this as I do expect it to grow in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call it "Properties" or another term from SARIF?
Because I don't think it is used for configuration details (such as environment variables and feature flags).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, please re-review the unresolved comments.
Approved.
No description provided.