feat: add succeeded to retentionPolicy Fixes #12135#14475
feat: add succeeded to retentionPolicy Fixes #12135#14475antbbn wants to merge 2 commits intoargoproj:mainfrom
Conversation
antbbn
requested review from
Joibel,
sarabala1979 and
terrytangyuan
as code owners
|
Not sure about these test failures, |
|
/retest |
1 similar comment
|
/retest |
|
@Joibel I saw you added some comments to retention_policy.go is this PR still relevant? |
Yes, the PR is still relevant. The comments were added as part of generating full docs for the workflow-controller-configmap. I'm sorry no-one has reviewed it yet. I'm busy on other things at the moment. |
No worries, I totally understand, i'll fix the conflicts in the meanwhile :) |
Signed-off-by: Antonio Bibiano <antbbn@gmail.com>
Signed-off-by: Antonio Bibiano <antbbn@gmail.com>
| switch phase { | ||
| case wfv1.WorkflowSucceeded: | ||
| maxWorkflows = c.retentionPolicy.Completed | ||
| maxWorkflows = c.retentionPolicy.Succeeded |
There was a problem hiding this comment.
Although I applaud the idea of maintaining backwards compatibility I think I'd rather this was a breaking change and we just document it as such:
- It's a feature so only going out in a minor or major release
- It's trivial to update your configuration to make it work the new way.
To that end think there should be a little more documentation in this PR in the docs/upgrading.md, but drop this compatibility block.
If we keep this we need to document this unexpected behaviour, and live with it "forever"
| return x | ||
| } | ||
|
|
||
| func (h *gcHeap) PeekPopTimestamp() (time.Time, error) { |
There was a problem hiding this comment.
PeekHeadTimestamp? This function doesn't appear to actually Pop which I'd assumed from it's name
3 participants
Retention of Completed workflows to delete from the other three phases
adds secondsAfterError to TTLStrategy to align with retentionPolicy
Fixes #12135
Motivation
Previous naming was not aligned with the behaviour.
Modifications
Separated retention policy check for completed workflows.
Added secondsAfterError to TTLStrategy because the Error phase was not considered in the TTL but was considered in the retention and I found this a bit confusing.
Verification
Tested in DevContainer by submittion various workflows that ended in all the phases involved in the retention.
Documentation
Added a more detailed explanation of retention to the workflow controller configmap yaml.