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

Add basic skeleton for aws_s3 testing towards a mocked S3 #133

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

onno-vos-dev
Copy link
Member

@onno-vos-dev onno-vos-dev commented Feb 23, 2023

aws_s3_util sits under /test for the time being as I'd like to get some mileage on it when we're ready to start the testing towards a real AWS instance. It's heavily based on a module we use at $DAYJOB so in theory it should work but don't want to commit to it being available to the outside world until it's been properly tested.

Furthermore I touched up the README a bit here and there and included a mention of aws_s3_presigned_url as it's additional functionality we provide that wasn't totally clear (got a question on Erlanger slack on whether that should be considered "legacy" and want to make it clear in the README that it is not 👍 😄

@onno-vos-dev onno-vos-dev force-pushed the skeleton-for-aws_s3-testing branch from 380927f to b56c0ac Compare February 23, 2023 10:58
@onno-vos-dev
Copy link
Member Author

Added regression tests for #134

@onno-vos-dev onno-vos-dev force-pushed the skeleton-for-aws_s3-testing branch from 5905fc1 to 655e086 Compare February 23, 2023 11:06
Copy link

@jadeallenx jadeallenx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mildly +1 on this because S3 has such a terrible API that nearly all developers will want some sugar on top of it to smooth out the rough bits and who wants to reinvent the wheel? Overall, though, I don't know if we should be providing sugar for other AWS APIs.

If we are going to start layering sugar over a set of AWS APIs, I think that should be a in a new separate project. We leave the underlying raw AWS APIs as-is and have a new top-level library that consumes this one to provide sugar. That's just my own personal aesthetic about how to separate concerns.

@onno-vos-dev
Copy link
Member Author

onno-vos-dev commented Feb 23, 2023

I'm mildly +1 on this because S3 has such a terrible API that nearly all developers will want some sugar on top of it to smooth out the rough bits and who wants to reinvent the wheel? Overall, though, I don't know if we should be providing sugar for other AWS APIs.

If we are going to start layering sugar over a set of AWS APIs, I think that should be a in a new separate project. We leave the underlying raw AWS APIs as-is and have a new top-level library that consumes this one to provide sugar. That's just my own personal aesthetic about how to separate concerns.

While I see your point and can agree to it, where should the tests live in that case? In the separate repo or here and this one pulls them in for testing? Each time a change is made to aws-erlang I'd like to run these types of tests. If we split these clients into separate repos it might become a pain to ensure that both this and the client(s) are tested.

How does the following sound?

  1. Move the aws_s3_util to a separate repo called aws_s3_client and we tag the version that is currently here as 1.0.0 (the API won't change). Only thing I'd like to add would be a multipart write but that can become a 1.1.0 once that's ready.
  2. We pull in aws_s3_client here for testing purposes and run aws_s3_SUITE. In the aws_s3_client repo we refer that tests are living in aws-erlang so they can be part of nightly builds and such and future actual AWS tests.
  3. Any additional clients, will follow the same recipe with a client living in a separate repo, with the tests living here.

Either way, I'd like to merge this PR as is and do the steps outlined above separately :-)

I'm personally not in favor in leaving aws-erlang with essentially zero test coverage as most tests are already moved out to aws_signature after we integrated with that.

@jadeallenx @andreashasse Your thoughts on that approach?

@jadeallenx
Copy link

I think what we have here is fine - I just think that if we add more sugar for other AWS services, that should be in its own repo

Copy link
Contributor

@philss philss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 cool!

I will try to copy this work for the aws-elixir in the (near) future.

@onno-vos-dev onno-vos-dev merged commit 84531e4 into master Feb 23, 2023
@onno-vos-dev onno-vos-dev deleted the skeleton-for-aws_s3-testing branch February 23, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants