-
-
Notifications
You must be signed in to change notification settings - Fork 442
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
etc: check for existing files during checks stage #731
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran shellcheck
on activate
😄
Pushed with ShellCheck suggestions. I am once again recalibrating my metric for how defensive I should be when programming in shell. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I suspect this would've been easier to review if the variable renaming was separated from the rearranging of code, but that's totally fair if that would've been way more work to split the commits that way
This ensures that activation fails early if there are any `/etc` files with unexpected state, rather than leaving the system half-activated.
Separated out variable renames to hopefully make the diff easier to read for future spelunkers. No change to the final tree. (Have to ask for re-approval, unfortunately.) |
@emilazy it's still approved FYI as you didn't make any overall changes |
Didn't realize it worked that way! Okay, merging this; fingers crossed. (We need VM tests...) |
This ensures that activation fails early if there are any
/etc
files with unexpected state, rather than leaving the system half-activated.Here be dragons, obviously; it would be quite bad to get anything wrong. I tried to write this as clearly as possible and I think it's a lot easier to follow than the previous version, but it should still be carefully reviewed. It worked for all the cases I tested. I endeavoured to follow the logic of the previous version as closely as possible, except for two cases, which should both be more conservative than the previous version:
/etc
file containing the string/etc/static
as carte blanche to overwrite it. (Why would we...? This one really confuses me. @LnL7 do you remember what's going on there?)/etc/static
only when the file in/etc/static
directly doesn't exist, not when recursively dereferencing symbolic links leads to a nonexistent file.