-
Notifications
You must be signed in to change notification settings - Fork 86
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
Allow user to set reportName #395
Conversation
* change default report name to `kuttl-report` * override default report name according to config file's `reportName` field * override default or config file report name according to command line option --report-name Addresses issue kudobuilder#394 Signed-off-by: Ben Blattberg <ben.blattberg@crunchydata.com>
@@ -58,6 +58,7 @@ func newTestCmd() *cobra.Command { | |||
mockControllerFile := "" | |||
timeout := 30 | |||
reportFormat := "" | |||
reportName := "kuttl-report" |
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.
These vars are set here, but it seems like the main value of setting the default value here is as a reminder of what the default is, since they get overwritten by the config file, yeah?
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.
it is overridden by the config file, however it isn't required to be in the config file. Even when defined in the config file, it is possible to override with a flag --report-name
in this case. The value of setting it here is to provide an undefined default. While there are a few ways of it being set, default, flag and config file... in the end what is established in the options.ReportName
is what matters
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.
after some refactoring, I understand better your question. IMO we should refactor the init to the be the order defined and not "check" the value throughout code.
// GetReportName returns the configured ReportName. | ||
func (h *Harness) GetReportName() string { | ||
reportName := "kuttl-report" | ||
if h.TestSuite.ReportName != "" { | ||
reportName = h.TestSuite.ReportName | ||
} | ||
return reportName | ||
} | ||
|
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.
Since this pattern was already established by the Timeout field, I followed that method of setting the default where it would be used.
thanks @benjaminjb ! I will take a closer review tomorrow and we may need to coordinate with our operator sdk scorecard folks... lets hold until we get their feedback... but that shouldn't take long. |
@benjaminjb @kensipe this change will definitely break We'll probably want to flag this as a 'breaking' change when |
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.
/lgtm
/lgtm |
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.
/lgtm and we double checked with operatorSDK team. there is 1 minor change I would like to see but it doesn't change the functionality. I plan on merging and create a small PR update.
thanks for the contribution and patience!
@@ -58,6 +58,7 @@ func newTestCmd() *cobra.Command { | |||
mockControllerFile := "" | |||
timeout := 30 | |||
reportFormat := "" | |||
reportName := "kuttl-report" |
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.
it is overridden by the config file, however it isn't required to be in the config file. Even when defined in the config file, it is possible to override with a flag --report-name
in this case. The value of setting it here is to provide an undefined default. While there are a few ways of it being set, default, flag and config file... in the end what is established in the options.ReportName
is what matters
@@ -250,6 +254,7 @@ For more detailed documentation, visit: https://kuttl.dev`, | |||
testCmd.Flags().IntVar(¶llel, "parallel", 8, "The maximum number of tests to run at once.") | |||
testCmd.Flags().IntVar(&timeout, "timeout", 30, "The timeout to use as default for TestSuite configuration.") | |||
testCmd.Flags().StringVar(&reportFormat, "report", "", "Specify JSON|XML for report. Report location determined by --artifacts-dir.") | |||
testCmd.Flags().StringVar(&reportName, "report-name", "kuttl-report", "Name for the report. Report location determined by --artifacts-dir and report file type determined by --report.") |
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.
now I'm wondering if we should rename --report
, as we add more report configuration options it is not as easy to understand and likely should be --report-type
. I also initially questioned the verbosity of this flag... however it provides good understanding without the user needing to understand the code. nice work... no changes
h.fatal(fmt.Errorf("fatal error writing report: %v", err)) | ||
} | ||
} | ||
|
||
// GetReportName returns the configured ReportName. | ||
func (h *Harness) GetReportName() string { |
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.
func (h *Harness) GetReportName() string { | |
func (h *Harness) reportName() string { |
I've taken too long and if this is the only change, I will merge and create a PR to mod it myself. First, prefer not to use "GetXYZ", this can simply be reportName
. Second, there is no need to make public, it's only use is in type. We can always expose later if desired.
* change default report name to `kuttl-report` * override default report name according to config file's `reportName` field * override default or config file report name according to command line option --report-name Addresses issue kudobuilder#394 Signed-off-by: Ben Blattberg <ben.blattberg@crunchydata.com> Signed-off-by: Israel Blancas <iblancasa@gmail.com>
What this PR does / why we need it:
kuttl-report
-- I can nix this part if you don't want it, but it seems to me thatkuttl-test.json
/kuttl-test.xml
is too close tokuttl-test.yaml
, the default config name, and liable to lead to some confusion.reportName
fieldSigned-off-by: Ben Blattberg ben.blattberg@crunchydata.com
Fixes #394
Fixes #284