-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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(executor): Optional input artifacts. Fixes #2990 #3019
Conversation
/lgtm |
@@ -164,6 +164,10 @@ func (we *WorkflowExecutor) LoadArtifacts() error { | |||
tempArtPath := artPath + ".tmp" | |||
err = artDriver.Load(&art, tempArtPath) | |||
if err != nil { | |||
if art.Optional && errors.IsCode(errors.CodeNotFound, err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently Artifact.Optional
means "not specified". With this we also enhance it to mean "not found" as well. Is this what we want or should we distinguish between the two?
Some users might expect different behaviors with each of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets discus at stand-up tomorrow, I'm having trouble getting my head around how "optional" could not be interpreted as "optional" - where as something specified in the spec can be inferred to be "not specified". It's like words have new meanings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Make Artifacts optional, if Artifacts doesn't generate or exist
Optional bool `json:"optional,omitempty" protobuf:"varint,8,opt,name=optional"`
Hmmm....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought I agree that the two meanings are essentially the same.
"Optional" should be taken from the point of view of the container that receives the artifact. The reason as to why it's not there should not matter.
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@@ -169,7 +169,14 @@ func (driver *ArtifactDriver) Load(inputArtifact *wfv1.Artifact, path string) er | |||
} | |||
} | |||
|
|||
return hdfscli.CopyToLocal(driver.Path, path) | |||
err = hdfscli.CopyToLocal(driver.Path, path) | |||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.Changes
Testing
ErrObjectNotExists
Prototypes
https://bit.ly/argo-wf-protos