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
rgw/s3: align output format with client request #47577
Conversation
133928e
to
9a9cb4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, only feedback is about naming
src/rgw/rgw_common.h
Outdated
HTML, | ||
}; | ||
|
||
static inline const char* toString(const RGWFormat f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to_string()
functions are a good way to associate names with the values of an enum class
in this case though, the strings we want are not just enum names but MIME types. could we call this function to_mime_type()
instead?
alternatively, we could rename the enum from RGWFormat
to RGWMimeType
so it's more obvious what to_string()
will give you - but RGWFormat
makes sense in that it controls which kind of ceph::Formatter
we construct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I thought about that after sending. I think it's better to rename the method.
Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
Also conditionally prints policy and resource in verify_bucket_permission. Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
This is a pure cleanup. The method to print an RGWFormat object as a MIME type is now called to_mime_type(). Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
In general, e.g., in the S3 ListBucket response, the output Formatter is already of the type requested in the HTTP Accept header. Therefore, while there are still many instances where the of Formatter is assumed to be XML, it appears necessarily safe and seemingly correct to extend this assumption to end_header(...), which sends the response content-type. Fixes: https://tracker.ceph.com/issues/55680 Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
9a9cb4c
to
7ad5386
Compare
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows