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

fix: explicitly allow remove root when performing library removal #11352

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

gnattu
Copy link
Member

@gnattu gnattu commented Apr 13, 2024

Changes

This PR added a workaround to temporarily mark the UserRoot as non-root during library validation for VirtualFolder removal. This is probably not the best solution but I'd like to see any suggestions.

The RootFolder protection is to not accidentally remove the whole library when the path is temporarily unavailable, and is a valid use case. But when the user explicitly asked for removal, we should not protect it. So perhaps IsRoot can be renamed to something like DeleteProtect?

Issues

Fixes #11269

Fixes #11269

Signed-off-by: gnattu <gnattuoc@me.com>
@gnattu gnattu requested review from cvium and a team April 13, 2024 06:53
Signed-off-by: gnattu <gnattuoc@me.com>
Signed-off-by: gnattu <gnattuoc@me.com>
@crobibero
Copy link
Member

I can confirm that this fixes the issue, but I'm not completely sold on the solution

@gnattu
Copy link
Member Author

gnattu commented Apr 15, 2024

I can confirm that this fixes the issue, but I'm not completely sold on the solution

I don't like doing it this way either, any better approach?

This is an alternate approach which is more proper, but changes all parts that uses/overrides the original ValidateChildren method

Signed-off-by: gnattu <gnattuoc@me.com>
@gnattu
Copy link
Member Author

gnattu commented Apr 17, 2024

I can confirm that this fixes the issue, but I'm not completely sold on the solution

I pushed an alternate approach, which looks more proper but it changes the signature of ValidateChildren methods so all places calling and overriding those methods are also modified. Does this looks better?

@EraYaN
Copy link
Member

EraYaN commented Apr 17, 2024

Seems like it's neater and I think, personally, the signature changing is fine.

@gnattu gnattu changed the title fix: mark UserRoot as non-root when performing removal fix: explicitly allow remove root when performing library removal Apr 17, 2024
Signed-off-by: gnattu <gnattuoc@me.com>
@cvium
Copy link
Member

cvium commented Apr 18, 2024

My original fix is a complete hack anyway

@Bond-009 Bond-009 merged commit 37d301e into master Apr 18, 2024
14 checks passed
@Bond-009 Bond-009 deleted the fix-library-removal branch April 18, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue]: Unable to reliably remove Libraries
6 participants