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 19: (5 comments) http://gerrit.cloudera.org:8080/#/c/22424/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22424/18//COMMIT_MSG@22 PS18, Line 22: custom_clu > Nit: these are custom cluster tests, not unit tests Updated. Thanks. http://gerrit.cloudera.org:8080/#/c/22424/18/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/22424/18/shell/impala_shell.py@222 PS18, Line 222: self.oauth_mock_response_cmd = options.oauth_mock_response_cmd > duplicate of line 219 Removed. Thanks. 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@207 PS16, Line 207: size=1) > In that case, is the "--user" flag needed at all? Since its the mock response, we technically do not need any of the params since we get the mock response doesn't validate any of the inputs. So here, we are constructing a mock request similar to a real request. http://gerrit.cloudera.org:8080/#/c/22424/18/tests/custom_cluster/test_shell_jwt_auth.py File tests/custom_cluster/test_shell_jwt_auth.py: http://gerrit.cloudera.org:8080/#/c/22424/18/tests/custom_cluster/test_shell_jwt_auth.py@243 PS18, Line 243: tmp_dir_placeholders=["oauth_auth_failure"], > Placeholders should be renamed to something like oauth_auth_failure Done http://gerrit.cloudera.org:8080/#/c/22424/18/tests/custom_cluster/test_shell_jwt_auth.py@261 PS18, Line 261: self._stop_impala_cluster() > This test is not asserting anything. It needs to check the failure message We are asserting that the impala_shell_cmd fails with the param expect_success=False. We do not have a way to capture the error on the impala_shell side. We do not have a assert_impalad_log_not_contains() function. Added a few more 'not' checks from success test. -- 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: 19 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: Tue, 08 Apr 2025 21:10:50 +0000 Gerrit-HasComments: Yes
