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

S3 tests #3713

Merged
merged 5 commits into from Aug 21, 2023
Merged

S3 tests #3713

merged 5 commits into from Aug 21, 2023

Conversation

Jamstah
Copy link
Collaborator

@Jamstah Jamstah commented Aug 12, 2022

The s3 tests were failing for numerous reasons, so this PR fixes them.

Long term it would be better to mock the s3 SDK so that everyone could run them, and to run AWS connected tests in the CICD pipeline.

@Jamstah
Copy link
Collaborator Author

Jamstah commented Nov 7, 2022

Rebased. For whatever reason, it seems I'm now considered a first time contributor again, so can I please have an OK to test?

@milosgajdos
Copy link
Member

Long term it would be better to mock the s3 SDK so that everyone could run them, and to run AWS connected tests in the CICD pipeline.

Mocking sounds good and I'm all for it but it merely covers unit tests - we need those indeed. I'm more keen on using some S3-compatible API we could stand up during the tests. Something like minio or some such - maybe localstack (alas this is slow).

@milosgajdos
Copy link
Member

@Jamstah could you rebase, please

Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
If we haven't set a storage class there's no point in checking the
storage class applied to the object - s3 will choose one.

Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
This test will only work on an s3 bucket on an s3 outpost. Most
developers won't have access to one of these.

Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
Other storage drivers will only return children and below, s3 should do
the same. The only reason it was returning was because of the addition
of a / to ensure we treat the from as a directory.

Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
@Jamstah
Copy link
Collaborator Author

Jamstah commented Aug 15, 2023

Rebased

@davidspek
Copy link
Collaborator

I've looked at the code quickly but want to do so more in depth tomorrow before I add my review. Just wanted to let you know I am busy with it.

@Jamstah
Copy link
Collaborator Author

Jamstah commented Aug 15, 2023

Thanks David.

A quick test run was ok for me but there have been quite a few changes since I wrote it. Be harsh, I don't take offense easily :)

Copy link
Collaborator

@davidspek davidspek left a comment

Choose a reason for hiding this comment

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

LGTM

@milosgajdos milosgajdos merged commit 59dd684 into distribution:main Aug 21, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants