Add scripts to manage static IP addresses #1256
Conversation
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
Cool, this is great! Lots of improvements over the hosted version.
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 6
set -e
Can we also set -u once we've finished parsing all the flags? Ditto in the unset script.
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 14
--router router_ip: The IP address of the local router.
Can we give examples for all the flags, like:
The IP address of the local router (e.g., 10.0.0.1)
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 20
Multiple DNS servers can be provided. If unspecified, defaults to the router's
Can we group this with the --dns flag documentation above?
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 21
DNS, then Google (8.8.8.8), then Cloudflare (1.1.1.1).
Can we give full examples that show how to use the different flags? (one using the minimum number of flags, one using all the flags)
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 25
# Use the same values as the previous script for compatibility.
I'd like to cut this. If a reader sees this and doesn't know about the hosted script, they won't understand this. But they don't really need to know because once this script exists, it will have to be compatible with previous runs of the script, so we'll know not to just switch the markers.
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 27
readonly MARKER_START='# --- AUTOGENERATED BY TINYPILOT - START ---'
I'd like to avoid duplicating these in different files. Especially here where we have to keep them in-sync between set and unset scripts.
Can we put these in ../lib/markers.sh so that we can share them across scripts? And then change-hostname can share it too.
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 44
echo "Only one static IP address should be provided." >&2
Can we name the flag specifically?
(ditto for routers and interfaces)
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 83
if [[ -z "${IP_ADDRESS}" ]] || [[ -z "${ROUTERS}" ]] ; then
Can we print a more specific error message to stderr?
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 106
echo "Please remove the existing configuration and try again." >&2
Can we tell them the path to the config file so they know where they have to make the fix?
In: debian-pkg/opt/tinypilot-privileged/scripts/unset-static-ip:
> Line 6
# set-static-ip script. It is intentionally excluded from the help text.
Let's keep things simple and document the flag normally in the usage output. We would only leave it undocumented if we wanted to avoid having to support it for users who want it for other reasons, but the cost of supporting this flag is so low and it's fairly unlikely that users will script against this flag and expect us to maintain it.
In: debian-pkg/opt/tinypilot-privileged/scripts/unset-static-ip:
> Line 86
echo "Unclosed marker section." >&2
Can we make this a bit more actionable so that we're guiding the user toward what they need to do to fix it?
👀 @cghague it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 6
set -e
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 141
echo "Please remove the existing configuration and try again." >&2
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/unset-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/unset-static-ip:
Resolved
👀 @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: debian-pkg/opt/tinypilot-privileged/scripts/lib/markers.sh:
> Line 2
# shellcheck disable=SC2034
I agree with the suppression but can we add a comment explaining why to save the reader from having to look up the shellcheck code?
In: debian-pkg/opt/tinypilot-privileged/scripts/lib/markers.sh:
> Line 6
readonly MARKER_START='# --- AUTOGENERATED BY TINYPILOT - START ---'
Can we use these in change-hostname as well?
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 62
echo "Only one static IP address should be provided using --ip." >&2
Can we leave off the trailing period? I personally often copy/paste the flag from the command-line, so having a trailing period makes the copy a little tougher.
(ditto for other places where we have a period right after a flag)
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
> Line 64
echo " set-static-ip --help" >&2
Can we refactor these two lines into a function so we don't have to duplicate them?
In: debian-pkg/opt/tinypilot-privileged/scripts/unset-static-ip:
> Line 33
source "${SCRIPT_DIR}"/lib/markers.sh
Can we quote the full path? Your way works but our convention is to quote the full path if we need to quote part of it.
👀 @cghague it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: debian-pkg/opt/tinypilot-privileged/scripts/lib/markers.sh:
> Line 2
# shellcheck disable=SC2034
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/lib/markers.sh:
> Line 8
readonly MARKER_START='# --- AUTOGENERATED BY TINYPILOT - START ---'
Resolved. I thought this was out of scope and so was going to raise a separate change for it.
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/unset-static-ip:
Resolved
👀 @mtlynch it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
⏳ Approval Pending (2 unresolved comments)
Approval will be granted automatically when all comments are resolved
LGTM, thanks!
In: debian-pkg/opt/tinypilot-privileged/scripts/change-hostname:
> Line 13
source "${SCRIPT_DIR}/lib/markers.sh"
Can we use . instead of source? I think source is more readable, but . is more consistent with the rest of the repo.
In: debian-pkg/opt/tinypilot-privileged/scripts/lib/markers.sh:
> Line 4
# disable the unused variable check for this file.
Our convention is to either put the comment on the same line or before the shellcheck directive.
👀 @cghague it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: debian-pkg/opt/tinypilot-privileged/scripts/change-hostname:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/lib/markers.sh:
> Line 3
# disable the unused variable check for this file.
Resolved
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
Approved: I have approved this change on CodeApprove and all of my comments have been resolved.
cghague
deleted the
1244-add-scripts-for-setting-and-unsetting-a-static-ip-address
branch
Resolves #1244 by adding scripts to manage static IP addresses.
