-
Notifications
You must be signed in to change notification settings - Fork 25
feat(aws-s3): Migrate S3 Upload task to use Data.From facility #653
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(aws-s3): Migrate S3 Upload task to use Data.From facility #653
Conversation
|
Quick heads up: I've posted about a Data API limitation with String.class in the sub issue kestra-io/kestra#12345. This affects the implementation approach. Would be great if you could check that before reviewing. Thank you! 🙏 |
|
Hi @Malaydewangan09, could you please review this PR when you have time? I'd appreciate any feedback or guidance on the changes. Thanks! |
|
Sure @KarthikReddyMaru, thanks for the contribution! 🚀 |
|
Looks quite OK to me 👍, @fdelbrayelle do you have any points to put up here or if I am overlooking something? |
fdelbrayelle
left a comment
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.
Check my comment
|
Unfortunately removing those lines breaks JSON array parsing. |
This PR migrates the
Uploadtask to use the standardizedData.Frompattern as part of the ubiquitousfrominitiative.closes kestra-io/kestra#12345
What changes are being made and why?
The
Uploadtask previously used manualObjecttype handling with pattern matching for Collections, JSON arrays, and single strings. This PR standardizes it to use theData.Fromfacility:Key Changes:
Data.Frominterface for consistency with other pluginsData.from().readAs()reactive APIData.From.TITLEandData.From.DESCRIPTIONinternalStorageURI = truefrom@PluginProperty(now handled by Data API)Implementation Details:
The migration uses a hybrid approach due to a Data API limitation: