-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
JENKINS-68822 added support for removing all builds with LogRotator #9405
Conversation
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
Run lsb = removeLastSuccessfulBuild ? null : job.getLastSuccessfulBuild(); | ||
Run lstb = removeLastStableBuild ? null : job.getLastStableBuild(); |
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.
hmm I think this logic may not be 100% correct.
I have selected "Remove last stable build" - Yes
and then I have a stable build (success) and 2 failures.
My stable build never gets removed because I have set Remove last successful build - no.
The UI seems to be a bit too tied to the implementation.
At a minimum I think if remove last stable build is selected as yes and the last successful build is a stable build then it should be removed.
I wonder if we need both UI options, would it be enough to just add the stable option?
Or do you have any ideas on how it could be simpler?
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.
Indeed, having a single UI setting is simpler.
@@ -41,5 +41,9 @@ THE SOFTWARE. | |||
description="${%if not empty, only up to this number of builds have their artifacts retained}" field="artifactNumToKeepStr"> | |||
<f:number clazz="positive-number" min="1" step="1" /> | |||
</f:entry> | |||
<f:entry title="${%Remove last stable and successful builds}" |
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.
what about these? The label seems too tied to the developer writing code instead of from a user perspective imo.
<f:entry title="${%Remove last stable and successful builds}" | |
<f:entry title="${%Remove last build}" |
or
<f:entry title="${%Remove last stable and successful builds}" | |
<f:entry title="${%Remove last successful build}" |
successful is still a bit confusing as 'unstable' is successful from my reading of the code.
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.
Picked the shorter option, more clear to me as developer :)
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.
Thanks!
Weak -0.5 as this does not appear to be a common use case and is better off being done in a plugin. |
/label ready-for-merge This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback. |
Congratulations on getting your very first Jenkins core pull request merged 🎉🥳 |
See JENKINS-68822.
Testing done
Verified configuration in UI
Checked that old configuration is loaded from disk
Run the unit tests
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Before the changes are marked as
ready-for-merge
:Maintainer checklist