feat: Upload input artifacts when submitting workflows from the UI#15237
Open
panicboat wants to merge 44 commits intoargoproj:mainfrom
Open
feat: Upload input artifacts when submitting workflows from the UI#15237panicboat wants to merge 44 commits intoargoproj:mainfrom
panicboat wants to merge 44 commits intoargoproj:mainfrom
Conversation
panicboat
changed the title
panicboat
force-pushed
the
feat/large-file-upload
branch
3 times, most recently
from
b3e076c to
9fd62ba
Compare
Member
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughWalkthroughThis PR implements input artifact upload functionality, allowing users to upload files from the UI when submitting workflows that define input artifacts in their WorkflowTemplate specifications. The feature includes backend API server enhancements for artifact uploads, streaming support across all artifact drivers, workflow submission modifications to apply artifact overrides, and a new React component for artifact file input in the submission panel. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/Browser
participant Client as ArtifactsInput<br/>(React Component)
participant Server as Argo Server<br/>(/upload-artifacts)
participant Handler as ArtifactServer<br/>.UploadInputArtifact
participant Repo as Artifact<br/>Repository
participant Driver as Artifact<br/>Driver
UI->>Client: Select & upload file
Client->>Server: POST file (multipart)<br/>with namespace, templateName, artifactName
Server->>Handler: Route to UploadInputArtifact
Handler->>Handler: Authenticate & parse request
Handler->>Repo: Load WorkflowTemplate<br/>& artifact config
Repo-->>Handler: Template & artifact details
Handler->>Handler: Generate UUID for artifact key
Handler->>Driver: SaveStream(reader,<br/>outputArtifact)
Driver->>Driver: Get storage client<br/>(S3, GCS, etc.)
Driver->>Repo: Upload file stream<br/>to configured storage
Repo-->>Driver: Upload complete
Driver-->>Handler: Return success
Handler->>Client: JSON response<br/>(name, key, location)
Client->>Client: Update state & display<br/>uploaded artifact key
Client->>UI: Show success indicator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (9)
workflow/controller/controller_test.go (1)
477-490: Seed the informer with the persisted object, not the pre-create stub.
GetIndexer().Add(taskResult)caches the local pre-create instance, so the store can miss API-populated metadata likeresourceVersionanduid. Add the object returned fromCreateinstead to keep the test cache faithful.♻️ Proposed fix
- _, err := woc.controller.wfclientset.ArgoprojV1alpha1().WorkflowTaskResults(woc.wf.Namespace). + createdTaskResult, err := woc.controller.wfclientset.ArgoprojV1alpha1().WorkflowTaskResults(woc.wf.Namespace). Create( ctx, taskResult, metav1.CreateOptions{}, ) if err != nil { panic(err) } @@ - if err := woc.controller.taskResultInformer.GetIndexer().Add(taskResult); err != nil { + if err := woc.controller.taskResultInformer.GetIndexer().Add(createdTaskResult); err != nil { panic(err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/controller/controller_test.go` around lines 477 - 490, The test seeds the informer with the pre-create stub "taskResult" which lacks API-set metadata; replace the stub with the persisted object returned by the Create call. After calling woc.controller.wfclientset.ArgoprojV1alpha1().WorkflowTaskResults(...).Create(...), capture the returned object (the created TaskResult) and call woc.controller.taskResultInformer.GetIndexer().Add(createdTaskResult) instead of Add(taskResult) so the informer's cache contains the API-populated resourceVersion/uid.workflow/artifacts/oss/oss_test.go (2)
72-97: Consider referencing the source list to avoid drift.
TestOssTransientErrorCodesduplicates the list of transient error codes from the implementation. IfossTransientErrorCodesin the source file changes, this test's expected list could become stale. Consider either:
- Iterating directly over the exported/package-level
ossTransientErrorCodesslice (asTestIsTransientOSSErrdoes on line 16), or- Adding a test that asserts the expected codes match the actual
ossTransientErrorCodesslice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/artifacts/oss/oss_test.go` around lines 72 - 97, TestOssTransientErrorCodes duplicates the transient-code list and may drift from the source; change it to reference the canonical ossTransientErrorCodes slice directly (or assert equality against it) instead of hardcoding values: update TestOssTransientErrorCodes to iterate over ossTransientErrorCodes (the package-level variable used by isTransientOSSErr) and call isTransientOSSErr for each entry (or add an assertion that the hardcoded expected list equals ossTransientErrorCodes), so the test stays in sync with the implementation.
99-103: Consider adding more meaningful assertions.This test only verifies that
maxObjectSizeequals 5GB, which is essentially testing a constant. While this documents the expected value, consider adding tests that verify the behavior dependent on this threshold (e.g., mocking file sizes to verify simple vs. multipart upload path selection) if such logic exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/artifacts/oss/oss_test.go` around lines 99 - 103, Test only asserts the constant maxObjectSize which isn't sufficient; extend TestPutFileSimpleVsMultipart to exercise the upload-path selection logic by invoking the function(s) that decide simple vs multipart upload (locate the code that uses maxObjectSize, e.g., the uploader method or helper that checks file size) with mocked file sizes just under and just over maxObjectSize and assert the expected path is chosen (e.g., verify that PutFileSimple is called for size < maxObjectSize and PutFileMultipart for size >= maxObjectSize), or mock the storage client to observe which upload routine runs when calling the public upload entrypoint used in production.ui/src/shared/models/submit-opts.ts (1)
5-7: Consider expanding the format documentation.The comment mentions
s3://andgcs://schemes, but the backend supports additional artifact types (Azure, OSS, HDFS, etc.). Consider updating to be more inclusive, e.g.,name=<scheme>://bucket/keyor listing additional supported schemes.📝 Suggested comment improvement
- // Artifacts to override for the workflow - // Format: name=s3://bucket/key or name=gcs://bucket/key + // Artifacts to override for the workflow + // Format: name=<scheme>://bucket/key (e.g., s3://, gcs://, azure://, oss://) or name=key🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/shared/models/submit-opts.ts` around lines 5 - 7, Update the JSDoc/comment for the artifacts field to describe a generic scheme format and include additional supported schemes; specifically change the comment above the artifacts?: string[] declaration to say something like "Format: name=<scheme>://bucket/key (e.g., s3://, gcs://, azure://, oss://, hdfs://, file://)" or similar so it covers backends beyond S3/GCS and documents that <scheme> is the storage provider.workflow/artifacts/azure/azure.go (1)
307-323: Consider wrapping the UploadStream error for consistency.The error from
UploadStream(line 321-322) is returned directly without context. Other methods in this file wrap errors with descriptive messages (e.g., lines 289-290, 295-296, 300-301). For consistency and better debugging, consider wrapping this error.♻️ Suggested fix
blobClient := containerClient.NewBlockBlobClient(outputArtifact.Azure.Blob) _, err = blobClient.UploadStream(ctx, reader, nil) - return err + if err != nil { + return fmt.Errorf("unable to upload stream to blob %s: %w", outputArtifact.Azure.Blob, err) + } + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/artifacts/azure/azure.go` around lines 307 - 323, In SaveStream, wrap the error returned from blobClient.UploadStream with a descriptive message instead of returning it raw; update the return to use fmt.Errorf("unable to upload Azure blob %s to container %s: %w", outputArtifact.Azure.Blob, outputArtifact.Azure.Container, err) (or similar) so failures from blobClient.UploadStream are consistent with other wrapped errors in this file.server/workflow/workflow_server.go (2)
834-847: Consider removing verbose debug logging before merge.The anonymous function for
artifactsCountadds complexity for a debug log. This level of detail may be excessive for production. Consider simplifying or using conditional logging.💡 Suggested simplification
logger.WithFields(logging.Fields{ - "submitOptions": req.SubmitOptions, "hasSubmitOptions": req.SubmitOptions != nil, - "artifactsCount": func() int { - if req.SubmitOptions != nil { - return len(req.SubmitOptions.Artifacts) - } - return -1 - }(), + "artifactsCount": len(req.GetSubmitOptions().GetArtifacts()), "hasWorkflowTemplateRef": wf.Spec.WorkflowTemplateRef != nil, }).Debug(ctx, "SubmitWorkflow: checking conditions")Note: This assumes proto-generated getters exist. If not, keep the nil-safe approach but simplify formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/workflow/workflow_server.go` around lines 834 - 847, The debug block is overly verbose and uses an unnecessary anonymous function for "artifactsCount"; simplify logging by removing the closure and either omit "artifactsCount" entirely or compute it in a nil-safe way before the logger call (e.g. set a local int var artifactsCount = -1; if req.SubmitOptions != nil { artifactsCount = len(req.SubmitOptions.Artifacts) }) and then call logging.RequireLoggerFromContext(ctx).WithFields(...).Debug(ctx, ...) including only the needed fields (e.g. "hasSubmitOptions", "artifactsCount" or drop it) and keep the wf.Spec.WorkflowTemplateRef check as-is to avoid the inline lambda complexity.
882-887: Silently skipping malformed artifact overrides may hide user errors.If an override string doesn't contain
=, it's silently ignored. Consider logging a warning or returning an error so users know their input was not applied.💡 Suggested improvement
for _, artifactStr := range req.SubmitOptions.Artifacts { parts := strings.SplitN(artifactStr, "=", 2) if len(parts) == 2 { overrides[parts[0]] = parts[1] + } else { + logger.WithField("artifact", artifactStr).Warn(ctx, "Ignoring malformed artifact override (expected format: name=key)") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/workflow/workflow_server.go` around lines 882 - 887, The loop parsing req.SubmitOptions.Artifacts silently ignores entries without "=", which can hide user mistakes; update the parsing in the artifact override handling (the loop over req.SubmitOptions.Artifacts that fills overrides) to detect malformed artifactStr values and either log a warning that includes the offending artifactStr and context (e.g., which request/user) or return a validation error to the caller; specifically, replace the current unconditional skip with a branch that calls the server's logger (or returns an error from the submit handler) when len(parts) != 2 so users are informed their override was not applied.workflow/artifacts/hdfs/hdfs_test.go (1)
182-188: Consider asserting error type unconditionally.The
if okcheck means the test silently passes if the error is not anArgoError. This could mask regressions if the error type changes.💡 Suggested improvement
// Verify it's a CodeNotImplemented error var argoErr argoerrors.ArgoError ok := errors.As(err, &argoErr) - if ok { - assert.Equal(t, argoerrors.CodeNotImplemented, argoErr.Code()) - } + require.True(t, ok, "expected ArgoError type") + assert.Equal(t, argoerrors.CodeNotImplemented, argoErr.Code())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/artifacts/hdfs/hdfs_test.go` around lines 182 - 188, The test currently uses errors.As(err, &argoErr) with an if ok guard which allows the test to silently pass if the error isn't an ArgoError; replace the conditional with an unconditional assertion that the error is an ArgoError (e.g., require.True(t, errors.As(err, &argoErr)) or assert.True(t, ok) immediately after calling errors.As) and then assert the Code() equals argoerrors.CodeNotImplemented on argoErr; update the call sites around errors.As and argoErr to ensure the test fails if the type assertion fails.ui/src/shared/components/artifacts-input/artifacts-input.tsx (1)
14-18: Consider typinglocationmore specifically.
location: anyloses type safety. If the backend response structure is known, define it explicitly or import the type from a shared location.💡 Suggested improvement
export interface ArtifactUploadResponse { name: string; key: string; - location: any; + location: Record<string, unknown>; // Or a more specific ArtifactLocation type }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/shared/components/artifacts-input/artifacts-input.tsx` around lines 14 - 18, The ArtifactUploadResponse interface uses location: any which removes type safety; replace it with a specific type (e.g., define an interface like ArtifactLocation { url: string; bucket?: string; region?: string; ... } or import an existing backend/shared type) and update ArtifactUploadResponse to use location: ArtifactLocation (or the imported type), then adjust any consumers of ArtifactUploadResponse to match the new shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/artifacts/artifact_server.go`:
- Around line 193-195: The newKey construction uses user-controlled
header.Filename directly (see artifactCopy.GetKey() usage and newKey variable);
sanitize header.Filename first to prevent path traversal by replacing/backing
out path separators and resolving to a safe basename (e.g., use filepath.Base
and strip any remaining ".." or path separators, reject empty or dot-only names,
and optionally normalize to a safe character set), then build newKey from the
sanitized filename before storing or using it.
- Around line 243-247: The generateUUID function currently ignores the error
from rand.Read which can produce an all-zero UUID on failure; change
generateUUID to return (string, error) and propagate the rand.Read error: check
the error from rand.Read(b), return a descriptive error if non-nil, otherwise
hex-encode and return the UUID; then update the caller that invokes generateUUID
(the site referenced in the diff) to handle the returned error (propagate it,
log and return, or fail the operation as appropriate) so failures are not
silently ignored.
In `@ui/src/shared/components/artifacts-input/artifacts-input.tsx`:
- Line 79: The URL constructed for the POST in artifacts-input.tsx uses raw
template variables and can break with special characters; update the xhr.open
call so namespace, workflowTemplateName, and artifactName are each wrapped with
encodeURIComponent (e.g., use encodeURIComponent(namespace),
encodeURIComponent(workflowTemplateName), encodeURIComponent(artifactName))
before building the `/upload-artifacts/...` path to ensure the request URL is
correctly encoded.
- Around line 107-110: The dropzone text claims drag-and-drop support but no
drag handlers are implemented; either add proper drag handlers (onDrop,
onDragOver/onDragEnter, and corresponding handlers that call the existing
handleFileSelect or a new handler to accept dropped files) on the
div.artifacts-input__dropzone, or simply remove the "or drag and drop" copy and
change the span text to "Click to select a file" (or similar) to reflect the
current behavior; reference the div with className 'artifacts-input__dropzone'
and the existing handleFileSelect function when making the change.
In `@ui/src/workflows/components/submit-workflow-panel.tsx`:
- Around line 72-86: The submit path uses uploadedArtifacts directly and can run
before pending uploads complete; modify the submit flow in
submit-workflow-panel.tsx (the block that builds artifactOverrides and calls
services.workflows.submit) to first wait for any in-flight uploads to finish
(e.g., await Promise.all of upload promise list or wait for onUploadComplete to
resolve) or disallow clicking by blocking until all uploads complete, and only
then build artifactOverrides from the final uploadedArtifacts and set
isSubmitting; ensure you reference the same upload completion mechanism used
elsewhere (onUploadComplete/uploadPromises/uploadState) so the artifacts array
passed to services.workflows.submit includes newly completed uploads.
In `@workflow/artifacts/gcs/gcs_test.go`:
- Around line 109-113: TestListFileRelPathsNonExistent uses a hard-coded POSIX
path; update the test to create an OS-neutral non-existent path by using
t.TempDir() and building the missing path with filepath.Join (e.g.,
filepath.Join(t.TempDir(), "nonexistent")) before calling listFileRelPaths so
the test works on Windows and POSIX systems; reference
TestListFileRelPathsNonExistent and listFileRelPaths and ensure the constructed
path does not get created so require.Error(t, err) still holds.
In `@workflow/artifacts/gcs/gcs.go`:
- Around line 244-267: SaveStream currently retries uploads using
waitutil.Backoff while consuming the provided io.Reader via io.Copy, which
breaks retries because the reader cannot be rewound; fix by buffering the
incoming stream to a temp file before entering the retry loop: create a temp
file (os.CreateTemp), copy the entire io.Reader into it once, close it, and then
in the retry closure open the temp file (os.Open), seek to start (or rely on
fresh open), use that file as the source for io.Copy to the GCS writer, close
the opened file each attempt, and remove the temp file after success/final
failure; keep the SaveStream signature unchanged and reference SaveStream,
io.Reader, io.Copy, waitutil.Backoff, newGCSClient, and outputArtifact.GCS.Key
when implementing.
In `@workflow/artifacts/http/http.go`:
- Around line 143-179: SaveStream currently constructs the request body from a
non-rewindable io.Reader so req.GetBody is not set and 307/308 redirects cannot
be followed; mirror the approach used in Save by fully buffering the reader into
a byte slice (e.g., io.ReadAll) before creating the http.Request, set the
request Body to a new bytes.Reader/io.NopCloser over that buffer, and assign
req.GetBody to a function that returns a fresh io.ReadCloser (bytes.NewReader
wrapped) so the client can replay the body on redirects; update the branch that
handles outputArtifact.HTTP (and the Artifactory branch if appropriate) to use
the buffered payload before calling h.Client.Do(req).
In `@workflow/artifacts/oss/oss.go`:
- Around line 280-302: The closure passed to waitutil.Backoff retries using the
same non-seekable reader, so subsequent attempts to
bucket.PutObject(outputArtifact.OSS.Key, reader) may upload zero/truncated data;
fix by materializing the stream before the retry loop (e.g., read into a byte
slice or use an io.ReadSeeker) and inside the closure create a fresh io.Reader
for each attempt (use bytes.NewReader(data) or seek to 0 on a ReadSeeker) so
PutObject always receives a full reader; update references around
waitutil.Backoff, PutObject, reader, and any callers of
setBucketLogging/ossDriver.newOSSClient/isTransientOSSErr accordingly.
- Around line 288-300: SaveStream currently skips the bucket creation and
lifecycle configuration that Save performs: before calling setBucketLogging,
bucket := osscli.Bucket(...) and bucket.PutObject(...) you must mirror Save’s
setup by invoking CreateBucketIfNotPresent (or equivalent) for
outputArtifact.OSS.Bucket, then apply the same lifecycle rule handling (the
function or logic used by Save to set lifecycle rules), and only after those
succeed call setBucketLogging and bucket.PutObject; update the SaveStream
function (and any helper it uses) to call CreateBucketIfNotPresent, apply the
lifecycle configuration, then proceed to setBucketLogging and PutObject so
stream uploads behave identically to Save.
In `@workflow/artifacts/s3/s3_test.go`:
- Around line 825-833: The tests are invoking the mock client's PutStream
directly (reader := strings.NewReader(tc.content); err :=
tc.s3client.PutStream(...)) which bypasses ArtifactDriver.SaveStream and
therefore doesn't exercise error wrapping, retry logic, or use of the
errMsg/done fields; change the test to call ArtifactDriver.SaveStream (or the
production SaveStream wrapper used by production code) with the same
bucket/key/reader and the done channel so the driver invokes the s3client and
performs its full logic, then assert against the returned error and the expected
errMsg/done outcomes instead of asserting the mock client's direct PutStream
return value; reference the s3client.PutStream mock, ArtifactDriver.SaveStream
method, and the test case fields errMsg and done when making these changes.
In `@workflow/artifacts/s3/s3.go`:
- Line 285: SaveStream currently retries s3cli.PutStream using the same
io.Reader without resetting it, causing partial uploads for non-seekable
readers; modify SaveStream to either (A) detect seekable readers via type
assertion to io.Seeker and call Seek(0,0) before each retry of s3cli.PutStream
(ensure you handle and log Seek errors), or (B) enforce only seekable readers by
returning an error if the provided reader does not implement io.Seeker, or (C)
buffer the entire reader into memory/temp file before calling s3cli.PutStream so
retries start from the buffered beginning; locate the retry loop around the call
to s3cli.PutStream and implement one of these fixes for SaveStream to guarantee
full uploads on retry.
In `@workflow/util/merge.go`:
- Around line 137-144: The current loop replaces the already-merged
targetWf.Spec.Arguments.Artifacts[index] with the raw artifact from
wfSpec/wftSpec, discarding lower-priority fields; instead, keep the existing
target artifact as the base and overlay missing/non-zero fields from the
higher-precedence artifact. In the loop that iterates
targetWf.Spec.Arguments.Artifacts (using wfArtifactsMap/wftArtifactsMap and
artifactsToMapByName), create a copy of the current target artifact, then copy
only the non-empty fields from art.DeepCopy() into that base (or perform a
shallow/field-wise merge) and assign the merged result back to
targetWf.Spec.Arguments.Artifacts[index] so key-only overrides preserve
bucket/endpoint and other inherited fields.
---
Nitpick comments:
In `@server/workflow/workflow_server.go`:
- Around line 834-847: The debug block is overly verbose and uses an unnecessary
anonymous function for "artifactsCount"; simplify logging by removing the
closure and either omit "artifactsCount" entirely or compute it in a nil-safe
way before the logger call (e.g. set a local int var artifactsCount = -1; if
req.SubmitOptions != nil { artifactsCount = len(req.SubmitOptions.Artifacts) })
and then call logging.RequireLoggerFromContext(ctx).WithFields(...).Debug(ctx,
...) including only the needed fields (e.g. "hasSubmitOptions", "artifactsCount"
or drop it) and keep the wf.Spec.WorkflowTemplateRef check as-is to avoid the
inline lambda complexity.
- Around line 882-887: The loop parsing req.SubmitOptions.Artifacts silently
ignores entries without "=", which can hide user mistakes; update the parsing in
the artifact override handling (the loop over req.SubmitOptions.Artifacts that
fills overrides) to detect malformed artifactStr values and either log a warning
that includes the offending artifactStr and context (e.g., which request/user)
or return a validation error to the caller; specifically, replace the current
unconditional skip with a branch that calls the server's logger (or returns an
error from the submit handler) when len(parts) != 2 so users are informed their
override was not applied.
In `@ui/src/shared/components/artifacts-input/artifacts-input.tsx`:
- Around line 14-18: The ArtifactUploadResponse interface uses location: any
which removes type safety; replace it with a specific type (e.g., define an
interface like ArtifactLocation { url: string; bucket?: string; region?: string;
... } or import an existing backend/shared type) and update
ArtifactUploadResponse to use location: ArtifactLocation (or the imported type),
then adjust any consumers of ArtifactUploadResponse to match the new shape.
In `@ui/src/shared/models/submit-opts.ts`:
- Around line 5-7: Update the JSDoc/comment for the artifacts field to describe
a generic scheme format and include additional supported schemes; specifically
change the comment above the artifacts?: string[] declaration to say something
like "Format: name=<scheme>://bucket/key (e.g., s3://, gcs://, azure://, oss://,
hdfs://, file://)" or similar so it covers backends beyond S3/GCS and documents
that <scheme> is the storage provider.
In `@workflow/artifacts/azure/azure.go`:
- Around line 307-323: In SaveStream, wrap the error returned from
blobClient.UploadStream with a descriptive message instead of returning it raw;
update the return to use fmt.Errorf("unable to upload Azure blob %s to container
%s: %w", outputArtifact.Azure.Blob, outputArtifact.Azure.Container, err) (or
similar) so failures from blobClient.UploadStream are consistent with other
wrapped errors in this file.
In `@workflow/artifacts/hdfs/hdfs_test.go`:
- Around line 182-188: The test currently uses errors.As(err, &argoErr) with an
if ok guard which allows the test to silently pass if the error isn't an
ArgoError; replace the conditional with an unconditional assertion that the
error is an ArgoError (e.g., require.True(t, errors.As(err, &argoErr)) or
assert.True(t, ok) immediately after calling errors.As) and then assert the
Code() equals argoerrors.CodeNotImplemented on argoErr; update the call sites
around errors.As and argoErr to ensure the test fails if the type assertion
fails.
In `@workflow/artifacts/oss/oss_test.go`:
- Around line 72-97: TestOssTransientErrorCodes duplicates the transient-code
list and may drift from the source; change it to reference the canonical
ossTransientErrorCodes slice directly (or assert equality against it) instead of
hardcoding values: update TestOssTransientErrorCodes to iterate over
ossTransientErrorCodes (the package-level variable used by isTransientOSSErr)
and call isTransientOSSErr for each entry (or add an assertion that the
hardcoded expected list equals ossTransientErrorCodes), so the test stays in
sync with the implementation.
- Around line 99-103: Test only asserts the constant maxObjectSize which isn't
sufficient; extend TestPutFileSimpleVsMultipart to exercise the upload-path
selection logic by invoking the function(s) that decide simple vs multipart
upload (locate the code that uses maxObjectSize, e.g., the uploader method or
helper that checks file size) with mocked file sizes just under and just over
maxObjectSize and assert the expected path is chosen (e.g., verify that
PutFileSimple is called for size < maxObjectSize and PutFileMultipart for size
>= maxObjectSize), or mock the storage client to observe which upload routine
runs when calling the public upload entrypoint used in production.
In `@workflow/controller/controller_test.go`:
- Around line 477-490: The test seeds the informer with the pre-create stub
"taskResult" which lacks API-set metadata; replace the stub with the persisted
object returned by the Create call. After calling
woc.controller.wfclientset.ArgoprojV1alpha1().WorkflowTaskResults(...).Create(...),
capture the returned object (the created TaskResult) and call
woc.controller.taskResultInformer.GetIndexer().Add(createdTaskResult) instead of
Add(taskResult) so the informer's cache contains the API-populated
resourceVersion/uid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 140aa4f4-ba19-4e84-84b3-637f4f777d5b
⛔ Files ignored due to path filters (5)
api/openapi-spec/swagger.jsonis excluded by!**/api/openapi-spec/*.jsonpkg/apis/workflow/v1alpha1/generated.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/apis/workflow/v1alpha1/openapi_generated.gois excluded by!**/*_generated.gopkg/apis/workflow/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.*.gosdks/java/client/docs/IoArgoprojWorkflowV1alpha1SubmitOpts.mdis excluded by!**/sdks/java/client/**
📒 Files selected for processing (43)
.features/pending/input-artifact-upload.md.spellingapi/jsonschema/schema.jsondocs/fields.mdexamples/workflow-template/input-artifacts.yamlpkg/apiclient/argo-kube-client.gopkg/apis/api-rules/violation_exceptions.listpkg/apis/workflow/v1alpha1/common.gopkg/apis/workflow/v1alpha1/generated.protoserver/apiserver/argoserver.goserver/artifacts/artifact_server.goserver/artifacts/artifact_server_test.goserver/workflow/workflow_server.goserver/workflow/workflow_server_test.goui/src/shared/components/artifacts-input/artifacts-input.scssui/src/shared/components/artifacts-input/artifacts-input.tsxui/src/shared/components/artifacts-input/index.tsui/src/shared/models/submit-opts.tsui/src/workflow-templates/workflow-template-details.tsxui/src/workflows/components/submit-workflow-panel.tsxui/webpack.config.jsworkflow/artifacts/azure/azure.goworkflow/artifacts/azure/azure_test.goworkflow/artifacts/common/common.goworkflow/artifacts/gcs/gcs.goworkflow/artifacts/gcs/gcs_test.goworkflow/artifacts/git/git.goworkflow/artifacts/hdfs/hdfs.goworkflow/artifacts/hdfs/hdfs_test.goworkflow/artifacts/http/http.goworkflow/artifacts/logging/driver.goworkflow/artifacts/logging/driver_test.goworkflow/artifacts/oss/oss.goworkflow/artifacts/oss/oss_test.goworkflow/artifacts/plugin/plugin.goworkflow/artifacts/raw/raw.goworkflow/artifacts/s3/s3.goworkflow/artifacts/s3/s3_test.goworkflow/controller/controller_test.goworkflow/util/merge.goworkflow/util/merge_test.goworkflow/util/util.goworkflow/util/util_test.go
Comment on lines
+288
to
+300
| bucketName := outputArtifact.OSS.Bucket | ||
| err = setBucketLogging(osscli, bucketName) | ||
| if err != nil { | ||
| return !isTransientOSSErr(ctx, err), err | ||
| } | ||
| bucket, err := osscli.Bucket(bucketName) | ||
| if err != nil { | ||
| return !isTransientOSSErr(ctx, err), err | ||
| } | ||
| err = bucket.PutObject(outputArtifact.OSS.Key, reader) | ||
| if err != nil { | ||
| return !isTransientOSSErr(ctx, err), err | ||
| } |
Contributor
There was a problem hiding this comment.
Keep SaveStream's setup behavior aligned with Save.
This path goes straight to Bucket/PutObject and skips CreateBucketIfNotPresent plus LifecycleRule handling that Save performs. Stream uploads can therefore fail against auto-create repos or silently miss lifecycle configuration even though file-based uploads work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/artifacts/oss/oss.go` around lines 288 - 300, SaveStream currently
skips the bucket creation and lifecycle configuration that Save performs: before
calling setBucketLogging, bucket := osscli.Bucket(...) and bucket.PutObject(...)
you must mirror Save’s setup by invoking CreateBucketIfNotPresent (or
equivalent) for outputArtifact.OSS.Bucket, then apply the same lifecycle rule
handling (the function or logic used by Save to set lifecycle rules), and only
after those succeed call setBucketLogging and bucket.PutObject; update the
SaveStream function (and any helper it uses) to call CreateBucketIfNotPresent,
apply the lifecycle configuration, then proceed to setBucketLogging and
PutObject so stream uploads behave identically to Save.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2022b92. OSS SaveStream now buffers the stream to a temp file before entering the retry loop, so the reader is fresh on each retry.
Contributor
There was a problem hiding this comment.
@panicboat, thank you for the update! The temp-file buffering is a good improvement — it ensures the io.Reader can be re-read on each retry attempt without exhausting the stream.
However, the original concern is still present in the updated code. Looking at the current SaveStream implementation in the file, it still skips two pieces of setup that Save performs:
-
CreateBucketIfNotPresent—Saveconditionally creates the bucket ifoutputArtifact.OSS.CreateBucketIfNotPresentis set:if outputArtifact.OSS.CreateBucketIfNotPresent { exists, existsErr := osscli.IsBucketExist(bucketName) // ... if !exists { err = osscli.CreateBucket(bucketName) } }
SaveStreamhas no equivalent, so stream uploads will fail against auto-create repos where file-based uploads work fine. -
LifecycleRule—SavecallssetBucketLifecycleRule(osscli, outputArtifact.OSS)when a lifecycle rule is configured.SaveStreamomits this entirely, so lifecycle policies won't be applied on stream-based uploads.
Could you add those two blocks inside the waitutil.Backoff closure of SaveStream, mirroring what Save does? For example, after setBucketLogging and before osscli.Bucket(...):
if outputArtifact.OSS.CreateBucketIfNotPresent {
exists, existsErr := osscli.IsBucketExist(bucketName)
if existsErr != nil {
return !isTransientOSSErr(ctx, existsErr), fmt.Errorf("failed to check if bucket %s exists: %w", bucketName, existsErr)
}
if !exists {
if createErr := osscli.CreateBucket(bucketName); createErr != nil {
return !isTransientOSSErr(ctx, createErr), fmt.Errorf("failed to automatically create bucket %s: %w", bucketName, createErr)
}
}
}
// ...obtain bucket...
if outputArtifact.OSS.LifecycleRule != nil {
if lcErr := setBucketLifecycleRule(osscli, outputArtifact.OSS); lcErr != nil {
return !isTransientOSSErr(ctx, lcErr), lcErr
}
}
Contributor
Author
There was a problem hiding this comment.
Good catch. Fixed in 8033441 — added CreateBucketIfNotPresent and LifecycleRule setup to SaveStream to match Save behavior.
Comment on lines
+825
to
+833
| reader := strings.NewReader(tc.content) | ||
| err := tc.s3client.PutStream(tc.bucket, tc.key, reader, int64(len(tc.content))) | ||
| if tc.errMsg == "" { | ||
| require.NoError(t, err) | ||
| } else { | ||
| require.Error(t, err) | ||
| // For this test, we're testing the mock client directly | ||
| // The actual error wrapping happens in the ArtifactDriver.SaveStream | ||
| } |
Contributor
There was a problem hiding this comment.
Exercise the real save-stream path here.
Both tests call PutStream on the mock directly, so they still pass if ArtifactDriver.SaveStream stops wrapping errors, skips retry handling, or never invokes the client at all. The unused errMsg/done fields are a symptom of that gap.
Also applies to: 861-867
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/artifacts/s3/s3_test.go` around lines 825 - 833, The tests are
invoking the mock client's PutStream directly (reader :=
strings.NewReader(tc.content); err := tc.s3client.PutStream(...)) which bypasses
ArtifactDriver.SaveStream and therefore doesn't exercise error wrapping, retry
logic, or use of the errMsg/done fields; change the test to call
ArtifactDriver.SaveStream (or the production SaveStream wrapper used by
production code) with the same bucket/key/reader and the done channel so the
driver invokes the s3client and performs its full logic, then assert against the
returned error and the expected errMsg/done outcomes instead of asserting the
mock client's direct PutStream return value; reference the s3client.PutStream
mock, ArtifactDriver.SaveStream method, and the test case fields errMsg and done
when making these changes.
Contributor
Author
There was a problem hiding this comment.
Noted. The existing S3 tests cover the underlying client behavior. Adding SaveStream-specific integration tests is a good follow-up but out of scope for this PR.
Contributor
There was a problem hiding this comment.
@panicboat, understood — the existing tests do validate the mock client's PutStream behaviour, and tightening the coverage around ArtifactDriver.SaveStream's retry/error-wrapping logic can reasonably land in a follow-up. Would you like me to open a GitHub issue to track that work?
Contributor
Author
There was a problem hiding this comment.
Yes, please open an issue for that. Happy to address it in a follow-up PR.
Comment on lines
+137
to
+144
| wfArtifactsMap := artifactsToMapByName(wfSpec) | ||
| wftArtifactsMap := artifactsToMapByName(wftSpec) | ||
| for index, artifact := range targetWf.Spec.Arguments.Artifacts { | ||
| if art, ok := wfArtifactsMap[artifact.Name]; ok { | ||
| targetWf.Spec.Arguments.Artifacts[index] = *art.DeepCopy() | ||
| } else if art, ok := wftArtifactsMap[artifact.Name]; ok { | ||
| targetWf.Spec.Arguments.Artifacts[index] = *art.DeepCopy() | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve inherited artifact fields during precedence resolution.
targetWf.Spec.Arguments.Artifacts[index] is already the merged value. Replacing it with the raw wfSpec/wftSpec artifact drops lower-priority fields, so a key-only override can lose the bucket/endpoint copied from the template/defaults.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/util/merge.go` around lines 137 - 144, The current loop replaces the
already-merged targetWf.Spec.Arguments.Artifacts[index] with the raw artifact
from wfSpec/wftSpec, discarding lower-priority fields; instead, keep the
existing target artifact as the base and overlay missing/non-zero fields from
the higher-precedence artifact. In the loop that iterates
targetWf.Spec.Arguments.Artifacts (using wfArtifactsMap/wftArtifactsMap and
artifactsToMapByName), create a copy of the current target artifact, then copy
only the non-empty fields from art.DeepCopy() into that base (or perform a
shallow/field-wise merge) and assign the merged result back to
targetWf.Spec.Arguments.Artifacts[index] so key-only overrides preserve
bucket/endpoint and other inherited fields.
Contributor
Author
There was a problem hiding this comment.
The current behavior is intentional. When an artifact override is provided, DeepCopy() replaces the entire artifact, which is the correct semantics for overriding uploaded artifact keys. Partial field overlay would risk mixing stale location fields with the new key.
Contributor
There was a problem hiding this comment.
@panicboat, understood — that's a clear and valid rationale. Full replacement ensures the uploaded artifact's location is authoritative and avoids any confusion from partially inherited stale location fields. I'll defer to this design.
✏️ Learnings added
Learnt from: panicboat
Repo: argoproj/argo-workflows PR: 15237
File: workflow/util/merge.go:137-144
Timestamp: 2026-04-06T10:00:46.495Z
Learning: In `workflow/util/merge.go` (`JoinWorkflowSpec`), the artifact precedence loop intentionally replaces `targetWf.Spec.Arguments.Artifacts[index]` with a full `DeepCopy()` of the higher-priority artifact (from `wfSpec` or `wftSpec`). This is by design: when an artifact key is overridden (e.g., after a UI upload), the entire artifact location should be replaced, not merged field-by-field, to avoid mixing stale bucket/endpoint fields from the template with the new key.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
panicboat
force-pushed
the
feat/large-file-upload
branch
from
cea013c to
fc0e07d
Compare
Contributor
Author
|
Addressed the nitpick comments from the review:
|
panicboat
force-pushed
the
feat/large-file-upload
branch
3 times, most recently
from
154ed88 to
d7bdeb0
Compare
…loads Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
…line character Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
…truncated uploads Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
…load Replace `location: any` with `Record<string, unknown>` for type safety, extract upload logic into shared `uploadFile` function, and add drag-and-drop event handlers to the dropzone element. Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
Signed-off-by: panicboat <panicboat@gmail.com>
panicboat
force-pushed
the
feat/large-file-upload
branch
from
8cc621a to
acf237c
Compare
Contributor
Author
|
@Joibel @terrytangyuan Also, on a separate note, the CI seems to be a bit unstable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #12656
Motivation
Currently, when submitting a workflow from the UI, users cannot provide input artifacts directly.
The only way to use input artifacts is to pre-upload files to the artifact repository and manually specify the key in the WorkflowTemplate.
This creates a poor user experience, especially for ad-hoc workflow executions that require different input files each time.
This PR enables users to upload files directly through the UI when submitting a workflow, making input artifacts truly usable from the web interface.
Modifications
Backend:
SaveStreammethod to the ArtifactDriver interface for streaming uploadsSaveStreamin all artifact drivers (S3, GCS, Azure, OSS, HDFS, HTTP, Git, Raw, Plugin)POST /upload-artifacts/{namespace}/{workflowTemplateName}/{artifactName}for file uploadsJoinWorkflowSpecto ensure workflow artifact overrides take precedence over template defaultsFrontend:
upload-artifactsto webpack proxy configuration for developmentArchitecture
sequenceDiagram participant User participant UI as Argo UI participant Server as Argo Server participant S3 as S3/GCS/Azure participant K8s as Kubernetes User->>UI: Select file UI->>Server: POST /upload-artifacts/{ns}/{tmpl}/{artifact} Server->>K8s: Get WorkflowTemplate K8s-->>Server: Template config Server->>S3: Upload file (new key) S3-->>Server: Success Server-->>UI: {name, key, location} User->>UI: Click Submit UI->>Server: POST /api/v1/workflows/{ns}/submit Note right of UI: artifacts: ["name=newKey"] Server->>K8s: Get WorkflowTemplate Server->>Server: Override artifact key Server->>K8s: Create Workflow K8s-->>Server: Workflow Server-->>UI: SuccessData flow
flowchart TD subgraph Frontend A[Select File] --> B[XHR Upload] B --> C{Success?} C -->|Yes| D[Store in uploadedArtifacts] C -->|No| E[Show Error] D --> F[Submit Button] F --> G[Build artifactOverrides] G --> H[POST /submit] end subgraph Backend Upload B --> I[UploadInputArtifact] I --> J[Get WorkflowTemplate] J --> K[Get artifact config] K --> L[Generate new key] L --> M[SaveStream] M --> N[Return response] end subgraph Backend Submit H --> O[SubmitWorkflow] O --> P[Get WorkflowTemplate] P --> Q[Copy artifact config] Q --> R[Override key] R --> S[Create Workflow] endVerification
arguments.artifacts2026-01-15.11.43.54.mov
Scope
ClusterWorkflowTemplate and CLI support is out of scope for this PR.
This pull request focuses on using the workflow template web UI upload feature. ClusterWorkflowTemplate and CLI users can continue using the existing workflow: pre-upload files to the artifact repository and specify the key manually.
ClusterWorkflowTemplate and CLI support (e.g.,
argo submit --from wftmpl/name --input-artifact name=./file.zip) can be added in a future PR, reusing the/upload-artifacts/endpoint introduced here.Documentation
Added feature documentation in
.features/pending/input-artifact-upload.md.Users can discover this feature when they:
Summary by CodeRabbit
Release Notes
New Features
Documentation