Convert print statements to logging statements #1893
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
sfc-gh-aalam
added
the
NO-CHANGELOG-UPDATES
|
@invisiblethreat could you fix the unit tests too and use |
Head branch was pushed to by a user without write access
invisiblethreat
force-pushed
the
move-sso-print-to-logging
branch
from
6e83fdd
to
cd7fa2e
Compare
|
@sfc-gh-aalam Had some issues getting tests to run as expected locally, but built out some toy scripts to assess the functionality of |
| assert ( | ||
| "Initiating login request with your identity provider. A browser window " | ||
| "should have opened for you to complete the login. If you can't see it, " | ||
| "check existing browser windows, or your OS settings. Press CTRL+C to " | ||
| f"abort and try again...\nGoing to open: {REF_SSO_URL if disable_console_login else REF_CONSOLE_LOGIN_SSO_URL} to authenticate...\nWe were unable to open a browser window for " | ||
| "you, please open the url above manually then paste the URL you " | ||
| "are redirected to into the terminal.\n" | ||
| ) | ||
| ) in caplog.text |
There was a problem hiding this comment.
you would have to break this into two assert statements because logger is adding extra text in between. same for the other test
assert "Initiating login request with your identity provider. A browser window should have opened for you to
complete the login. If you can't see it, check existing browser windows, or your OS settings. Press CTRL+C to abort
and try again...\nGoing to open: https://testsso.snowflake.net/sso to authenticate...\n" in "INFO
snowflake.connector.auth.webbrowser:webbrowser.py:169 Initiating login request with your identity provider. A
browser window should have opened for you to complete the login. If you can't see it, check existing browser
windows, or your OS settings. Press CTRL+C to abort and try again...\nINFO
snowflake.connector.auth.webbrowser:webbrowser.py:177 Going to open: https://testsso.snowflake.net/sso to
authenticate...\n"
| "Initiating login request with your identity provider. A " | ||
| "browser window should have opened for you to complete the " | ||
| "login. If you can't see it, check existing browser windows, " | ||
| "or your OS settings. Press CTRL+C to abort and try again..." | ||
| ) | ||
|
|
||
| logger.debug("step 2: open a browser") | ||
| print(f"Going to open: {sso_url} to authenticate...") |
There was a problem hiding this comment.
We print it to stdout so when the program failed to open a browser, CLI users could manually open the link in browser, not sure if logging would work in this case.
There was a problem hiding this comment.
This is at least a breaking change and should go through announcement. But I'm not too sure whether we'd want to change this in the first place. I think the opening message we wouldn't want to get rid off in the default case.
How about introducing a setting that switches to logging instead of printing, but we'd leave the default setting on printing? @invisiblethreat would that work for you?
There was a problem hiding this comment.
Yes, that would work. I'd actually propose that log is the default and a more verbose mode is available for troubleshooting, or this particular use case. Defaulting a library to STDOUT feels like the wrong thing to do, but I'm not privy to all the use cases 😄
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1178890: When using
externalbrowserprinting to STDOUT breaks pipelining #1892Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
The use of
printbreaks piping outputs, as the output is contaminated with the message when usingexteranlbrowswerauthentication. Moving these messages into logging allows for pipelining, and also elevating the the failure mode to a more suitable level of urgency.