Add job setting to suppress console logging #94
Conversation
This plugin is really noisy when you have loads of assets to archive, so this adds a toggle to suppress all output to the build console.
| @@ -119,7 +127,9 @@ public static S3Profile getProfile(String profileName) { | |||
| } | |||
|
|
|||
| private void log(final PrintStream logger, final String message) { | |||
| logger.println(StringUtils.defaultString(getDescriptor().getDisplayName()) + ' ' + message); | |||
| if(! suppressLogging) { | |||
There was a problem hiding this comment.
Sorry, but I don't think that this is a right place for suppressing. Probably, you want to suppress only messages that's related to pushing files to s3 - not all messages (including warning and errors).
So, I can see two solutions:
- Move this check to line 185/195
- Introduce three different levels: errors (after such
logyou can seereturnstatement orsetResult(failed)), warning ('no files found', 'skip publishing', etc.) and info (this one you want to skip).
There was a problem hiding this comment.
I've addressed this in my latest commit. Let me know if you like the implementation.
|
Thanks for PR, please, check review |
Allow users to control the noisiness of the S3 plugin console output by using the `java.util.logging.Level` field definitions.
| @@ -66,6 +68,7 @@ public S3BucketPublisher(String profileName, List<Entry> entries, List<MetadataP | |||
| this.userMetadata = userMetadata; | |||
|
|
|||
| this.dontWaitForConcurrentBuildCompletion = dontWaitForConcurrentBuildCompletion; | |||
| this.consoleLogLevel = Level.parse(consoleLogLevel); | |||
There was a problem hiding this comment.
Unfortunately, parse is executing under synchronized. In highly loaded environment it's not very efficient (we run ~1.5k jobs in parallel). I will think how to do it better.
There was a problem hiding this comment.
@Jimilian Please check my latest commit to see if that is a more acceptable solution.
There was a problem hiding this comment.
Yes, I was thinking about the same. Thanks you!
|
@coltrey I'm not familiary with how pipeline scripts work, but this is concerning. Do you have a reference you could link? I'm guessing this would be a significant disruption for plugin users, right? |
|
@coltrey, @dobbymoodge I'm not 100% sure as well, but IMHO everything should be ok. In case of step Unfortunately, I don't have time for testing that right now. |
|
@dobbymoodge I'm a lousy developer, but here's a an example and a stack trace, and I realize this comment might be on the wrong commit thread, but I think when the constructor was changed, it broke compatibility. Downgrading to 0.10.9 from 0.10.11 solved the issue (also I have more recently confirmed that 0.10.10 was also working). When I was troubleshooting, I pulled up the syntax generator and it looked like some things may've changed...: This is the 0.10.11 syntax generator example, the generated values for consoleLogLEvel and pluginFailureResultConstraint are one of the things that made me question the constructor change: This works in 0.10.9, but not 0.10.11: This is the stack trace we get: |
|
@dobbymoodge @Jimilian |
|
@coltrey This looks legit. Can you file an issue so I can reference it in the fix PR? |
|
@dobbymoodge JENKINS-40786 |
|
@dobbymoodge So, it's better to introduce several not null checks... |
|
What about compatibility with the snippet generator? The 0.10.11 release will generate invalid snippets (ex: |
This plugin is really noisy when you have loads of assets to archive,
so this adds a toggle to suppress all output to the build console.