aws / aws-iot-device-sdk-embedded-C

SDK for connecting to AWS IoT from a device using embedded C.
MIT License
974 stars 622 forks source link

New HTTP demo to generate a pre-signed URL to an S3 object file #1901

Closed giuspen closed 3 months ago

giuspen commented 4 months ago

New HTTP demo to generate a pre-signed URL to an S3 object file.

The setup is identical to the demo HTTP S3 download and in fact I implemented this based on that demo / starting from a duplication of demos/http/http_demo_s3_download.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

kstribrnAmzn commented 4 months ago

Thank you for this contribution! I will review your PR today and send this out to the team for further review.

kstribrnAmzn commented 4 months ago

Do you intend for this demo to only generate the pre-signed URL? Or would you also like it to download the item using the pre-signed URL?

kstribrnAmzn commented 4 months ago

There is a lot of duplicated code between your demo and the download demo (which you based it off of). Would you mind refactoring some of these functions to a common location like this?

giuspen commented 4 months ago

Thanks for your review @kstribrnAmzn , I will push new commits with the requested changes. I didn't think about also downloading the object because there is already another demo that is doing exactly that - the http download from S3 multi threaded takes a presigned URL as an input

kstribrnAmzn commented 4 months ago

Thank you so much @giuspen! Looking forward to reviewing when you push the changes. Keeping pre-signed URL creation and download as separate demos works for me. It should be far simpler for a new user to understand.

giuspen commented 4 months ago

I addressed all the points of the review except for the refactoring to share code with the other demo, will do that in one of the next few days...

giuspen commented 4 months ago

@kstribrnAmzn I refactored most of the common code in a shared .h/.c please let me know your thoughts

kstribrnAmzn commented 4 months ago

I'll take a look later today. Thank you for taking the time to refactor this :)

kstribrnAmzn commented 4 months ago

On first pass I don't see anything troubling. I will make a more thorough review tomorrow morning with fresh eyes and see if I can fix the failing workflows (or can make suggestions for how you may).

Thank you for your hard work on this! Its really awesome to see how the refactor you did shrunk both demos by over 1k lines combined. I'd says that's a huge win :)

kstribrnAmzn commented 4 months ago

/bot run uncrustify

kstribrnAmzn commented 4 months ago

I've pushed a commit applying some fixes from the workflow failures. I doubt I've got them all so I may be applying a further commit.

giuspen commented 4 months ago

Thanks for your review @kstribrnAmzn I'm pretty busy for a couple of days but then I'll process your comments!

giuspen commented 3 months ago

@kstribrnAmzn thanks again for your work on my PR, I removed the unneeded code that you spotted and the leftover from copy/paste in the README

kstribrnAmzn commented 3 months ago

Merged. Thank you for your hard work on this one @giuspen 😄

giuspen commented 3 months ago

Cool, thanks to you @kstribrnAmzn take care ;)