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: accept direct file upload for webhook/http (s3) #4903

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

HugoCasa
Copy link
Contributor

@HugoCasa HugoCasa commented Dec 11, 2024

Important

Add support for direct file uploads in webhook/http requests with S3 integration using a new WebhookArgs struct.

  • Behavior:
    • Add WebhookArgs struct in args.rs to handle webhook arguments and file uploads.
    • Implement to_push_args_owned() in WebhookArgs to process file uploads to S3.
    • Modify 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, and run_job_by_hash to use WebhookArgs.
  • Refactoring:
    • Move request handling logic from windmill-queue/src/jobs.rs to windmill-api/src/args.rs.
    • Remove redundant code from windmill-queue/src/jobs.rs.
  • Misc:
    • Add args.rs module to lib.rs.

This description was created by Ellipsis for e0f42bb. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 6 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:
    Using unwrap_or on content_type can lead to a panic if the header is not present. Consider using a more graceful handling method, such as unwrap_or_else with a default value or handling the None 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 because unwrap_or does not panic; it provides a default value. The suggestion to use unwrap_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 addresses unwrap_or, which is safe in this context.
    The use of unwrap_or is safe and appropriate here, and the comment does not provide strong evidence of an issue.
    The comment is incorrect regarding the behavior of unwrap_or and should be deleted.
2. backend/windmill-api/src/args.rs:64
  • Draft comment:
    Using unwrap_or on content_type can lead to a panic if the header is not present. Consider using a more graceful handling method, such as unwrap_or_else with a default value or handling the None case explicitly.
  • Reason this comment was not posted:
    Marked as duplicate.
3. backend/windmill-api/src/args.rs:66
  • Draft comment:
    Using unwrap_or on content_type can lead to a panic if the header is not present. Consider using a more graceful handling method, such as unwrap_or_else with a default value or handling the None case explicitly.
  • Reason this comment was not posted:
    Marked as duplicate.
4. backend/windmill-api/src/args.rs:68
  • Draft comment:
    Using unwrap_or on content_type can lead to a panic if the header is not present. Consider using a more graceful handling method, such as unwrap_or_else with a default value or handling the None case explicitly.
  • Reason this comment was not posted:
    Marked as duplicate.
5. backend/windmill-api/src/args.rs:70
  • Draft comment:
    Using unwrap_or on content_type can lead to a panic if the header is not present. Consider using a more graceful handling method, such as unwrap_or_else with a default value or handling the None 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.

Copy link

cloudflare-workers-and-pages bot commented Dec 11, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: e0f42bb
Status: ✅  Deploy successful!
Preview URL: https://9caceb0f.windmill.pages.dev
Branch Preview URL: https://hc-webhook-file-upload.windmill.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 the job_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 for get_random_file_name and upload_file_internal might break functionality if the job_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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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.

@rubenfiszel rubenfiszel merged commit 563a492 into main Dec 11, 2024
3 checks passed
@rubenfiszel rubenfiszel deleted the hc/webhook-file-upload branch December 11, 2024 23:33
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants