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.

Sign up for GitHub

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

Jump to bottom

Convert print statements to logging statements #1893

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+17 −11
Open

Convert print statements to logging statements #1893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be22037
Convert print statements to logging statements
invisiblethreat Feb 29, 2024
cd7fa2e
Use caplog fixture
invisiblethreat Mar 1, 2024
44d9c83
Ran pre-commit hooks
invisiblethreat Mar 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/snowflake/connector/auth/webbrowser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,17 +166,17 @@ def prepare(
)
return

print(
logger.info(
"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...")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @sfc-gh-mkeller

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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 😄

logger.info(f"Going to open: {sso_url} to authenticate...")
if not self._webbrowser.open_new(sso_url):
print(
logger.warning(
"We 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."
Expand Down
22 changes: 14 additions & 8 deletions test/unit/test_auth_webbrowser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from __future__ import annotations

import base64
import logging
import socket
from unittest import mock
from unittest.mock import MagicMock, Mock, PropertyMock, patch
Expand Down Expand Up @@ -248,9 +249,11 @@ def test_auth_webbrowser_post(_, disable_console_login):
)
@patch("secrets.token_bytes", return_value=PROOF_KEY)
def test_auth_webbrowser_fail_webbrowser(
_, capsys, input_text, expected_error, disable_console_login
_, caplog, input_text, expected_error, disable_console_login
):
"""Authentication by WebBrowser with failed to start web browser case."""

caplog.set_level(logging.INFO)
rest = _init_rest(
REF_SSO_URL, REF_PROOF_KEY, disable_console_login=disable_console_login
)
Expand Down Expand Up @@ -279,15 +282,15 @@ def test_auth_webbrowser_fail_webbrowser(
user=USER,
password=PASSWORD,
)
captured = capsys.readouterr()
assert captured.out == (
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
Comment on lines +285 to +292
Copy link
Collaborator

Choose a reason for hiding this comment

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

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"


if expected_error:
assert rest._connection.errorhandler.called # an error
assert auth.assertion_content is None
Expand All @@ -303,8 +306,11 @@ def test_auth_webbrowser_fail_webbrowser(

@pytest.mark.parametrize("disable_console_login", [True, False])
@patch("secrets.token_bytes", return_value=PROOF_KEY)
def test_auth_webbrowser_fail_webserver(_, capsys, disable_console_login):
def test_auth_webbrowser_fail_webserver(_, caplog, disable_console_login):
"""Authentication by WebBrowser with failed to start web browser case."""

caplog.set_level(logging.INFO)

rest = _init_rest(
REF_SSO_URL, REF_PROOF_KEY, disable_console_login=disable_console_login
)
Expand Down Expand Up @@ -338,13 +344,13 @@ def test_auth_webbrowser_fail_webserver(_, capsys, disable_console_login):
user=USER,
password=PASSWORD,
)
captured = capsys.readouterr()
assert captured.out == (
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...\n"
)
) in caplog.text

assert rest._connection.errorhandler.called # an error
assert auth.assertion_content is None

Expand Down
Loading