linode / longview

Linode Longview Agent
Other
334 stars 45 forks source link

Only remove longview files from /etc/linode #30

Closed RobertDeRose closed 8 years ago

RobertDeRose commented 8 years ago

@dwfreed This look better to you?

Since we don't include the config files in the package we can't use the package manager to handle the config files. Also, longview.key should not be saved since if you remove and then reinstall longview you would probably need a new key.

dwfreed commented 8 years ago

Eh... not really. See my argument in the other thread about keeping the files for the user. Also, rm -rf is a great way to accidentally delete the user's entire system.

RobertDeRose commented 8 years ago

@dwfreed Well, generally, I feel that if a package creates files, it should remove them when it's removed. It doesn't really make sense to me that we leave behind are garbage when we leave. It's like going to a camp ground and taking your liter with you when you leave. Lets not liter people's filesystem with things they probably didn't even know we made.

@jamesottinger and @displague What's your opinion?

dwfreed commented 8 years ago

There's a difference between files the package can definitively say it owns, and config files which may or may not have been modified by the user, and are thus now owned by the user, not the package. I'd prefer to err on the side of user ownership rather than package ownership.

RobertDeRose commented 8 years ago

@dwfreed well /etc/linode/longview.d is owned by root, because the package script generates the files there, and for longview.key file hanging around after the package is uninstalled could cause issues if the customer decides they wish to reinstall longview, but now have a new key, which was the main focus of this PR. It's a small chance, and at the end of the day, it would only affect people that chose to install longview manually, maybe because they don't trust piping shell scripts into their VMs, which is wise, but it could cause issues that might be frustrating for a customer to figure out. I think the pros outweigh the cons here, especially refocusing it to just removing longview related files.

dwfreed commented 8 years ago

If they're smart enough to realize that piping web pages to a root shell is not a great idea, they're smart enough to realize that their Longview key has probably changed, and that they need to update the file to use the new key. (Or they're paranoid for no particular reason, in which case they aren't likely to run Longview anyway...) Either way, your glob is still too general; remove specifically the files that Longview has created, and then rmdir longview.d.

RobertDeRose commented 8 years ago

@dwfreed @jamesottinger I would just vote to roll this change back then and just close this PR. It doesn't even seem worth it in the end. I was supposed to handle a small edge case, but it seems there is enough opinions that it's just not worth it.

@jamesottinger could you rollback the last merge?