Skip to content

Allow exceptions to pass when cleaning up buckets #640

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dang
Copy link
Contributor

@dang dang commented Apr 8, 2025

We clean up buckets 3 times, using each client, so that all buckets can be removed, regardless of which client created it. However, the cleanup will throw an exception if permission is denied, due to using the wrong
user. This will stop cleanup, and cause all subsequent tests to ERROR.
Instead, pass these exceptions, allowing full cleanup.

We clean up buckets 3 times, using each client, so that all buckets can
be removed, regardless of which client created it.  However, the cleanup
will throw an exception if permission is denied, due to using the wrong
user.   This will stop cleanup, and cause all subsequent tests to ERROR.
Instead, pass these exceptions, allowing full cleanup.

Signed-off-by: Daniel Gryniewicz <[email protected]>
@dang dang requested a review from cbodley April 8, 2025 16:41
@cbodley
Copy link
Contributor

cbodley commented Apr 8, 2025

i guess i'm conflicted on this. usually, i only see cleanup permission errors where something in the test case failed, so didn't run the logic to restore permissions. i view this as a bug in the test case itself. #428 is an example where we added try/finally for unconditional cleanup

if the cleanup logic ignores all errors, we risk introducing new test cases that silently leave things behind. i think i'd prefer to leave those as failures

@dang
Copy link
Contributor Author

dang commented Apr 11, 2025

The test case I'm trying to fix is test_object_raw_get_x_amz_expires_not_expired_tenant. It creates a bucket with "public-read" and an object with "public-read" using the tenant_client. Then, teardown tries (first) do delete with the normal client, and fails, and aborts. I don't see how this test can ever pass.

@dang
Copy link
Contributor Author

dang commented Apr 11, 2025

The alternative would be to not ERROR test cases unless that test case left things behind. The issue is that all the rest of the test cases ERROR for the rest of the run.

@cbodley
Copy link
Contributor

cbodley commented Apr 14, 2025

The test case I'm trying to fix is test_object_raw_get_x_amz_expires_not_expired_tenant. It creates a bucket with "public-read" and an object with "public-read" using the tenant_client. Then, teardown tries (first) do delete with the normal client, and fails, and aborts. I don't see how this test can ever pass.

why does the normal client see the tenanted user's bucket? it uses client.list_buckets() which should only return buckets owned by that user

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.

2 participants