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

feat: Add default field in parameters.valueFrom #2500

Merged
merged 4 commits into from
Mar 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions api/openapi-spec/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
],
"swagger": "2.0",
"info": {
"description": "Workflow Service API performs CRUD actions against application resources",
"title": "Argo",
"version": "latest"
},
Expand Down Expand Up @@ -2722,6 +2723,10 @@
"type": "object",
"title": "ValueFrom describes a location in which to obtain the value to a parameter",
"properties": {
"default": {
"type": "string",
"title": "Default specifies a value to be used if retrieving the value from the specified source fails"
},
"jqFilter": {
"type": "string",
"title": "JQFilter expression against the resource object in resource templates"
Expand Down Expand Up @@ -5874,5 +5879,31 @@
"description": "HTTP Basic authentication",
"type": "basic"
}
},
"x-stream-definitions": {
"io.argoproj.workflow.v1alpha1.LogEntry": {
"properties": {
"error": {
"$ref": "#/definitions/grpc.gateway.runtime.StreamError"
},
"result": {
"$ref": "#/definitions/io.argoproj.workflow.v1alpha1.LogEntry"
}
},
"title": "Stream result of io.argoproj.workflow.v1alpha1.LogEntry",
"type": "object"
},
"io.argoproj.workflow.v1alpha1.WorkflowWatchEvent": {
"properties": {
"error": {
"$ref": "#/definitions/grpc.gateway.runtime.StreamError"
},
"result": {
"$ref": "#/definitions/io.argoproj.workflow.v1alpha1.WorkflowWatchEvent"
}
},
"title": "Stream result of io.argoproj.workflow.v1alpha1.WorkflowWatchEvent",
"type": "object"
}
}
}
1 change: 1 addition & 0 deletions examples/output-parameter.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ spec:
parameters:
- name: hello-param
valueFrom:
default: "Foobar" # Default value to use if retrieving valueFrom fails. If not provided workflow will fail instead
path: /tmp/hello_world.txt

- name: print-message
Expand Down
4 changes: 4 additions & 0 deletions pkg/apiclient/cronworkflow/cron-workflow.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1552,6 +1552,10 @@
"parameter": {
"type": "string",
"title": "Parameter reference to a step or dag task in which to retrieve an output parameter value from\n(e.g. '{{steps.mystep.outputs.myparam}}')"
},
"default": {
"type": "string",
"title": "Default specifies a value to be used if retrieving the value from the specified source fails"
}
},
"title": "ValueFrom describes a location in which to obtain the value to a parameter"
Expand Down
4 changes: 4 additions & 0 deletions pkg/apiclient/workflow/workflow.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1929,6 +1929,10 @@
"parameter": {
"type": "string",
"title": "Parameter reference to a step or dag task in which to retrieve an output parameter value from\n(e.g. '{{steps.mystep.outputs.myparam}}')"
},
"default": {
"type": "string",
"title": "Default specifies a value to be used if retrieving the value from the specified source fails"
}
},
"title": "ValueFrom describes a location in which to obtain the value to a parameter"
Expand Down
4 changes: 4 additions & 0 deletions pkg/apiclient/workflowarchive/workflow-archive.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1360,6 +1360,10 @@
"parameter": {
"type": "string",
"title": "Parameter reference to a step or dag task in which to retrieve an output parameter value from\n(e.g. '{{steps.mystep.outputs.myparam}}')"
},
"default": {
"type": "string",
"title": "Default specifies a value to be used if retrieving the value from the specified source fails"
}
},
"title": "ValueFrom describes a location in which to obtain the value to a parameter"
Expand Down
4 changes: 4 additions & 0 deletions pkg/apiclient/workflowtemplate/workflow-template.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1420,6 +1420,10 @@
"parameter": {
"type": "string",
"title": "Parameter reference to a step or dag task in which to retrieve an output parameter value from\n(e.g. '{{steps.mystep.outputs.myparam}}')"
},
"default": {
"type": "string",
"title": "Default specifies a value to be used if retrieving the value from the specified source fails"
}
},
"title": "ValueFrom describes a location in which to obtain the value to a parameter"
Expand Down
769 changes: 405 additions & 364 deletions pkg/apis/workflow/v1alpha1/generated.pb.go

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions pkg/apis/workflow/v1alpha1/generated.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/apis/workflow/v1alpha1/workflow_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,9 @@ type ValueFrom struct {
// Parameter reference to a step or dag task in which to retrieve an output parameter value from
// (e.g. '{{steps.mystep.outputs.myparam}}')
Parameter string `json:"parameter,omitempty" protobuf:"bytes,4,opt,name=parameter"`

// Default specifies a value to be used if retrieving the value from the specified source fails
Default string `json:"default,omitempty" protobuf:"bytes,5,opt,name=default"`
}

// Artifact indicates an artifact to place at a specified path
Expand Down
58 changes: 58 additions & 0 deletions test/e2e/functional_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,64 @@ func (s *FunctionalSuite) TestTerminateBehavior() {
})
}

