gaurav singh has posted comments on this change. ( http://gerrit.cloudera.org:8080/22424 )
Change subject: IMPALA-13675: OAuth AuthN Support for Impala Shell ...................................................................... Patch Set 18: (10 comments) http://gerrit.cloudera.org:8080/#/c/22424/16/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/22424/16/shell/impala_shell.py@1010 PS16, Line 1010: : > Nit: add : at the end to match line 998 Done http://gerrit.cloudera.org:8080/#/c/22424/16/shell/impala_shell.py@1028 PS16, Line 1028: print("Error getting OAuth access tokens", e) > Lines 1027 and 1028 don't provide much extra value. I think it would be be Done http://gerrit.cloudera.org:8080/#/c/22424/16/shell/impala_shell.py@1030 PS16, Line 1030: finally: > This line will print the error and continue execution. It should re-throw Added sys.exit(1) with the exception info. That should be enough to debug/diagnose the error than continuing with further execution. http://gerrit.cloudera.org:8080/#/c/22424/16/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/22424/16/shell/option_parser.py@373 PS16, Line 373: # This is primarily for testing purposes. > The hidden options which we're suppressing let's at least put the help text Done http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py File tests/custom_cluster/test_shell_jwt_auth.py: http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@26 PS16, Line 26: > flake8 requires having 2 blank lines between imports and the class definiti Done http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@197 PS16, Line 197: rror" in res > These args set the coordinator jwt/jwks flags, not the OAuth flags. Should Changed to OAUTH_IMPALAD_ARGS http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@199 PS16, Line 199: assert "Not connected to Impala, could not execute queries." in result.stderr > These need to be their own log dirs so they do not conflict with the jwt te changed to oauth_auth_success. http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@207 PS16, Line 207: size=1) > Why specify the '--user' flag? Shouldn't the user be read out of the oauth Yes. However, this is just a place holder and its not verified since we do not have a live oauth server. http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@219 PS16, Line 219: -q' > Should this be OAuth? Done http://gerrit.cloudera.org:8080/#/c/22424/16/tests/custom_cluster/test_shell_jwt_auth.py@242 PS16, Line 242: impala_log_dir="{oauth_auth_success}", > This test does not test OAuth success, it tests failure and does not run a Updated. -- To view, visit http://gerrit.cloudera.org:8080/22424 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84e26d54f6a53696660728efb239ffd43de4c55d Gerrit-Change-Number: 22424 Gerrit-PatchSet: 18 Gerrit-Owner: gaurav singh <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Mon, 07 Apr 2025 21:54:34 +0000 Gerrit-HasComments: Yes
