tiny-pilot / tinypilot

Use your Raspberry Pi as a browser-based KVM.
https://tinypilotkvm.com
MIT License
2.89k stars 244 forks source link

Delegate to unified strip-marker-sections script #1722

Closed jotaen4tinypilot closed 5 months ago

jotaen4tinypilot commented 5 months ago

Resolves https://github.com/tiny-pilot/tinypilot/issues/1710. Stacked on https://github.com/tiny-pilot/tinypilot/pull/1720.

Based on the simplifications in unset-static-ip, this PR removes the code redundancies in the unset-static-ip and change-hostname privileged scripts, and calls the new, unified strip-marker-sections script instead.

Notes

Review on CodeApprove

mtlynch commented 5 months ago

I couldn’t figure out why we have been using tee previously to write to the config file, because we discarded tee’s stdout output by redirecting to /dev/null. I’m also not sure why we used sudo for invoking tee – we already execute the script with root privileges. It’s a bit odd, because I cannot imagine we did that without good reason 🤔

I think we wrote that assuming the whole script wouldn't necessarily run with root privileges.

But the pattern of sudo tee is one we use in a few places. It's because if we do:

sudo some-cmd > /etc/dhcpcd.conf

Then the command fails if /etc/dhcpcd.conf is owned by root and is not world writable. But if we do:

sudo some-cmd | sudo tee /etc/dhcpcd.conf > /dev/null

It lets us write to /etc/dhcpcd.conf from a root user context.

jotaen4tinypilot commented 5 months ago
Automated comment from CodeApprove ➜

⏳ @jdeanwallace please review this Pull Request

jotaen4tinypilot commented 5 months ago

I think we wrote that assuming the whole script wouldn't necessarily run with root privileges.

Ah, thanks for the explanation, that makes sense then. Do we need to preserve this behavior (in the strip-marker-sections script), or shall we keep the currently proposed > redirect-to-file? For our own purposes (e.g., when calling the scripts from Python), the > works fine.

mtlynch commented 5 months ago

Ah, thanks for the explanation, that makes sense then. Do we need to preserve this behavior (in the strip-marker-sections script), or shall we keep the currently proposed > redirect-to-file? For our own purposes (e.g., when calling the scripts from Python), the > works fine.

No, we can leave those as-is. We only need to do the tee workaround when we're running in a non-root context to begin with so we have to sudo to write to the file.