@afdesk can you take another look?

@simar7 will merge it, right?

@tom1299 is it possible to write a test for this case? While it is a trivial change, it is an important one. Maybe we can use this test setup?

@simar7 Adding a test case like above is a very good idea. I will have a deeper look into the env tests and try to add a test case for the ttl jobs.

@simar7 So I had a look at the env tests. And the main issue for adding a test case to the env tests is that the ScanJobController can not be easily added to the env test setup here. Since the ScanJobController contains the method to test, it will never be executed.
I also think that even if it would be possible to add the controller, the JobHasAnyCondition Predicate would prevent the Controller from processing the job since the Jobs created in the env tests do not seem to have a status.
Here is the predicate code:

var JobHasAnyCondition = predicate.NewPredicateFuncs(func(obj client.Object) bool {
	if job, ok := obj.(*batchv1.Job); ok {
		return len(job.Status.Conditions) > 0
	}
	return false
})

And it is registered with ScanJobController here:

func (r *ScanJobController) SetupWithManager(mgr ctrl.Manager) error {
	var predicates []predicate.Predicate
	if !r.ConfigData.VulnerabilityScanJobsInSameNamespace() {
		predicates = append(predicates, InNamespace(r.Config.Namespace))
	}
	predicates = append(predicates, ManagedByTrivyOperator, IsVulnerabilityReportScan, JobHasAnyCondition)
	return ctrl.NewControllerManagedBy(mgr).
		For(&batchv1.Job{}, builder.WithPredicates(predicates...)).
		Complete(r.reconcileJobs())
}

By the way, there seems to be a redundant implementation in the ScanJobController itself here:

if len(job.Status.Conditions) == 0 {
	log.V(1).Info("Ignoring Job without conditions")
	return ctrl.Result{}, nil
}

Which means that the when the Predicate would be removed, the ScanJobController would still not process the job. For me this might also be an issue that needs to be fixed.

So I think it would be rather hard to test the changes in the env tests. Or maybe you could give me another hint how to add a test ? Otherwise I can suggest looking into the integration tests. Maybe we could add a test case here ?

As an alternative I added a integration test here 932e1a8. the tests validates that if ttl is set for scan jobs, they are not deleted immediately. What do you think about that ? Would that be a way to to the testing ?