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.

Sign up for GitHub

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

Jump to bottom

rgw: filter prefix should be used when generating expiration header #30584

Merged
merged 1 commit into from Sep 3, 2020
Merged

rgw: filter prefix should be used when generating expiration header #30584

merged 1 commit into from Sep 3, 2020

Conversation

zhangsw
Copy link
Contributor

@zhangsw zhangsw commented Sep 26, 2019 •
edited

@zhangsw
Copy link
Contributor Author

zhangsw commented Oct 8, 2019

@cbodley pls help review this，thanks~

theanalyst
theanalyst reviewed Dec 5, 2019
View reviewed changes
src/rgw/rgw_lc.cc Outdated
auto& filter = rule.get_filter();
auto& prefix = filter.has_prefix()?filter.get_prefix():rule.get_prefix();
Copy link
Member

Choose a reason for hiding this comment

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

nit: spacing

@cbodley
Copy link
Contributor

cbodley commented Dec 5, 2019

@zhangsw can you please add a s3test case for this?

@zhangsw zhangsw mentioned this pull request Dec 17, 2019
Open
@zhangsw
Copy link
Contributor Author

zhangsw commented Dec 17, 2019

Test case: ceph/s3-tests#334

mattbenjamin
mattbenjamin approved these changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment •
edited

Choose a reason for hiding this comment

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

I think the change is correct. I think the commit message should state, in this case, more completely what the change does. i.e., it would be helpful to mention the distinction between rule prefix and filter prefix.

@stale
Copy link

stale bot commented Feb 15, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Feb 15, 2020
@stale stale bot removed the stale label Apr 20, 2020
@zhangsw
Copy link
Contributor Author

zhangsw commented Apr 20, 2020

@mattbenjamin I've updated the commit message.

mattbenjamin
mattbenjamin approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

lgtm

@mattbenjamin
Copy link
Contributor

mattbenjamin commented May 7, 2020 •
edited

HI @zhangsw,

  1. I tested this PR by hand, and I find it works great!
  2. I tested the s3-tests case in Check prefix works well when set lifecycle configuration with filter. s3-tests#334 , and it doesn't quite work, because the test assertion uses dict.has_key(), which isn't in python3; suggested fix in the PR

tl/dr: casey would like the s3-test fix before merging this, update when you can

regards,
Matt

@mattbenjamin mattbenjamin self-assigned this Jun 23, 2020
@zhangsw
rgw: filter prefix should be used when generating expiration header. …
25dd43a 
…The rule prefix is set with old version lifecycle configure.

While filter prefix is the latest version which AWS suggests users use.

Fixes: https://tracker.ceph.com/issues/45144

Signed-off-by: zhang Shaowen <zhangshaowen_yewu@cmss.chinamobile.com>
@ivancich ivancich added the wip-eric-testing-2 for ivancich testing label Aug 18, 2020
@ivancich
Copy link
Member

ivancich commented Sep 3, 2020

Teuthology results -- no regressions noted:

@ivancich ivancich merged commit 29836e6 into ceph:master Sep 3, 2020
@ivancich
Copy link
Member

ivancich commented Sep 3, 2020

@zhangsw @theanalyst @mathslinux No backports are listed in the tracker. If backports appropriate please update the tracker.

@ivancich ivancich removed needs-qa wip-eric-testing-2 for ivancich testing labels Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattbenjamin mattbenjamin mattbenjamin approved these changes

@theanalyst theanalyst theanalyst approved these changes

Assignees

@mattbenjamin mattbenjamin

Labels
rgw
Projects
None yet
Milestone
No milestone
7 participants
@zhangsw @cbodley @mattbenjamin @ivancich @theanalyst @dang @batrick