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:add "ListBucketsWithStat" function #801

Merged
merged 2 commits into from
Jan 24, 2023

Conversation

TheMoti
Copy link
Contributor

@TheMoti TheMoti commented Dec 24, 2022

Added new function ListBucketsWithStat to get all buckets of all users info (for admin keys only).

fixes #800

@TheMoti
Copy link
Contributor Author

TheMoti commented Jan 1, 2023

Hi @thotz,
Would you mind reviewing this PR? or if you're busy right now can you give an estimation on when it might get reviewed?
Thank you

@thotz
Copy link
Collaborator

thotz commented Jan 2, 2023

Hey @TheMoti The change looks good to me, In go-ceph bindings for adding new apis, there are certain format need to be followed. @phlogistonjohn @anoopcs9 can guide through that

@anoopcs9 anoopcs9 added the API This PR includes a change to the public API of a go-ceph package label Jan 5, 2023
@TheMoti
Copy link
Contributor Author

TheMoti commented Jan 8, 2023

Hey @TheMoti The change looks good to me, In go-ceph bindings for adding new apis, there are certain format need to be followed. @phlogistonjohn @anoopcs9 can guide through that

Hi again,
I ran make fmt but no changes appeared. I'm not sure what should I change. Can you mention the problems? @thotz @anoopcs9

Many thanks

@phlogistonjohn
Copy link
Collaborator

I think @thotz meant make api-update - more details here and here.
It looks like you did that in the last commit of the series and the formatting of the doc comment itself looks OK.

I would suggest that for the scope of these changes are small and tightly related the commits be squashed together in one commit. While you do that it would be good to follow our format for commit messages: [topic]: [description] (and then details on later lines are optional). For your change I would suggest:

rgw: add ListBucketsWithStat function

Add an rgw api function that combines listing and stat'ing buckets.

If you're not familiar with the process of squashing patches with git, feel free to ask and we'll try to assist.

@TheMoti TheMoti force-pushed the rgw/get-all-buckets-with-stats branch from bbfe6dc to 1978fdc Compare January 8, 2023 16:26
@TheMoti
Copy link
Contributor Author

TheMoti commented Jan 8, 2023

I think @thotz meant make api-update - more details here and here. It looks like you did that in the last commit of the series and the formatting of the doc comment itself looks OK.

I would suggest that for the scope of these changes are small and tightly related the commits be squashed together in one commit. While you do that it would be good to follow our format for commit messages: [topic]: [description] (and then details on later lines are optional). For your change I would suggest:

rgw: add ListBucketsWithStat function

Add an rgw api function that combines listing and stat'ing buckets.

If you're not familiar with the process of squashing patches with git, feel free to ask and we'll try to assist.

Done!
I'm not sure about the branch name format though. Is it ok?

@anoopcs9
Copy link
Collaborator

Done! I'm not sure about the branch name format though. Is it ok?

Branch name should not matter much but there is something I forgot to comment earlier(and I am sorry for the delay). New APIs in go-ceph are supposed to be tagged ceph_preview(in go build tags style) in a separate source file. Similarly corresponding test would also move to new file with ceph_preview build tag. For more details refer API Status in the Developers Guide and the API Stability Plan.

Thus you can have 2 commits, one which adds the API and another to deal with the API stability process by including the changes(to api-status.json and api-status.md) made as a result of make api-update.

@anoopcs9 anoopcs9 changed the title rgw:add"ListBucketsWithStat"function rgw:add "ListBucketsWithStat"function Jan 11, 2023
@anoopcs9 anoopcs9 changed the title rgw:add "ListBucketsWithStat"function rgw:add "ListBucketsWithStat" function Jan 11, 2023
@TheMoti TheMoti force-pushed the rgw/get-all-buckets-with-stats branch from 053e9da to d24789c Compare January 15, 2023 15:34
@TheMoti
Copy link
Contributor Author

TheMoti commented Jan 15, 2023

@anoopcs9 Can you check new changes?

//go:build ceph_preview
// +build ceph_preview

package admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since all new apis need to be added as separate file, I recommend filename should be similar to API like list_bucket_with.go than admin_bucket.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@TheMoti TheMoti force-pushed the rgw/get-all-buckets-with-stats branch from d24789c to 8c58124 Compare January 16, 2023 18:36
@anoopcs9 anoopcs9 requested a review from thotz January 17, 2023 06:08
@TheMoti TheMoti force-pushed the rgw/get-all-buckets-with-stats branch 2 times, most recently from 381b3c7 to 56004cc Compare January 17, 2023 14:10
thotz
thotz previously approved these changes Jan 17, 2023
@TheMoti TheMoti force-pushed the rgw/get-all-buckets-with-stats branch from 56004cc to 174ea01 Compare January 17, 2023 14:13
@mergify mergify bot dismissed thotz’s stale review January 17, 2023 14:14

Pull request has been modified.

@TheMoti TheMoti requested review from anoopcs9 and thotz and removed request for anoopcs9 and thotz January 17, 2023 14:14
@anoopcs9
Copy link
Collaborator

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Jan 18, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link

mergify bot commented Jan 18, 2023

rebase

✅ Branch has been successfully rebased

@leseb leseb force-pushed the rgw/get-all-buckets-with-stats branch from 174ea01 to fd7d17a Compare January 18, 2023 07:13
@anoopcs9
Copy link
Collaborator

@TheMoti Looks good in general.

Just a small nit: We prefer to have Signed-off-by: line included within each commit message. Can you please add that and push again? Sorry for dragging it out further 😕

@anoopcs9
Copy link
Collaborator

warning This pull request got rebased on behalf of a random user of the organization. This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@phlogistonjohn Mergify needs some attention 😬

@TheMoti TheMoti force-pushed the rgw/get-all-buckets-with-stats branch from fd7d17a to 110bb79 Compare January 18, 2023 11:40
@TheMoti TheMoti requested review from thotz and anoopcs9 and removed request for anoopcs9 and thotz January 18, 2023 11:44
docs/api-status.md Outdated Show resolved Hide resolved
@phlogistonjohn
Copy link
Collaborator

Mergify needs some attention grimacing

Indeed. It'd be nice if they provided a link to some documentation.

I'll do it for them: https://blog.mergify.com/selecting-your-bot-account/

@TheMoti TheMoti force-pushed the rgw/get-all-buckets-with-stats branch from 110bb79 to c0d9603 Compare January 20, 2023 05:51
@TheMoti TheMoti requested a review from anoopcs9 January 20, 2023 21:27
@anoopcs9
Copy link
Collaborator

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Jan 22, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link

mergify bot commented Jan 22, 2023

rebase

✅ Branch has been successfully rebased

@guits guits force-pushed the rgw/get-all-buckets-with-stats branch from c0d9603 to bea513b Compare January 22, 2023 05:59
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.

Thank you for bearing with us in fixing all nitpicks.

lgtm.

@anoopcs9 anoopcs9 requested a review from thotz January 22, 2023 06:20
@TheMoti
Copy link
Contributor Author

TheMoti commented Jan 24, 2023

@thotz

Add an rgw api function that combines listing and stat'ing buckets.

Signed-off-by: moti <motaharesdq@gmail.com>
Add api stability.

Signed-off-by: moti <motaharesdq@gmail.com>
@mergify
Copy link

mergify bot commented Jan 24, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@guits guits force-pushed the rgw/get-all-buckets-with-stats branch from bea513b to 3c09fe0 Compare January 24, 2023 13:20
@mergify mergify bot merged commit 3c45e67 into ceph:master Jan 24, 2023
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.

rgw/get all buckets with stats
4 participants