Pipe ka-clone's logging to stdout and stderr depending on level so other scripts can parse non-fatal errors #21
Conversation
lillialexis
changed the title
lillialexis
changed the title
There was a problem hiding this comment.
I read the commit message but don't really understand it. What are you actually trying to accomplish with this PR? I get the impression it has something to do with being clearer about stdout vs stderr, but you're not changing any existing logging, only adding new logging, so it's all very unclear to me.
| @@ -446,6 +458,10 @@ def link_gitconfig_khan(): | |||
|
|
|||
| tmpl = _expanded_home_path('.gitconfig.khan') | |||
| if not os.path.isfile(tmpl): | |||
| # Send to stderr | |||
| logging.error( | |||
| '** The git config file {} is not installed'.format(tmpl) | |||
There was a problem hiding this comment.
I'm not sure I understand this PR. Won't it print "The git config file is not installed" twice, once due to this line and once due to line 466?
There was a problem hiding this comment.
Oh, yeah, you're right. 🤦♀️
It doesn't print twice if you call ka-clone through the OLC module, as the module uses inherit for stdout/stdin, when calling ka-clone, and pipe for stderr. That means that it is able to dump the stdout-stream to the console as it runs, but capture the stderr separately to be able to display to the user after the command finishes (when stderr not empty). Here is the PR that complements this one and does that: https://github.com/Khan/our-lovely-cli/pull/893
There's actually an even worse issue with this PR and PR 893, that hit me as I was drifting off to sleep last night. As it currently stands, there is a sort-of race condition, where a user could be on post-893-OLC and pre-21-ka-clone. In that rare-but-possible case, because the default behavior of Python's logging is to output to stderr, then it will appear to the user that ka-clone hit issues no matter what. It looks like this:
✔ Let's go! … <press enter>
🐱 Whoops! Looks like something wasn't quite right with ka-clone! The script ran
but sent something to stderr:
-> Set user.email to [email protected]
-> Linking commit message template ~/.git_template/commit_template...
*** The commit template is already linked in the
current repo, skipping...
-> Including git config ~/.gitconfig.khan...
*** The git config is already included in current repo, skipping...
-> Backing up existing hooks...
*** Backed up existing hooks to .git/hooks.backup_2025-01-09T09:18:47
*** More info: https://khanacademy.org/r/gitfaq#id-3c30
-> Created new hooks directory at .git/hooks
-> Added pre-push linting hook
-> Added hooks to protect master branch
-> Added commit-msg branch-name hook
You can also scroll up and read the output to see what went wrong.
Please report this issue in the #github-prs channel so we can fix it for
everyone!
That's the normal output of ka-clone.
When I originally used a version flag to make sure ka-clone was on the correct version for OLC, I could have just bumped that from 1 to 2 in this PR and PR 893 to close this gap. But then we switched to using presence-of-command-line-args instead.
But, I think that actually ends up working out better, given this feedback! Maybe I can just add a new arg that OLC has and checks for (closing the race-condition-gap) that only outputs to stderr when true (fixing the double-output-bug).
There was a problem hiding this comment.
If your goal is to have ka-clone emit output that can be parsed by another script, you should just support this directly. Have a --json flag or something, that emits the results of ka-clone as a json blob, to stdout, and have OLC parse that blob and do what it needs to do. And when --json is set, you can change the way that _cli_log_step_indented_info behaves, maybe it saves its data to a buffer somewhere, and you can format and emit that buffer at program exit.
There was a problem hiding this comment.
While I agree that that's the most "right" way of doing things, I think that it isn't worth the time it will take to implement. I'm trying to limit the amount of new code, as I need to wrap this project up. But I also want a quick and easy way to flag to the user that something isn't quite right, as the warnings from the ka-clone output get lost. And the only thing I'm doing with the output is piping it to the user; I'm not parsing it any more than just seeing if something is in stderr versus stdout. (The OLC tools do this with the git output too.)
Summary:
If ka-clone hits an issue that isn't worth killing the process over, log it to stderr, otherwise log the rest to stdout. Needed to redirect python's logging from stderr to stdout
ssue: FEI-6095
Test plan: