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

test: REST multipart upload listing, completion #912

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

lrm25
Copy link
Contributor

@lrm25 lrm25 commented Oct 24, 2024

No description provided.

@lrm25 lrm25 force-pushed the test_cmdline_list_parts_three branch 2 times, most recently from d57f10a to 11a5e3f Compare October 24, 2024 21:08

# params: client, bucket name
# return 0 for success, 1 for error
delete_bucket_or_contents() {

Choose a reason for hiding this comment

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

I'm a little confused as to this method - why isn't it two methods? One for deleting only bucket contents and one for deleting the bucket itself? Considering that it is mainly just a wrapper with an variable check it just seems weird to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just to combine operations for static and non-static bucket cleanup. For both, occasionally you need to delete individual objects due to WORM protection. And just to avoid putting the static check in each test. If you think there's a better way to do it though, let me know.

Choose a reason for hiding this comment

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

Could maybe just use different naming to be clearer what is going on - bucket_cleanup? bucket_cleanup_worm_compliant? And then add a comment to each section explaining why there's different operations for static/non-static.


# params: client, bucket name
# return 0 for success, 1 for error
delete_bucket_or_contents_if_exists() {

Choose a reason for hiding this comment

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

Same question here? Seems like the 'or' clause belongs a level higher in the code and not in a method name.

@lrm25 lrm25 force-pushed the test_cmdline_list_parts_three branch from 11a5e3f to f9b2cf4 Compare October 30, 2024 23:42
@lrm25 lrm25 force-pushed the test_cmdline_list_parts_three branch from f9b2cf4 to bf4fc71 Compare October 31, 2024 00:11
Copy link

@anodelman anodelman left a comment

Choose a reason for hiding this comment

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

Easier for me to read, thanks.

@benmcclelland benmcclelland merged commit ab517e6 into main Oct 31, 2024
20 checks passed
@benmcclelland benmcclelland deleted the test_cmdline_list_parts_three branch October 31, 2024 17:48
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.

3 participants