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

Conversation

simster7
Copy link
Member

Closes #2494 and #2495.

Adds field default in parameters.valueFrom that specifies a default value to be used when retrieving the otherwise specified value fails.

(Thanks @samath117!)

@simster7 simster7 requested review from alexec and sarabala1979 March 23, 2020 16:07
@simster7 simster7 linked an issue Mar 23, 2020 that may be closed by this pull request
@@ -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.

}
} 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

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

@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@33cd4f2). Click here to learn what that means.
The diff coverage is 4%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2500   +/-   ##
=========================================
  Coverage          ?   11.36%           
=========================================
  Files             ?       74           
  Lines             ?    31158           
  Branches          ?        0           
=========================================
  Hits              ?     3540           
  Misses            ?    27143           
  Partials          ?      475
Impacted Files Coverage Δ
pkg/apis/workflow/v1alpha1/workflow_types.go 11.54% <ø> (ø)
workflow/executor/resource.go 0% <0%> (ø)
pkg/apis/workflow/v1alpha1/generated.pb.go 0.45% <0%> (ø)
workflow/controller/operator.go 60.8% <0%> (ø)
workflow/executor/executor.go 4.5% <25%> (ø)

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 33cd4f2...2461cfb. Read the comment docs.

})
}


Choose a reason for hiding this comment

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

File is not goimports-ed with -local github.com/argoproj/argo (from goimports)

Suggested change

simster7 and others added 2 commits March 23, 2020 14:41
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
@simster7 simster7 merged commit 09291d9 into argoproj:master Mar 23, 2020
@danxmoran
Copy link
Contributor

Will this be / could this be ported to the 2.7 release branch?

@simster7
Copy link
Member Author

@danxmoran Yup, that's the plan!

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.

Extend default output parameters to steps/dag templates Default output parameters don't work (anymore?)
4 participants