Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve voltage warning lines in TinyPilot logs #1544

Merged

Conversation

cghague
Copy link
Contributor

@cghague cghague commented Aug 2, 2023

Resolves #1495

Parses the output of journalctl into 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 journalctl

I initially considered piping the output of journalctl into grep for 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 into journalctl:

  • The -q argument is used to suppress the unwanted journalctl header text.
  • The -r argument causes journalctl to output from newest to oldest.
  • The -n 1 argument causes journalctl to stop after outputting a single line.
  • The -g "voltage" argument limits output to only lines that contain the word “voltage”.

The end result of this combination of arguments is that journalctl will 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 simple if <command> test to be used.

Short arguments were used for consistency with the majority of the rest of the collect-debug-logs script.
Review on CodeApprove

@cghague cghague requested a review from mtlynch August 2, 2023 14:32
@mtlynch mtlynch requested review from jdeanwallace and removed request for mtlynch August 2, 2023 14:33
Copy link
Collaborator

mtlynch commented Aug 2, 2023

Automated comment from CodeApprove ➜

@jdeanwallace please review this Pull Request

@mtlynch
Copy link
Collaborator

mtlynch commented Aug 2, 2023

@cghague - Just a heads up that I'm reassigning these reviews to @jdeanwallace since I'm backed up this week.

Copy link
Contributor

@jdeanwallace jdeanwallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@mtlynch mtlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cghague cghague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jdeanwallace jdeanwallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.

@jdeanwallace jdeanwallace dismissed their stale review August 9, 2023 23:55

Review approved on CodeApprove

@cghague cghague merged commit f9ab492 into master Aug 9, 2023
12 checks passed
@cghague cghague deleted the 1495-improve-voltage-warning-lines-in-tinypilot-logs branch August 9, 2023 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve voltage warning lines in TinyPilot logs
3 participants