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

rgw/admin: parse more fields by bucket struct #1068

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

clwluvw
Copy link
Member

@clwluvw clwluvw commented Feb 1, 2025

The bucket's versioning information and ObjectLock status are now accessible by the Bucket struct.

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Added a new file that begins with //go:build ceph_preview
  • Ran make api-update to record new APIs

@clwluvw
Copy link
Member Author

clwluvw commented Feb 3, 2025

@anoopcs9 @phlogistonjohn Can you please take a look?

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

Seems OK on first glance, but I'm not too knowledgeable about RGW. What would fill me with confidence would be a unit test or two :-)

@clwluvw clwluvw force-pushed the bucket-stats branch 3 times, most recently from 52eaac5 to 0c4d6cc Compare February 3, 2025 17:40
@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Feb 3, 2025
@phlogistonjohn
Copy link
Collaborator

Thanks for adding a test! The general strucutre looks good but there's something about it failing on the older versions FWIW.

@clwluvw
Copy link
Member Author

clwluvw commented Feb 3, 2025

I tried to skip the tests for versions doesn't support reporting versioning and object lock with suite.T().Skip() but apparently go doesn't like them:

=== NAME  TestRadosGWTestSuite/TestBucket
    bucket_test.go:64: versioning is not reported in bucket stats
=== NAME  TestRadosGWTestSuite/TestBucket/enable_versioning
    testing.go:1577: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test

--- FAIL: TestRadosGWTestSuite/TestBucket/enable_versioning (0.00s)

Not sure what is the best way to skip them. @phlogistonjohn Do you have any idea?

@phlogistonjohn
Copy link
Collaborator

Would something along these lines work for your use case?

func skipIfQuincy(t *testing.T) {

@clwluvw
Copy link
Member Author

clwluvw commented Feb 3, 2025

@phlogistonjohn I was skipping the whole suite. Now it should be fixed. Can you take another look?

@phlogistonjohn
Copy link
Collaborator

@Mergifyio rebase

Copy link

mergify bot commented Feb 3, 2025

rebase

✅ Branch has been successfully rebased

phlogistonjohn
phlogistonjohn previously approved these changes Feb 3, 2025
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

Generally, lgtm but I want a 2nd pair of eyes on this because rgw is my weakest area in the codebase.

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

Can we have the subject for commit messages updated to include a / to read rgw/admin: xx?

Also are we in agreement to close #1058 once the current set of changes gets merged?

rgw/admin/bucket_test.go Outdated Show resolved Hide resolved
rgw/admin/bucket.go Outdated Show resolved Hide resolved
@anoopcs9 anoopcs9 changed the title rgwadmin: parse more fields by bucket struct rgw/admin: parse more fields by bucket struct Feb 4, 2025
@mergify mergify bot dismissed phlogistonjohn’s stale review February 4, 2025 06:37

Pull request has been modified.

@clwluvw
Copy link
Member Author

clwluvw commented Feb 4, 2025

@anoopcs9 Can you please take another look?

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@mergify mergify bot merged commit 1f461a1 into ceph:master Feb 4, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants