Adding functionality to delete successfully completed jobs in a FIFO … #293
Conversation
|
cc: @SimonCqk |
|
@SimonCqk would it be a good idea to strip deleted workloads from CronHistory as well. |
…fashion based on the given history limits in a cron controller Signed-off-by: Sanskar Bhushan <[email protected]>
Signed-off-by: Sanskar Bhushan <[email protected]>
sbdtu5498
force-pushed
the
add-deleting-successfully-completed-jobs-based-on-history-limits
branch
from
7e083bf
to
0c306c6
Compare
|
@SimonCqk please review. |
controllers/apps/cron_controller.go
Outdated
|
|
||
| // Register the required Kubernetes API types with the Scheme object. | ||
| corev1.AddToScheme(s) | ||
| v1alpha1.AddToScheme(s) |
There was a problem hiding this comment.
scheme created here can be a static one and being reused.
nice work! |
Signed-off-by: Sanskar Bhushan <[email protected]>
|
@sbdtu5498 LGTM in my view, anyway, can you add some unit tests for this feature? |
|
Sure thing! Also, do you think it would be a good idea to have two different historyLimit e.g. successfulHistoryLimit and failedHistoryLimit, to give more freedom to users over which jobs to delete as this function hereby considers only successful jobs while implementing historyLimit? |
sounds like a nice-to-have feature, since it introduces api changes, can you implement it in another PR and merge this commit first? |
|
Sure! |
|
@sbdtu5498 hi, it seems that UT/e2e checks failed, would you rebase upstream master branch and add more tests, then the PR can move on!! |
Ⅰ. Describe what this PR does
This PR adds functionality to delete successfully completed jobs based on the given history limits in a FIFO manner in the cron controller.
II. Does this pull request fix one issue?
fixed #244.
III. Special notes for reviewers if any.
Note that in this PR failed jobs are not considered while deleting completed jobs so that they can be manually diagnosed by the individual, as per good practice. But if required or if the reviewers consider it to be more beneficial appropriate changes can be made.