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

tests/storage-vm: Test that we avoid deleting existing target directories not created by LXD when unmounting disks #83

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tests/storage-vm: Test that we avoid deleting existing target directories not created by LXD when unmounting disks #83

wants to merge 1 commit into from
+33 −0

Conversation

gabrielmougard
Copy link
Contributor

Related to canonical/lxd#12700

@gabrielmougard gabrielmougard force-pushed the fix/rm-vm-target-dir branch 3 times, most recently from c6a03e1 to b97d789 Compare February 27, 2024 07:34
@gabrielmougard gabrielmougard marked this pull request as ready for review February 27, 2024 07:36
@gabrielmougard gabrielmougard mentioned this pull request Feb 27, 2024
Open
simondeziel
simondeziel requested changes Feb 27, 2024
View reviewed changes
Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

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

I don't see the difference between the test with /opt and /tmp/empty-dir as both are not created by LXD.

tests/storage-vm Outdated Show resolved Hide resolved
tests/storage-vm Outdated Show resolved Hide resolved
tests/storage-vm Outdated Show resolved Hide resolved
tests/storage-vm Outdated Show resolved Hide resolved
tests/storage-vm Outdated Show resolved Hide resolved
@gabrielmougard
tests/storage-vm: Test that we avoid deleting existing target directo…
cad636e 
…ries not created by LXD when unmounting disks

Signed-off-by: Gabriel Mougard <[email protected]>
@gabrielmougard
Copy link
Contributor Author

I don't see the difference between the test with /opt and /tmp/empty-dir as both are not created by LXD.

you're right, these two cases test the same thing, removing the second one.

@simondeziel
Copy link
Member

@gabrielmougard @tomponline I wonder how much of that whole test (not just the bits added by Gabriel) also apply to container. Should we consider creating a storage-ctn variant of the whole script?

@tomponline
Copy link
Member

@gabrielmougard @tomponline I wonder how much of that whole test (not just the bits added by Gabriel) also apply to container. Should we consider creating a storage-ctn variant of the whole script?

Possibly, although i'd expect to see containers tested in the main test suite so that the general logic and container specific implementation is tested on every PR.

@simondeziel
Copy link
Member

We should wait for canonical/lxd#12700 to be merged so that we can trigger a run in here before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simondeziel simondeziel simondeziel approved these changes

@tomponline tomponline Awaiting requested review from tomponline tomponline is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gabrielmougard @simondeziel @tomponline