-
Notifications
You must be signed in to change notification settings - Fork 82
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
Commands can Opt-out of Logging #116
Conversation
Signed-off-by: Ken Sipe <kensipe@gmail.com>
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 have two problems with the field name:
- it is two words joined without camelCasing makint inconsistent and hard to read
- without reading the code it sounds as if this is going to not log the
running command [echo foo]
message. We should mention that it's the stderr/out output that is not going to be logged. - I feel that the word
ignore
is more suited to situations where we do not react to some event (such as withignoreFailure
above. Here we kind of spontaneously decide to not do something, so I thinkskip
is better.
@@ -114,6 +114,8 @@ type Command struct { | |||
Background bool `json:"background"` | |||
// Override the TestSuite timeout for this command (in seconds). | |||
Timeout int `json:"timeout"` | |||
// If set, the command is NOT logged. Useful to sensitive logs or to reduce noise. |
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.
// If set, the command is NOT logged. Useful to sensitive logs or to reduce noise. | |
// If set, the OUTPUT from the command is NOT logged. Useful to sensitive logs or to reduce noise. |
@@ -114,6 +114,8 @@ type Command struct { | |||
Background bool `json:"background"` | |||
// Override the TestSuite timeout for this command (in seconds). | |||
Timeout int `json:"timeout"` | |||
// If set, the command is NOT logged. Useful to sensitive logs or to reduce noise. | |||
IgnoreLog bool `json:"ignorelog"` |
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.
IgnoreLog bool `json:"ignorelog"` | |
SkipOutputLog bool `json:"skipOutputLog"` |
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.
Not sure whether this should be skipLogOutput
or skipOutputLog
...
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 ok with both namings, tbh.
I wonder if it makes sense to separate stdout and stderr though. It might be hard to debug command failures if the output is always skipped...
thanks @porridge ! I struggled with the naming a bit as well... and prefer your suggestions. thanks @ANeumann82 . If debugging is needed they can always remove the flag. Your comment gave me pause to consider |
Signed-off-by: Ken Sipe <kensipe@gmail.com>
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.
🚢
Provides the ability for a command to opt-out of logging.
This is useful when:
Co-authored-by: Marcin Owsiany mowsiany@D2iQ.com
Signed-off-by: Ken Sipe kensipe@gmail.com
Fixes #