Don't unmount entire plugin manager tree on remove

[cpuguy83]

This was mistakenly unmounting everything under plugins/* instead of
just plugins/<id>/* anytime a plugin is removed.

ping @fermayo @caervs

[thaJeztah]

LGTM

can you add a cherry pick label and add it to the tracking issue for cherry picking?

[vdemeester]

LGTM 👼

[mghazizadeh]

How's this tested?

[thaJeztah]

@mghazizadeh could you open an issue for that instead? Comments on closed/merged pull request can easily get overlooked

[cpuguy83]

I'm not sure a test is warranted in this case. This was a blatant logic mistake akin to a typo we just didn't pick up on it during code review.

[mghazizadeh]

But it's not a typo. It's a logic change to recursively unmount up to localDir instead of all the way up to root, therefore a test to satisfy the new logic is warranted despite the fact that it should be quite obvious.

[cpuguy83]

@mghazizadeh The fact that we are unmounting anything at all here is a very low level detail particular to the current implementation and is not part of the package interface. Adding a test for this is fragile and will most likely end up either breaking or effectively testing nothing due to implementation changes.
The logic itself didn't really change here we just managed to grab the wrong variable name.

When adding tests we need to be careful about testing the right things. Adding a test to prevent someone from changing this back to the incorrect variable does not seem like the right course.

It might be nice to add a new integration test suite for these targeted regressions which may only run nightly rather than on every PR.