Functionality to delete logs. #3545
Conversation
artifacts to be deleted.
daniel-beck
added
the
needs-more-reviews
|
I'm not sure about this one, but cannot put my finger on exactly why. The implementation looks sound. Is removal of logs, frequently considered essential, something we should encourage by options in the out of the box UI? Or should we rather encourage use of e.g. Regarding this being a potential breaking change: Are we confident nothing breaks if there's no log file? Would it perhaps be better to replace the file contents with a message that said that (and perhaps when) the log was removed? |
|
@jenkinsci/code-reviewers I think more feedback here would be useful. |
| File rootDir = getRootDir(); | ||
| LOGGER.log(FINE, "{0}: {1} logs successfully deleted", new Object[] { this, rootDir }); | ||
| synchronized (this) { | ||
| Util.deleteFile(getLogFile()); |
There was a problem hiding this comment.
It makes an assumption that the logs are stored on the disk.
Some plugins like Logstash plugin do not actually do that, especially if there is External Logging implementation like jenkinsci/logstash-plugin#18 in place
Not a big problem for this PR as long as the method is overridable.
But it would great to ensure it does not blow up if the log file does not exist
|
|
||
| @DataBoundConstructor | ||
| public LogRotator (String daysToKeepStr, String numToKeepStr, String artifactDaysToKeepStr, String artifactNumToKeepStr) { | ||
| public LogRotator (String daysToKeepStr, String numToKeepStr, String artifactDaysToKeepStr, String artifactNumToKeepStr, String logDaysToKeepStr, String logNumToKeepStr) { |
There was a problem hiding this comment.
🐛 this method is a part of public API, so this change breaks binary compatibility. The original method should be retained OR you could use @DataBoundSetters
|
@ThyRex I like the idea, but we need to ensure it is binary compatible. Making sure that it does not blow up on missing file is also important IMHO |
oleg-nenashev
added
the
needs-comments-resolved
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
I would recommend closing this PR and just creating a plugin with this option, as
- I do not see the functionality as being broadly desirable: there is little value in a build record with no log. (Whereas it is perfectly normal to not care about artifacts from old builds.) There seems to be no compelling reason to make Jenkins core more complicated for this, when an extension point already exists for fine-tuning build cleanup behavior.
- There is already work underway to switch log storage to an external (cloud) service, which would undercut the principal motivation for deleting log files to begin with: saving disk space on
$JENKINS_HOME.
4 participants
See JENKINS-51229.
Proposed changelog entries
Submitter checklist
* Use the
Internal:prefix if the change has no user-visible impact (API, test frameworks, etc.)Desired reviewers
@oleg-nenashev