-
Notifications
You must be signed in to change notification settings - Fork 224
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
fix: Remove job deletion for jobs with TTLSecondsAfterFinished set #2375
base: main
Are you sure you want to change the base?
fix: Remove job deletion for jobs with TTLSecondsAfterFinished set #2375
Conversation
@@ -357,6 +357,10 @@ func (r *ScanJobController) completedContainers(ctx context.Context, scanJob *ba | |||
} | |||
|
|||
func (r *ScanJobController) deleteJob(ctx context.Context, job *batchv1.Job) error { | |||
if job.Spec.TTLSecondsAfterFinished != nil && *job.Spec.TTLSecondsAfterFinished != 0 { |
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 read through the docs but it wasn't clear to me why we should check for both !=nil
and !=0
. Could you explain that?
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.
Actually now that you mentioned it, it seems to me that the !=0
is superfluous / not really necessary. I would remove this from the if statement.
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.
Could you update your PR?
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.
@simar7 I'm sorry but somehow I totally forgot to do the update. I made the change and squashed the commits so that the commit history looks better.
4a7e50a
to
92f70f0
Compare
@afdesk can you take another look? |
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.
looks good!
@tom1299 thanks for your contribution
@simar7 will merge it, right? |
Description
Currently jobs are directly deleted after they are completed by the trivy-operator without honouring the jobs
ttlSecondsAfterFinished
and thus also the helm configuration parameteroperator.scanJobTTL
.The change introduced in this PR will only delete the job if it has no
ttlSecondsAfterFinished
.Related issues
Checklist