Change docker.verbose parameter to String to handle multiple logging levels #1192
Conversation
There was a problem hiding this comment.
Thanks ! Looks good, but I have some suggestions. See below, especially wrt/ backwards compatibility (you should still allow 'true' and 'false' as valus) + a generalization to make it easier to add more LogVerboseCategory element.
Also, don't forget to add it to the documentation.
| @@ -78,7 +78,13 @@ public void info(String message, Object ... params) { | |||
|
|
|||
| /** {@inheritDoc} */ | |||
| public void verbose(String message, Object ... params) { | |||
| if (verbose) { | |||
| if (verbose != null && verbose.contains("build")) { | |||
There was a problem hiding this comment.
I would enable this also when this parameter is set to true, so that the old behaviour with -Ddocker.verbose=true still works (and which would switch on just the 'build' verbose part).
| @@ -106,7 +112,7 @@ public boolean isDebugEnabled() { | |||
| } | |||
|
|
|||
| public boolean isVerboseEnabled() { | |||
| return verbose; | |||
| return !verbose.isEmpty(); | |||
There was a problem hiding this comment.
I think if you use -Ddocker.verbose=false then verbose logging should be disabled as well. Please add a check for "false", too.
| * @param format verbose message format | ||
| * @param params parameter for formatting message | ||
| */ | ||
| void verboseApi(String format, Object ... params); |
There was a problem hiding this comment.
Could we generalise this ? I.e. to add an an enum LogVerboseCategory with elements "Build" and "Api", and a verbose(LogCategory cat, String format, .... params) method ?
That would make it much easier to add more than the Api for logging.
| @@ -28,7 +28,7 @@ | |||
| private final String prefix; | |||
| private final boolean batchMode; | |||
|
|
|||
| private boolean verbose; | |||
| private String verbose = "build"; | |||
There was a problem hiding this comment.
better call it "verboseModes" and maybe make it an array of LogVerboseCategory enums ? see below.
…PUT/DELETE to docker Signed-off-by: Kenny MacLeod <kmacleod@atlassian.com>
rohanKanojia
added
waiting on feedback
and removed
changes requested
documentation missing
labels
| * @param format verbose message format | ||
| * @param params parameter for formatting message | ||
| */ | ||
| void verbose(String format, Object ... params); | ||
| void verbose(AnsiLogger.LogVerboseCategory logVerboseCategory, String format, Object ... params); |
There was a problem hiding this comment.
We should move the category to this interface. It's a bad smell to have the interface to reference to one of its implementations.
| this.log = log; | ||
| this.verbose = verbose; | ||
| this.isVerbose = checkBackwardVersionValues(verbose); // For backward compatibility |
There was a problem hiding this comment.
What happens when "verbose" is null ? It doesn't look like that the code really checks this case. It should be that in this case verbose logging should be disabled.
+ added documentations + added tests + added group "all"
Fix #917