WiFi dialog: add script for printing marker section contents #1811
Conversation
|
CircleCI seems to be having some erratic issues related to git connectivity… I try running them again later. |
Automated comment from CodeApprove ➜⏳ @jdeanwallace please review this Pull Request |
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!
In: debian-pkg/opt/tinypilot-privileged/scripts/print-marker-sections:
> Line 42
echo "Illegal option: $1" >&2
Can we conform to saying "Unknown flag" like in the bash example from our style guide?
In: debian-pkg/opt/tinypilot-privileged/scripts/print-marker-sections:
> Line 42
echo "Illegal option: $1" >&2
Also can we direct users to use --help flag?
Same throughout.
In: debian-pkg/opt/tinypilot-privileged/scripts/print-marker-sections:
> Line 55
echo 'Input parameter missing: TARGET_FILE' >&2
Can we begin the line by redirecting the output to stderr?
Same throughout.
In: debian-pkg/opt/tinypilot-privileged/scripts/print-marker-sections.bats:
Perhaps it's useful, but I recently added echo "${output}" to some our BATS tests to help with debugging.
Essentially, we'll only see the script output if the test fails. Useful for debugging.
👀 @jotaen4tinypilot 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/print-marker-sections:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/print-marker-sections:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/print-marker-sections.bats:
Ah, good to know. I think I would leave this out for now, though, and use it on demand.
By the way, in case you didn’t know, another helpful bats debugging tip is to use the &3 descriptor for printing out things to the terminal.
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.
Related #131, part (c). Stacked on #1811. This PR adds the backend logic and endpoints that we need for the WiFi dialog. - There are endpoints for reading, writing, and deleting the WiFi configuration. There is also one endpoint for checking the status of the network connectivity, which allows us to display warnings or status indicators in the frontend. - As [mentioned in the code comment](https://github.com/tiny-pilot/tinypilot/pull/1812/files#diff-43d8494a595e7d1f838bc01d5f2c6b18eebf61ca63bc3bbf2837dbcb27a8e109R62-R63), we cannot read the `wpa_supplicant.conf` file directly due to file ownership. Therefore, we use the [new `print-marker-sections` privileged script](#1811) from the previous PR. - The scripts for enabling and disabling are executed in a “fire and forget” manner, otherwise it may happen that the frontend requests hang due to e.g. a change/interruption in the network (which would result in an eternal loading spinner). As we only write a config file and trigger service/daemon restarts, we wouldn’t get direct feedback about errors anyway, but we’d have to poll for the resulting status, which can take quite long, though. - Note that we also can’t really verify the WiFi credentials, because the desired WiFi might not be reachable at the time where the user wants to enter the settings (e.g., because they want to enter the config ahead of time). - The validation of the incoming request data is relatively important, because we will show the error messages in the frontend for input validation. So we need to make sure that we detect all relevant potential issues. The validation errors would be user-facing. This PR can probably be tested best via the [subsequent frontend PR](#1813) – that one isn’t 100% code-complete yet, but it’s pretty close in terms of functionality. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1812"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <jan@jotaen.net>
Related #131, part (b). Stacked on #1804.
I realized that we need to add another privileged script for reading files with marker sections, because we are unable to read those directly via the Python process in case those files are owned by the
rootuser.Usage is demonstrated in the subsequent backend PR; it’s basically …
… where the entire invocation has to be unblocked in the sudoers file.