func (s *FunctionalSuite) TestDefaultParameterOutputs() {
s.Given().
Workflow(`
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
name: default-params
labels:
argo-e2e: true
spec:
entrypoint: start
templates:
- name: start
steps:
- - name: generate-1
template: generate
- - name: generate-2
when: "True == False"
template: generate
outputs:
parameters:
- name: nested-out-parameter
valueFrom:
default: "Default value"
parameter: "{{steps.generate-2.outputs.parameters.out-parameter}}"

- name: generate
container:
image: docker/whalesay:latest
command: [sh, -c]
args: ["
echo 'my-output-parameter' > /tmp/my-output-parameter.txt
"]
outputs:
parameters:
- name: out-parameter
valueFrom:
path: /tmp/my-output-parameter.txt
`).
When().
SubmitWorkflow().
WaitForWorkflow(30 * time.Second).
Then().
ExpectWorkflow(func(t *testing.T, _ *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
assert.Equal(t, wfv1.NodeSucceeded, status.Phase)
assert.True(t, status.Nodes.Any(func(node wfv1.NodeStatus) bool {
if node.Outputs != nil {
for _, param := range node.Outputs.Parameters {
if param.Value != nil && *param.Value == "Default value" {
return true
}
}
}
return false
}))
})
}

func TestFunctionalSuite(t *testing.T) {
suite.Run(t, new(FunctionalSuite))
}
7 changes: 6 additions & 1 deletion workflow/controller/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -1801,7 +1801,12 @@ func getTemplateOutputsFromScope(tmpl *wfv1.Template, scope *wfScope) (*wfv1.Out
}
val, err := scope.resolveParameter(param.ValueFrom.Parameter)
if err != nil {
return nil, err
// We have a default value to use instead of returning an error
if param.ValueFrom.Default != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

test?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added an E2E test for this.

val = param.ValueFrom.Default
} else {
return nil, err
}
}
param.Value = &val
param.ValueFrom = nil
Expand Down
14 changes: 12 additions & 2 deletions workflow/executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -425,14 +425,24 @@ func (we *WorkflowExecutor) SaveParameters() error {
log.Infof("Copying %s from base image layer", param.ValueFrom.Path)
output, err = we.RuntimeExecutor.GetFileContents(mainCtrID, param.ValueFrom.Path)
if err != nil {
return err
// We have a default value to use instead of returning an error
if param.ValueFrom.Default != "" {
output = param.ValueFrom.Default
} else {
return err
}
}
} else {
log.Infof("Copying %s from from volume mount", param.ValueFrom.Path)
mountedPath := filepath.Join(common.ExecutorMainFilesystemDir, param.ValueFrom.Path)
out, err := ioutil.ReadFile(mountedPath)
if err != nil {
return err
// We have a default value to use instead of returning an error
if param.ValueFrom.Default != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

test?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't include tests for this because it would be difficult to mock this behavior and I have already included a non-E2E test that covers the same behavior here: https://github.com/argoproj/argo/pull/2500/files#diff-4ed679a69dc8fdd934fb1e7f7c0f3259R109-R139

If you think it's necessary, I can invest the time into mocking this or creating an e2e test

output = param.ValueFrom.Default
} else {
return err
}
}
output = string(out)
}
Expand Down
33 changes: 33 additions & 0 deletions workflow/executor/executor_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package executor

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -104,3 +105,35 @@ func TestIsBaseImagePath(t *testing.T) {
assert.False(t, we.isBaseImagePath("/user-mount/some-path/foo"))
assert.True(t, we.isBaseImagePath("/user-mount-coincidence"))
}

func TestDefaultParameters(t *testing.T) {
fakeClientset := fake.NewSimpleClientset()
mockRuntimeExecutor := mocks.ContainerRuntimeExecutor{}
templateWithOutParam := wfv1.Template{
Outputs: wfv1.Outputs{
Parameters: []wfv1.Parameter{
{
Name: "my-out",
ValueFrom: &wfv1.ValueFrom{
Default: "Default Value",
Path: "/path",
},
},
},
},
}
we := WorkflowExecutor{
PodName: fakePodName,
Template: templateWithOutParam,
ClientSet: fakeClientset,
Namespace: fakeNamespace,
PodAnnotationsPath: fakeAnnotations,
ExecutionControl: nil,
RuntimeExecutor: &mockRuntimeExecutor,
mainContainerID: fakeContainerID,
}
mockRuntimeExecutor.On("GetFileContents", fakeContainerID, "/path").Return("", fmt.Errorf("file not found"))
err := we.SaveParameters()
assert.NoError(t, err)
assert.Equal(t, "Default Value", *we.Template.Outputs.Parameters[0].Value)
}
11 changes: 8 additions & 3 deletions workflow/executor/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,15 @@ func (we *WorkflowExecutor) SaveResourceParameters(resourceNamespace string, res
log.Info(cmd.Args)
out, err := cmd.Output()
if err != nil {
if exErr, ok := err.(*exec.ExitError); ok {
log.Errorf("`%s` stderr:\n%s", cmd.Args, string(exErr.Stderr))
// We have a default value to use instead of returning an error
if param.ValueFrom.Default != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

test?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't include tests for this because it would be difficult to mock this behavior and I have already included a non-E2E test that covers the same behavior here: https://github.com/argoproj/argo/pull/2500/files#diff-4ed679a69dc8fdd934fb1e7f7c0f3259R109-R139

If you think it's necessary, I can invest the time into mocking this or creating an e2e test

out = []byte(param.ValueFrom.Default)
} else {
if exErr, ok := err.(*exec.ExitError); ok {
log.Errorf("`%s` stderr:\n%s", cmd.Args, string(exErr.Stderr))
}
return errors.InternalWrapError(err)
}
return errors.InternalWrapError(err)
}
output := string(out)
we.Template.Outputs.Parameters[i].Value = &output
Expand Down