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 38:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1888
PS37, Line 1888:
> Nit:  insert blank line below this line
Done


http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1889
PS37, Line 1889:
> Nit:  through a command
Done


http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1891
PS37, Line 1891:
> Nit:  insert blank line below this line
Done


http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1919
PS37, Line 1919:
> Need an "else" clause on this "if" statement to return a meaningful error i
Done


http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@1920
PS37, Line 1920:
> What happens when this token expires?  Will a new token be obtained?  If no
This is a refresh token feature. Its covered in another jira.


http://gerrit.cloudera.org:8080/#/c/22424/37/shell/impala_shell.py@2250
PS37, Line 2250:
> Nit, please use the Oxford Comma syntax and add a comma after -j.
Done


http://gerrit.cloudera.org:8080/#/c/22424/37/tests/custom_cluster/test_shell_jwt_auth.py
File tests/custom_cluster/test_shell_jwt_auth.py:

http://gerrit.cloudera.org:8080/#/c/22424/37/tests/custom_cluster/test_shell_jwt_auth.py@276
PS37, Line 276:         r'connected_user \(string\) = "test-user"', 
expected_count=0)
> Need to add an assert on the Impala OAuth success/fail metrics to ensure ne
Done


http://gerrit.cloudera.org:8080/#/c/22424/37/tests/shell/test_shell_commandline_jwt_auth.py
File tests/shell/test_shell_commandline_jwt_auth.py:

http://gerrit.cloudera.org:8080/#/c/22424/37/tests/shell/test_shell_commandline_jwt_auth.py@81
PS37, Line 81:     assert "Please specify at most one authentication mechanism 
(-k, -l, -j or -a)" \
> Will need to update all of these asserts after adding the comma after -j.
Done



--
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: 38
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: Michael Smith <[email protected]>
Gerrit-Reviewer: Riza Suminto <[email protected]>
Gerrit-Reviewer: gaurav singh <[email protected]>
Gerrit-Comment-Date: Tue, 03 Jun 2025 21:19:16 +0000
Gerrit-HasComments: Yes

Reply via email to