Skip to content

Conversation

@anoopcs9
Copy link
Collaborator

@anoopcs9 anoopcs9 commented Feb 5, 2025

We have at least 3 occurrences where code is duplicated for detecting the underlying Ceph version as a string. Instead, we could have it in common and invoke the corresponding wrapper whenever required.

@anoopcs9 anoopcs9 force-pushed the ceph-vers-detect-wrapper branch 3 times, most recently from 136e051 to c3f070b Compare February 5, 2025 10:07
@anoopcs9 anoopcs9 marked this pull request as ready for review February 5, 2025 10:36
@anoopcs9 anoopcs9 added the no-API This PR does not include any changes to the public API of a go-ceph package label Feb 5, 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.

nice cleanup :-)
I have a couple of small suggestions, nothing very major that you can choose to accept or decline. Let me know what you prefer and then I'll do a final approval.

Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
@anoopcs9 anoopcs9 force-pushed the ceph-vers-detect-wrapper branch from c3f070b to c3bcc8b Compare February 5, 2025 14:38
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, thanks!

@mergify mergify bot merged commit 2e15e72 into ceph:master Feb 5, 2025
16 checks passed
@anoopcs9 anoopcs9 deleted the ceph-vers-detect-wrapper branch May 9, 2025 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-API This PR does not include any changes to the public API of a go-ceph package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants