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

cephfs admin: deprecate the New function #849

Merged
merged 2 commits into from
Mar 27, 2023

Conversation

phlogistonjohn
Copy link
Collaborator

Fixes: #816

First commit fixes the version numbers in api-status from our recent API additions. I did this before touching the api-status.* to deprecate the New function.

Second commit deprecates the New function. The New function created a rados connection object but did not expose it via a public api, making it impossible to deterministically sever / clean up the connection. This commit assumes that the patch will be merged prior to the go-ceph v0.21 release so that we can plan to remove the function three versions later (v0.24). I chose three versions so that it is one release longer than our typical stabilization period as this function has been around for a while.

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

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Mar 20, 2023
@phlogistonjohn
Copy link
Collaborator Author

I labeled this an API change because it technically does create a planned change to the API. :-)

anoopcs9
anoopcs9 previously approved these changes Mar 21, 2023
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 anoopcs9 requested a review from ansiwen March 21, 2023 05:06
@mergify
Copy link

mergify bot commented Mar 27, 2023

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

ansiwen
ansiwen previously approved these changes Mar 27, 2023
Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: John Mulligan <jmulligan@redhat.com>
The New function created a rados connection object but did not expose it
via a public api, making it impossible to deterministically sever / clean up
the connection. This commit assumes that the patch will be merged prior
to the go-ceph v0.21 release so that we can plan to remove the function
three versions later (v0.24).  I chose three versions so that it is one
release longer than our typical stabilization period as this function
has been around for a while.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn phlogistonjohn force-pushed the jjm-deprecated-cephfs-admin-new branch from d9d758e to 738ab3e Compare March 27, 2023 14:25
@mergify mergify bot dismissed stale reviews from anoopcs9 and ansiwen March 27, 2023 14:26

Pull request has been modified.

@phlogistonjohn
Copy link
Collaborator Author

Rebased and pushed to resolve conflicts. Note that I did not fix versions for APIs that were added after I originally made these patches, I chose to do that because I thought it would be easier to re-review, but it does mean we'll need to fix the versions again at least once prior to release.

@anoopcs9 @ansiwen - please re-review.

@mergify mergify bot merged commit ad61049 into ceph:master Mar 27, 2023
@phlogistonjohn phlogistonjohn deleted the jjm-deprecated-cephfs-admin-new branch March 31, 2023 13:13
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.

cephfs.admin.New is missing Shutdown
3 participants