Improve voltage warning lines in TinyPilot logs#1544
Conversation
Automated comment from CodeApprove ➜⏳ @jdeanwallace please review this Pull Request |
|
@cghague - Just a heads up that I'm reassigning these reviews to @jdeanwallace since I'm backed up this week. |
jdeanwallace
left a comment
jdeanwallace
left a comment
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
⏳ Approval Pending (3 unresolved comments)
Approval will be granted automatically when all comments are resolved
LGTM!
In: Discussion
Short arguments were used for consistency with the majority of the rest of the collect-debug-logs script.
I think this is generally the right choice, but in this case there is already a few other instances of using the prefered long arguments (1, 2, 3) that I think we should just go with using long arguments in this PR.
In: debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs:
> Line 142
if LAST_VOLTAGE_LINE=$(journalctl -q -r -n 1 -g "voltage") ; then
Can we quote the command substitution?
From Google Shell style guide:
# "quote command substitutions" # Note that quotes nested inside "$()" don't need escaping. flag="$(some_command and its args "$@" 'quoted separately')"
In: debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs:
> Line 145
printf "No"
This is something new I just confirmed with Michael, but can we use a lowercase "yes"/"no"?
👀 @cghague it's your turn please take a look
mtlynch
left a comment
mtlynch
left a comment
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
+1
If we have a defined convention, we usually want to use that one. If a file is internally consistent in violating our convention, and it would look weird to use our official convention (e.g., spaces in a file that consistently uses tabs), then we should be locally consistent.
But for cases where the file itself is inconsistent in adhering to a convention, we should go with our style guide.
I've filed #1547 to make collect-debug-logs consistent.
👀 @cghague, @jdeanwallace it's your turn please take a look
cghague
left a comment
cghague
left a comment
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs:
Resolved
In: debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs:
Resolved
jdeanwallace
left a comment
jdeanwallace
left a comment
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
1495-improve-voltage-warning-lines-in-tinypilot-logs
branch
3 participants
Resolves #1495
Parses the output of
journalctlinto a simple “yes” or “no” result. If the result is “yes”, the latest voltage warning is also included in the output.Parsing the output of
journalctlI initially considered piping the output of
journalctlintogrepfor readability, but getting the correct exit code as well as just the latest voltage warning was quite awkward with this approach. I therefore settled on using just the filtering and output options that are built intojournalctl:-qargument is used to suppress the unwantedjournalctlheader text.-rargument causesjournalctlto output from newest to oldest.-n 1argument causesjournalctlto stop after outputting a single line.-g "voltage"argument limits output to only lines that contain the word “voltage”.The end result of this combination of arguments is that
journalctlwill efficiently output only the most recent voltage warning. In addition, the exit status of the command reflects whether or not any matching lines were found, which allows for a simpleif <command>test to be used.Short arguments were used for consistency with the majority of the rest of the
collect-debug-logsscript.