-
-
Notifications
You must be signed in to change notification settings - Fork 194
doc: Start documenting Heads logging and configuration variables #1888
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
base: master
Are you sure you want to change the base?
doc: Start documenting Heads logging and configuration variables #1888
Conversation
@tlaurion Thinking on this again, I think this is mergeable if you agree with the logging conventions I described. Or if not, please comment and let's reach a consensus 🙂 I didn't document every variable yet, but docs for some variables are better than docs for none. We can add them over time. Thought about this again because I came across some "warnings" that should not be warnings:
That's not a warning, we are doing exactly what is expected. This is informational and should be INFO. (
Same, in this context "boot default option" is exactly what the user expects us to do. This message is informational since rebooting to achieve this might be surprising, it should be INFO. |
@JonathonHall-Purism the two examples you bring here is where the crux of the issue is. Turning those warn into INFO would make them silent to the unexperienced user:
This leads to redefining what a warning is. Otherwise I love what was written here and agree with this pr content. A warning would:
Thoughts? |
OK, I see your point here. These two messages are important because the behavior is not clear without them. The GPG prompts are confusing, so we need to try to clear that up with this message. The reboot is normal and accomplishes what the user asks, but could be surprising. So However, I don't think I think this is a justifiable example for a level in between Let me try putting together a change for this. BTW, I think it is a huge documentation win if documenting our behavior exposes something that's confusing, and we fix it 💪 |
Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
Add NOTE level between INFO and warn. NOTE is appropriate for context that is required to understand confusing/surprising behavior in Heads, but that does not indicate a problem. Clarify warn level based on discussion. Fix LOG output in tables, LOG actually does go to the debug log currently in any configuration. Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
These messages don't indicate a problem, and they are not actionable. They aren't warnings. They _are_ important enough to clarify a confusing behavior that they should not be hidden with INFO messages, though. Use NOTE. Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
9b9ebaa
to
e1d58a3
Compare
# doc/logging.md. | ||
NOTE() { | ||
echo "NOTE:" "$@" | tee -a /tmp/debug.log | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JonathonHall-Purism I love eveyrhing I see here, but the change to warn to note removing the 2 seconds that was added by warn. Not a big blocker, but sometimes, the note happening before a clear, a reboot, or a binary input might just pass as silence.
Your call, but its also part of why warn was used (the sleep) permitting user to acknoeldge in absence of output and action on the console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warn -> note : no 2 seconds sleep on note?
Ping @JonathonHall-Purism |
Start documenting some design aspects of Heads to aid future PR discussions.
Logging has changed quite a few times, and it tends to be difficult to review because we don't have established expectations for how logging behaves. Logging guidelines are helpful when adding features to keep logging consistent.
We have a lot more config variables now than in the past. Only some of them are intended to be configured by the user in config.user, and it's not always clear which those are.
The logging documentation is fairly complete if we all agree on it 🙂 Comments welcome, let's establish a common expectation.
The config documentation is not complete, there are many more variables not documented. Help appreciated 🙂 If we agree on the structure, we could merge and do this incrementally. Some doc is better than none, it doesn't have to be blocked waiting for "perfect" docs IMO.
I've documented the updated logging behavior in #1875 since we agree on the changes in that PR.