Skip to content
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(controller): Fix template resolution for step groups. Fixes #1868 #1920

Merged
merged 20 commits into from
Jan 28, 2020
Merged

fix(controller): Fix template resolution for step groups. Fixes #1868 #1920

merged 20 commits into from
Jan 28, 2020

Conversation

dtaniwaki
Copy link
Member

@dtaniwaki dtaniwaki commented Jan 8, 2020

Fixes #1868 #1876

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234". Why? for the release notes.
  • Optional. I've added My organization is added to the README.
  • I've signed the CLA and required builds are green.

I have fixed the wrong use of template context for step groups and also set a template scope in every node just in case it might be useful in the future.

@codecov
Copy link

codecov bot commented Jan 8, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@9647545). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1920   +/-   ##
========================================
  Coverage          ?   8.88%           
========================================
  Files             ?      61           
  Lines             ?   34565           
  Branches          ?       0           
========================================
  Hits              ?    3071           
  Misses            ?   31106           
  Partials          ?     388

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9647545...b00f942. Read the comment docs.

@dtaniwaki
Copy link
Member Author

I also fixed an error of parallelism check caused by template scope which can be reproduced with the following manifests.

apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
  name: nested-steps-with-params
spec:
  templates:
    - name: main
      steps:
        - - name: main-sub
            template: sub
    - name: sub
      steps:
        - - name: sub-print-string
            template: print-string
            arguments:
              parameters:
               - name: letter
                 value: '{{item}}'
            withParam: '["x", "y", "z"]'
    - name: print-string
      inputs:
        parameters:
         - name: letter
      container:
        image: alpine:3.6
        command: [sh, -c]
        args: ["echo {{inputs.parameters.letter}}"]
---
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: nested-steps-with-params-
spec:
  entrypoint: main
  templates:
    - name: main
      templateRef:
        name: nested-steps-with-params
        template: main

@alexec alexec self-assigned this Jan 9, 2020
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need new tests for this?

@dtaniwaki
Copy link
Member Author

@alexec Added.

@ddseapy
Copy link
Contributor

ddseapy commented Jan 16, 2020

@alexec can this get merged before the release candidate? If so we should be able to help test the rc.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtaniwaki I'll approve this, I think you need to do git merge master?

@@ -204,7 +204,7 @@ func (d *dagContext) hasMoreRetries(node *wfv1.NodeStatus) bool {
func (woc *wfOperationCtx) executeDAG(nodeName string, tmplCtx *templateresolution.Context, templateScope string, tmpl *wfv1.Template, orgTmpl wfv1.TemplateHolder, boundaryID string) (*wfv1.NodeStatus, error) {
node := woc.getNodeByName(nodeName)
if node == nil {
node = woc.initializeExecutableNode(nodeName, wfv1.NodeTypeSteps, templateScope, tmpl, orgTmpl, boundaryID, wfv1.NodeRunning)
node = woc.initializeExecutableNode(nodeName, wfv1.NodeTypeDAG, templateScope, tmpl, orgTmpl, boundaryID, wfv1.NodeRunning)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might fix #2056

@alexec alexec merged commit d56a0e1 into argoproj:master Jan 28, 2020
@dtaniwaki dtaniwaki deleted the fix-template-resolution-in-step-group branch January 30, 2020 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WorkflowTemplate withParam causes "template not found"
4 participants