Skip to content

Conversation

@href
Copy link
Contributor

@href href commented Dec 17, 2025

This adds an initial set of S3 tests.

⚠️ Only meant to be merged once Ceph Squid is deployed.

@href href requested a review from gaudenz December 17, 2025 14:00
Copy link
Contributor

@gaudenz gaudenz 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 a bit unsure about the list of fixtures. Do we really need the access_key and secret_key fixtures? Isn't the objects_user fixture good enough for this?

On the other hand an s3_client and sns_client fixture might be handy. I see that constructing the client once in the test to show how to do it is nice, but if we grow this test suite, I might become repetitive.

On the other hand even the object_user fixture is debatable because we probably also want to have tests covering objects user and key creation.

@href
Copy link
Contributor Author

href commented Dec 18, 2025

I'm a bit unsure about the list of fixtures. Do we really need the access_key and secret_key fixtures? Isn't the objects_user fixture good enough for this?

I personally err on the side of too many fixtures: They are cheap, simple, and easy to understand. If it makes the code in front a bit nicer to read, they should be included.

On the other hand an s3_client and sns_client fixture might be handy. I see that constructing the client once in the test to show how to do it is nice, but if we grow this test suite, I might become repetitive.

My thought exactly, but since I don't have additional tests yet, I think this would be premature.

On the other hand even the object_user fixture is debatable because we probably also want to have tests covering objects user and key creation.

For that the create_object_user fixture could be used.

@href href force-pushed the denis/sns branch 2 times, most recently from 34537f9 to 41fd24b Compare December 19, 2025 12:11
Copy link

@ubscale ubscale left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@urscale urscale left a comment

Choose a reason for hiding this comment

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

LGTM

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.

5 participants