fix: use the execWf spec to determine artifactgc strategy #10066
Conversation
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
| } | ||
| } | ||
| return results | ||
| } |
There was a problem hiding this comment.
use woc.execWf to get the strategy defined in the WorkflowTemplate if there is one
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
| spec: | ||
| artifactGC: | ||
| strategy: OnWorkflowDeletion | ||
| workflowMetadata: |
There was a problem hiding this comment.
unlike the WorkflowTemplate test that was already in there, this one includes a strategy specified on the Workflow level
| @@ -151,16 +147,16 @@ func (woc *wfOperationCtx) processArtifactGCStrategy(ctx context.Context, strate | |||
| } | |||
| templateName := node.TemplateName | |||
| if templateName == "" && node.GetTemplateRef() != nil { | |||
| templateName = node.GetTemplateRef().Name | |||
| templateName = node.GetTemplateRef().Template | |||
| } | |||
There was a problem hiding this comment.
was accidentally referring to the WorkflowTemplate's Name here rather than the name of the template within the WorkflowTemplate
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
| if template == nil { | ||
| return fmt.Errorf("can't process Artifact GC Strategy %s: template name %q belonging to node %+v not found??", strategy, node.TemplateName, node) | ||
| return fmt.Errorf("can't process Artifact GC Strategy %s: template name %q belonging to node %+v not found??", strategy, templateName, node) |
There was a problem hiding this comment.
note: I confirmed that I no longer see any more cases of woc.wf used in this file that should really be woc.execWf
| @@ -104,11 +104,7 @@ func (woc *wfOperationCtx) processArtifactGCStrategy(ctx context.Context, strate | |||
| woc.log.Debugf("processing Artifact GC Strategy %s", strategy) | |||
|
|
|||
| // Search for artifacts | |||
| artifactSearchResults := woc.wf.SearchArtifacts(&wfv1.ArtifactSearchQuery{ | |||
There was a problem hiding this comment.
Why not just change to wow.execWF.SearchArtifacts?
There was a problem hiding this comment.
Tried that at first but the execWf doesn’t seem to include the Workflow Status (for node status). Should it?
There was a problem hiding this comment.
You're correct, you must inspect woc.execWF.Spec and woc.wf.Status. The former is what to run, the latter what to update at the end of reconciliation.
You should not query across the two.
There was a problem hiding this comment.
Does this look okay then?
There was a problem hiding this comment.
@juliev0 Make sure you include the test for workflowdefautls, workflow template
There was a problem hiding this comment.
@sarabala1979 I just ran a couple of tests locally to test WorkflowDefaults. In one, I just used a Workflow by itself and verified that it took the default from WorkflowDefaults. In the second, I used a WorkflowTemplate as well. As discussed, since the workflow-controller-configmap is not being dynamically reloaded, it's difficult to test this as part of the e2e test, since otherwise I need to change test/e2e/manifests/mixins/workflow-controller-configmap.yaml, which could have consequences for other e2e tests.
Signed-off-by: Julie Vogelmani julie_vogelman@intuit.com
Fixes #10015
This fixes the case where the entire Workflow is based on a WorkflowTemplate and the ArtifactGC strategy is on the Workflow level in the WorkflowTemplate.
The issue was that when searching for artifacts we were looking at
woc.wfrather thanwoc.execWf(which includes the merged WorkflowTemplate).