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/admin: Add support for /info section #936

Merged
merged 3 commits into from
Nov 8, 2023
Merged

rgw/admin: Add support for /info section #936

merged 3 commits into from
Nov 8, 2023

Conversation

anoopcs9
Copy link
Collaborator

@anoopcs9 anoopcs9 commented Oct 26, 2023 •
edited
Loading

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

based on #616

@anoopcs9 anoopcs9 changed the title Rgw getinfo api rgw/admin: Add add support for /info section Oct 26, 2023
@anoopcs9 anoopcs9 changed the title rgw/admin: Add add support for /info section rgw/admin: Add support for /info section Oct 26, 2023
@anoopcs9 anoopcs9 added the API This PR includes a change to the public API of a go-ceph package label Oct 26, 2023
@phlogistonjohn
Copy link
Collaborator

You're probably already aware of this since you also restarted some of the builds but some of the jobs are failing with a somewhat mysterious error, possibly a failure to download go modules. I restarted it again but if it fails again I may just suggest waiting a day or more in case this is an issue on the go module proxy server side.

@anoopcs9 anoopcs9 closed this Oct 30, 2023
@anoopcs9 anoopcs9 reopened this Oct 30, 2023
thotz
thotz previously approved these changes Oct 30, 2023
View reviewed changes
@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Nov 2, 2023

You're probably already aware of this since you also restarted some of the builds but some of the jobs are failing with a somewhat mysterious error, possibly a failure to download go modules. I restarted it again but if it fails again I may just suggest waiting a day or more in case this is an issue on the go module proxy server side.

May be info=read capability is not treated correctly before quincy.

@mergify mergify bot dismissed thotz’s stale review November 2, 2023 18:16

Pull request has been modified.

@anoopcs9 anoopcs9 force-pushed the rgw-getinfo-api branch 2 times, most recently from ea73894 to c667110 Compare November 2, 2023 18:53
@anoopcs9 anoopcs9 requested a review from thotz November 2, 2023 18:53
thotz
thotz previously approved these changes Nov 3, 2023
View reviewed changes
phlogistonjohn
phlogistonjohn reviewed Nov 3, 2023
View reviewed changes
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.

Good find on the setup script issue. I am a bit confused by the addition of launch_rgw which is basically exactly the same as launch_radosgw. Why not either rename launch_radosgw to launch_rgw or keep launch_ragdosgw and have the new function named launch_radosgw2 ?

@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Nov 3, 2023

Good find on the setup script issue. I am a bit confused by the addition of launch_rgw which is basically exactly the same as launch_radosgw. Why not either rename launch_radosgw to launch_rgw or keep launch_ragdosgw and have the new function named launch_radosgw2 ?

That's a very good suggestion.

@mergify mergify bot dismissed thotz’s stale review November 3, 2023 14:50

Pull request has been modified.

@phlogistonjohn
Copy link
Collaborator

Sorry for noticing things bit-by-bit, but I see that there's no Signed-off-by of yours @anoopcs9 . The co-authored bits looks good but it only has Matt's name in the sign off line.

@phlogistonjohn
Copy link
Collaborator

^ That applies to the first 2 commits only

@phlogistonjohn
Copy link
Collaborator

To be clear: I would add one for your own. I would leave any existing sign offs

@anoopcs9 @leseb
rgw/admin: Add support for new /info section
4e9e496 
Implement support for new /info AdminOps interface, which returns
a json record similar to:

{
  "info": {
    "storage_backends": [
      {
        "name":"rados",
        "cluster_id":"204a1415-0b03-4926-bf76-6aebd192a52d"
      }
    ]
  }
}

Co-authored-by: Anoop C S <[email protected]>
Co-authored-by: Sébastien Han <[email protected]>
Signed-off-by: Matt Benjamin <[email protected]>
Signed-off-by: Anoop C S <[email protected]>
@anoopcs9
rgw/admin: Add a test for API.GetInfo
140d04f 
Co-authored-by: Anoop C S <[email protected]>
Signed-off-by: Sébastien Han <[email protected]>
Signed-off-by: Anoop C S <[email protected]>
@anoopcs9
docs: Add rgw API.GetInfo to api-status
862f2c3 
Signed-off-by: Anoop C S <[email protected]>
@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Nov 6, 2023

To be clear: I would add one for your own. I would leave any existing sign offs

Updated individual commit messages with Sign Offs.

phlogistonjohn
phlogistonjohn approved these changes Nov 6, 2023
View reviewed changes
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.

While my RGW knowledge is shaky everything seems OK to me particularly WRT to i-dotting and t-crossing for go-ceph processes ;-)

@mergify mergify bot merged commit 3d86572 into ceph:master Nov 8, 2023
15 checks passed
@anoopcs9 anoopcs9 deleted the rgw-getinfo-api branch December 12, 2023 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phlogistonjohn phlogistonjohn phlogistonjohn approved these changes

@thotz thotz thotz approved these changes

@mattbenjamin mattbenjamin Awaiting requested review from mattbenjamin

@leseb leseb Awaiting requested review from leseb

Assignees
No one assigned
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anoopcs9 @phlogistonjohn @thotz