-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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(server): Download artifacts from UI. Fixes #4338 #4350
Conversation
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@@ -137,8 +137,7 @@ func (a *ArtifactServer) getArtifact(ctx context.Context, wf *wfv1.Workflow, nod | |||
if err != nil { | |||
return nil, err | |||
} | |||
|
|||
tmp, err := ioutil.TempFile(".", "artifact") | |||
tmp, err := ioutil.TempFile("/tmp", "artifact") |
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.
Will it be automatically deleted? Or do we need to delete it?
https://golang.org/pkg/io/ioutil/#TempFile
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.
already done - see below:
defer func() { _ = os.Remove(path) }()
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.
this change broke the output artifact downloads for us, we upgraded from 2.11.5 to 2.11.7, and we began to get errors like this:
open /tmp/artifact058606664: no such file or directory
reverting the upgrade fixed the downloads.
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.
@ekesken - likely using runAsNonRoot? You'll need to mount /tmp as an emptyDir volume on argo-server pod so you can write to it. This path should probably be configurable as an argument.
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.
I see that the fix in helm chart is already merged here: argoproj/argo-helm@9939da5
It was surprising to see a non-backward compatible change in a patch version change, that's why I wanted to point it here.
Back-ported to v2.11 |
…j#4350) Signed-off-by: Alex Collins <alex_collins@intuit.com> Signed-off-by: Alex Capras <alexcapras@gmail.com>
Signed-off-by: Alex Collins alex_collins@intuit.com
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.Fixes #4338