-
Notifications
You must be signed in to change notification settings - Fork 578
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: accept direct file upload for webhook/http (s3) #4903
Conversation
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.
👍 Looks good to me! Reviewed everything up to d80154b in 1 minute and 57 seconds
More details
- Looked at
1119
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. backend/windmill-api/src/args.rs:60
- Draft comment:
Usingunwrap_or
oncontent_type
can lead to a panic if the header is not present. Consider using a more graceful handling method, such asunwrap_or_else
with a default value or handling theNone
case explicitly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is incorrect becauseunwrap_or
does not panic; it provides a default value. The suggestion to useunwrap_or_else
is not necessary unless there's a performance concern, which is not mentioned. The comment does not indicate a required code change.
I might be overlooking a subtlety in the code's logic that could cause a panic elsewhere, but the comment specifically addressesunwrap_or
, which is safe in this context.
The use ofunwrap_or
is safe and appropriate here, and the comment does not provide strong evidence of an issue.
The comment is incorrect regarding the behavior ofunwrap_or
and should be deleted.
2. backend/windmill-api/src/args.rs:64
- Draft comment:
Usingunwrap_or
oncontent_type
can lead to a panic if the header is not present. Consider using a more graceful handling method, such asunwrap_or_else
with a default value or handling theNone
case explicitly. - Reason this comment was not posted:
Marked as duplicate.
3. backend/windmill-api/src/args.rs:66
- Draft comment:
Usingunwrap_or
oncontent_type
can lead to a panic if the header is not present. Consider using a more graceful handling method, such asunwrap_or_else
with a default value or handling theNone
case explicitly. - Reason this comment was not posted:
Marked as duplicate.
4. backend/windmill-api/src/args.rs:68
- Draft comment:
Usingunwrap_or
oncontent_type
can lead to a panic if the header is not present. Consider using a more graceful handling method, such asunwrap_or_else
with a default value or handling theNone
case explicitly. - Reason this comment was not posted:
Marked as duplicate.
5. backend/windmill-api/src/args.rs:70
- Draft comment:
Usingunwrap_or
oncontent_type
can lead to a panic if the header is not present. Consider using a more graceful handling method, such asunwrap_or_else
with a default value or handling theNone
case explicitly. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_fmoy68eslnVrECXU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Deploying windmill with Cloudflare Pages
|
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.
👍 Looks good to me! Incremental review on 73007b3 in 21 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. backend/windmill-api/src/args.rs:24
- Draft comment:
Ensure that thejob_helpers_ee
module is correctly structured in the new location to avoid breaking functionality. - Reason this comment was not posted:
Confidence changes required:50%
The import path change forget_random_file_name
andupload_file_internal
might break functionality if thejob_helpers_ee
module is not correctly structured in the new location. This should be verified.
Workflow ID: wflow_ZiGPk7mdchQ7FMCR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on e0f42bb in 18 seconds
More details
- Looked at
16
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_dSDLVWCYe6QC225N
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Add support for direct file uploads in webhook/http requests with S3 integration using a new
WebhookArgs
struct.WebhookArgs
struct inargs.rs
to handle webhook arguments and file uploads.to_push_args_owned()
inWebhookArgs
to process file uploads to S3.update_payload
,route_job
,run_flow_by_path
,run_script_by_path
,run_wait_result_script_by_path
,run_wait_result_script_by_hash
,run_wait_result_flow_by_path
, andrun_job_by_hash
to useWebhookArgs
.windmill-queue/src/jobs.rs
towindmill-api/src/args.rs
.windmill-queue/src/jobs.rs
.args.rs
module tolib.rs
.This description was created by for e0f42bb. It will automatically update as commits are pushed.