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

Swift: Add "bulk-delete" URL argument support #1930

Merged
merged 1 commit into from
Apr 19, 2020

Conversation

@coveralls
Copy link

coveralls commented Apr 10, 2020

Coverage Status

Coverage increased (+0.06%) to 78.681% when pulling b0d9caa on kayrus:swift-bulk-delete into 65804fe on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 10, 2020

Build succeeded.

@kayrus
Copy link
Contributor Author

kayrus commented Apr 10, 2020

@jtopjian ready for review

@jtopjian
Copy link
Contributor

We should be able to easily add an acceptance test for this, right? Let me know if you need help.

@kayrus
Copy link
Contributor Author

kayrus commented Apr 11, 2020

@jtopjian just noticed that bulk-delete is also valid for containers. It would be great if you help me with acceptance tests.

@kayrus kayrus force-pushed the swift-bulk-delete branch 3 times, most recently from 06c6a46 to 07cf4b8 Compare April 11, 2020 00:05
@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 11, 2020

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 11, 2020

Build succeeded.

@kayrus
Copy link
Contributor Author

kayrus commented Apr 11, 2020

According to https://github.com/openstack/swift/blob/a495f1e327fac17246a296721fb1eda8470e4773/swift/common/middleware/bulk.py#L156..L194, bulk delete should have a Content-Type: text/plain header and contain a body with filenames separated by a newline.

@kayrus kayrus changed the title Swift: Add "bulk-delete" URL argument support [WIP] Swift: Add "bulk-delete" URL argument support Apr 11, 2020
@jtopjian
Copy link
Contributor

jtopjian commented Apr 11, 2020

bulk delete should have a Content-Type: text/plain header and contain a body with filenames separated by a newline.

Right. Now I recall that I looked into this in the past and silently backed away 🙂

This means that we'll need to modify UpdateOpts and DeleteOpts for both containers and objects to include an option for a body, and then pass that body in the actual Update and Delete calls.

In addition, an actual structured response body is sent back, which differs from a normal Swift response where the bulk of the info is in the headers: https://docs.openstack.org/ocata/user-guide/cli-swift-bulk-delete.html

In my opinion, a better approach might be to make entirely new functions/actions called BulkDelete with their own BulkDeleteOpts. I believe Swift treats POST and DELETE equally, so let's just standardize on using POST (including a body with a DELETE call feels wrong).

BulkDeleteOpts can have a Containers or Objects field of type []string where the user can specify the list of containers/objects to be bulk deleted. in the To<Container/Object>BulkDeleteQuery method, we can return a plaintext string of []string joined by a newline, so that it can be passed to the client.Post method.

Finally, I think a 409 response (I think this means that only some of the requested containers/objects were deleted) might be a little common and we should either consider treating it as valid response code... but I'm not 100% sure on this, yet.

Thoughts?

@kayrus
Copy link
Contributor Author

kayrus commented Apr 12, 2020

@jtopjian no concerns, though at first glance it looked like a low hanging fruit.

@kayrus kayrus changed the title [WIP] Swift: Add "bulk-delete" URL argument support Swift: Add "bulk-delete" URL argument support Apr 17, 2020
@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 17, 2020

Build succeeded.

@kayrus
Copy link
Contributor Author

kayrus commented Apr 17, 2020

@jtopjian I reworked the PR. I also discovered and fixed redundant pointers usage and unencoded container/object names.

Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

@jtopjian jtopjian merged commit e547035 into gophercloud:master Apr 19, 2020
@kayrus kayrus deleted the swift-bulk-delete branch April 19, 2020 06:45
@kayrus
Copy link
Contributor Author

kayrus commented Apr 19, 2020

@jtopjian can you add a release note that container and object names are now urlencoded?

@jtopjian
Copy link
Contributor

Sure - but we should try to have things like this in separate PRs.

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.

object storage bulk delete
3 participants