Skip to content

Conversation

@nixpanic
Copy link
Member

Image.RemoveSnapByID removes an existing snapshot. This can be used to
manually remove a snapshot from the trash.

Closes: #1183

@nixpanic nixpanic changed the title rbd snap remove by id rbd: add rbd_snap_remove_by_id Oct 10, 2025
@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Oct 10, 2025
@mergify
Copy link

mergify bot commented Oct 13, 2025

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

@nixpanic
Copy link
Member Author

Thanks @anoopcs9 and @phlogistonjohn! Fixed the test-case and solved the merge conflicts in the api-status.

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.

Probably replace librbd:rbd_snap_remove_by_id in the commit message (Add librbd:rbd_snap_remove_by_id to the API status) to RemoveSnapByID to reflect the new go-ceph API than the underlying C API?

Comment on lines +9 to +10
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, for this test case, we only need either assert or require.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see using both as an issue: assert sets the test as failed but keeps going; require stops the test then and there if it fails. both have their uses. but I'm not objecting to simplifying it either.

phlogistonjohn
phlogistonjohn previously approved these changes Oct 13, 2025
Comment on lines +9 to +10
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see using both as an issue: assert sets the test as failed but keeps going; require stops the test then and there if it fails. both have their uses. but I'm not objecting to simplifying it either.

@nixpanic nixpanic force-pushed the rbd_snap_remove_by_id branch from 5fbce6b to 3235909 Compare November 4, 2025 12:31
@nixpanic
Copy link
Member Author

nixpanic commented Nov 4, 2025

Probably replace librbd:rbd_snap_remove_by_id in the commit message (Add librbd:rbd_snap_remove_by_id to the API status) to RemoveSnapByID to reflect the new go-ceph API than the underlying C API?

@anoopcs9, I've updated the commit message. Please have a look again, thanks!

@nixpanic nixpanic requested a review from anoopcs9 November 4, 2025 12:32
@mergify mergify bot dismissed phlogistonjohn’s stale review November 4, 2025 12:32

Pull request has been modified.

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

@phlogistonjohn
Copy link
Collaborator

@Mergifyio rebase

Image.RemoveSnapByID removes an existing snapshot. This can be used to
manually remove a snapshot from the trash.

Signed-off-by: Niels de Vos <ndevos@ibm.com>
The librbd:rbd_snap_remove_by_id function is implemented by
Image.RemoveSnapByID inside the rbd package.

Signed-off-by: Niels de Vos <ndevos@ibm.com>
@mergify
Copy link

mergify bot commented Nov 4, 2025

rebase

✅ Branch has been successfully rebased

@mergify mergify bot added the queued label Nov 5, 2025
@mergify mergify bot merged commit 35db829 into ceph:master Nov 5, 2025
15 of 16 checks passed
@mergify mergify bot removed the queued label Nov 5, 2025
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.

Missing rbd API components: function rbd_snap_remove_by_id

4 participants