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

s3tests_boto3/functional/test_s3.py: add test for list object versions with key marker #594

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samakshd
Copy link

@samakshd samakshd commented Oct 9, 2024

The key-marker parameter in the list-object-versions operation is inclusive in RGW, while it's exclusive in S3. This commit adds the test to check the corresponding case.

Issue ref: https://tracker.ceph.com/issues/68055
Fix PR: ceph/ceph#60068

…s with key marker

The key-marker parameter in the list-object-versions operation is inclusive in RGW, while it's exclusive in S3. This commit adds the test to
check the corresponding case.

Issue ref: https://tracker.ceph.com/issues/68055
Fix PR: ceph/ceph#60068

Signed-off-by: Samaksh Dhingra <[email protected]>
Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

thanks, this looks good. but i also don't see any tests for list_object_versions() with VersionIdMarker. could you please write two versions of the same object name and verify that VersionIdMarker is also inclusive?

@@ -1450,6 +1450,22 @@ def test_bucket_list_return_data_versioning():
assert obj['VersionId'] == key_data['VersionId']
_compare_dates(obj['LastModified'],key_data['LastModified'])

@pytest.mark.fails_on_dbstore
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this is needed - there are other list_object_versions() tests that work with dbstore

@samakshd
Copy link
Author

samakshd commented Oct 10, 2024

thanks, this looks good. but i also don't see any tests for list_object_versions() with VersionIdMarker. could you please write two versions of the same object name and verify that VersionIdMarker is also inclusive?

@cbodley, I wanted to know can there also be scenario where just versionIdMarker is passed and keyMarker is not passed? If yes, what is the expected behaviour for the same? Or should we assume they are passed together only?

@cbodley
Copy link
Contributor

cbodley commented Oct 11, 2024

@samakshd not sure, but it probably doesn't do anything without KeyMarker

…s with key marker and version id marker, modified key marker test

Added test for list object versions with key marker and version id marker. Modified the previous list object versions with key marker test to make it more stronger.

Signed-off-by: Samaksh Dhingra <[email protected]>
@samakshd
Copy link
Author

samakshd commented Oct 11, 2024

Hi @cbodley, I have added the tests for list object version with key marker and version id marker. I have also modified key marker test to make it more stronger as I realized that my initial PR fix was not correct. I have added another commit to my existing fix PR.

However I accidentally did git rebase main to my branch, which introduced all new commits from other contributors, and it got a bit messed up. Since I am new to use git this much extensively, this happened. I reset my branch to first commit, by git hard reset, did fresh changes to it again and merged first with updated main branch and then pushed it. This however did not revert some review requests and tags that got into PR due to the commits that came because of rebasing.

Should I create another clean PR for the fix?

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