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

rados: implement rados_getaddrs #1037

Merged
merged 1 commit into from
Oct 23, 2024
Merged

rados: implement rados_getaddrs #1037

merged 1 commit into from
Oct 23, 2024

Conversation

gman0
Copy link
Contributor

@gman0 gman0 commented Oct 18, 2024

This PR implements binding for rados_getaddrs.

Includes a unit test.

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

@gman0 gman0 mentioned this pull request Oct 18, 2024
Closed
anoopcs9
anoopcs9 reviewed Oct 21, 2024
View reviewed changes
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.

See below for a small comment.

rados/rados_getaddrs_test.go Outdated Show resolved Hide resolved
rados/rados_getaddrs.go Outdated Show resolved Hide resolved
phlogistonjohn
phlogistonjohn requested changes Oct 21, 2024
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.

One request, and a pair of thoughts.

rados/rados_getaddrs.go Outdated Show resolved Hide resolved
rados/rados_getaddrs.go Show resolved Hide resolved
rados/rados_getaddrs_test.go Show resolved Hide resolved
@anoopcs9 anoopcs9 added the API This PR includes a change to the public API of a go-ceph package label Oct 21, 2024
@gman0
Copy link
Contributor Author

gman0 commented Oct 22, 2024

@phlogistonjohn I've fixed the comment. @anoopcs9 so I should remove the preview build tag? In the docs it says all new APIs must have it.

anoopcs9
anoopcs9 requested changes Oct 22, 2024
View reviewed changes
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.

@anoopcs9 so I should remove the preview build tag? In the docs it says all new APIs must have it.

You just have to drop the second variant for defining build constraint.

rados/rados_getaddrs.go Outdated Show resolved Hide resolved
rados/rados_getaddrs_test.go Outdated Show resolved Hide resolved
anoopcs9
anoopcs9 previously approved these changes Oct 22, 2024
View reviewed changes
phlogistonjohn
phlogistonjohn reviewed Oct 22, 2024
View reviewed changes
docs/api-status.json Outdated
},
{
"name": "Conn.GetAddrs",
"comment": "GetAddrs wraps the function to get the addresses of\nthe RADOS session, suitable for blocklisting.\n\nImplements:\n\n\tint rados_getaddrs(rados_t cluster, char **addrs)\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This generated JSON needs to be updated now that the code comment is changed. Try using git to revert the change to just this file and then re-run the make command to regenerate the JSON. (then squash it all back together :-) )

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you need more instruction on all that feel free to ask.

@anoopcs9
Copy link
Collaborator

There seems to be a newer version of protobuf(and protobuf-compiler) causing update issues with squid CI jobs. This is due to the fact that protobuf-compiler is only available with CRB repository which is not enabled by default with ceph container images. Let's see if we could get another rebuild of squid with the latest protobuf and protobuf-compiler in time.

@phlogistonjohn
Copy link
Collaborator

There seems to be a newer version of protobuf(and protobuf-compiler) causing update issues with squid CI jobs. This is due to the fact that protobuf-compiler is only available with CRB repository which is not enabled by default with ceph container images. Let's see if we could get another rebuild of squid with the latest protobuf and protobuf-compiler in time.

It's not a required check so we have the option of trying to fix it and then asking @gman0 to rebase or just ignoring the failure for this PR. I will look into fixing it now... but if it proves tricky we can skip it.

@phlogistonjohn
Copy link
Collaborator

squid workaround in #1038

@mergify mergify bot dismissed anoopcs9’s stale review October 23, 2024 08:15

Pull request has been modified.

anoopcs9
anoopcs9 approved these changes Oct 23, 2024
View reviewed changes
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.

@anoopcs9
Copy link
Collaborator

@Mergifyio rebase

@anoopcs9
rados: implement rados_getaddrs
5c95766 
This commit implements binding for rados_getaddrs.

Includes a unit test.

Signed-off-by: Robert Vasek <[email protected]>
@mergify Mergify
Copy link

mergify bot commented Oct 23, 2024

rebase

✅ Branch has been successfully rebased

phlogistonjohn
phlogistonjohn approved these changes Oct 23, 2024
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.

lgtm

@mergify mergify bot merged commit da1e313 into ceph:master Oct 23, 2024
16 checks passed
@gman0 gman0 deleted the rados-getaddrs branch October 24, 2024 12:09
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

@anoopcs9 anoopcs9 anoopcs9 approved these changes

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
@gman0 @anoopcs9 @phlogistonjohn