S3Uploader Update

[jswank]

Generalize the S3Uploader method - only a bucket name is required.

Other aspects of the AWS connection are configured using standard AWS_ environment variables.

A few more comments on the old code- copypasta from https://jswank.prose.sh/2023-03-30-javascript_and_openai#the-module:

1. Using AWS credentials in this fashion is not required - rely on the SDK to use the AWS_ environment variables directly.
2. Requiring the region to be set in this fashion is not required - rely on the variables used by the SDK, and set a default.
3. Constructing URLs for S3 buckets in this fasion (path-based) is deprecated: a virtual host approach should be used.
4. The version of the SDK is v2 - making this use v3 should be trivial.

[MatthewGidcomb]

I've never liked how this had to check for so many env vars, and dreaded having to repeat that whenever we get around to Azure Blob Storage. This is so much better, thanks.

I think the E2E tests are failing because it's trying and failing to upload images to S3 (based on the logs in the test artifacts here). I thought I had that configured to save images locally, but I see in docker-compose.test.yml that it's trying to pull the old AWS vars in from the environment. We can probably remove all that now, including (and especially) the bucket var,, right?

[jswank]

Thanks for looking! I wasn't sure if this test was accurate or not...

The logic with respect to deciding whether to use S3 or the local uploader changed a bit; this test was affected.

The old way: S3 would be used if a bunch of variables were set. The new way depends only on whether AWS_S3_UPLOADS_BUCKET is set. The GA-based Cypress test was previously using the local uploader because AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY were unset. My change- without modifying the docker-compose.test.yml file- caused the test to try to use S3 - and fail because those credentials were unset.

The change to docker-compose.test.yml makes the test pass by unsetting AWS_S3_UPLOADS_BUCKET.

[MatthewGidcomb]

We had discussed the docker-compose.test.yml setup Sunday but didn't have time to mess with it. Would like to get @chriswininger's signoff on that change before we merge.

Other than that, I have two small questions/comments.

[jswank]

I don't know what the best practice is for NPM: I don't think there is a practical reason to prefer one over the other in this case.

On Thu, Apr 6, 2023, at 14:32, Matthew Gidcomb wrote:

@.**** commented on this pull request.

In web-portal/package.json https://github.com/infinite-industries/infinite/pull/388#discussion_r1160275774:

• "aws-sdk": "^2.281.1",
• @.***/client-s3": "3.x",

Is there a reason to use 3.X here instead of ^3?

— Reply to this email directly, view it on GitHub https://github.com/infinite-industries/infinite/pull/388#pullrequestreview-1375599973, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE2NTQYHQIIOSS5DQNSKRTW74ZAPANCNFSM6AAAAAAWNXQIHE. You are receiving this because you authored the thread.Message ID: @.***>

[MatthewGidcomb]

That's fair. It would be nice to keep it consistent w/ the rest of this package.json file.

[jswank]

I'm not sure how the version definition should be made more consistent with other definitions in package.json.

The goal is to instruct NPM to use version 3. of the AWS SDK.

I did some more research on the caret notation used for other dependencies- include everything that does not increment the first non-zero portion of semver- and I think using it is more consistent (maybe this is more idiomatic, too?). So, rather than using @aws-sdk/client-s3": "3.x", I should use "@aws-sdk/client-s3": "^3.312".

I've updated the PR with that change. If this doesn't address the concern, please let me know what does!

[MatthewGidcomb]

That looks right -- ^3 will take newest major version 3, both minor and patch updates.

Looks good. Thanks for doing this.