vdk-control-cli: Adopt new vdk-control-api-auth library #840
Conversation
doks5
force-pushed
the
person/andonova/vdk-auth
branch
from
e55e23b to
d530a8a
Compare
projects/vdk-control-cli/src/vdk/internal/control/command_groups/login_group/login.py
Outdated
Show resolved
Hide resolved
doks5
force-pushed
the
person/andonova/vdk-auth
branch
2 times, most recently
from
c1e144d to
77f201d
Compare
With the release of the vdk-control-api-auth library, we now have a stand-alone authentication package that is to be used by all Versatile Data Kit components that need to authenticate against a third-party service. This change adopts the new authentication package in vdk-control-cli, and is now used when issuing `vdk login` commands. Additionally, authentication logic in vdk-control-cli which is overlapping with such logic in vdk-control-api-auth is removed. Testing Done: Installed the new changes locally and ran `vdk login` to verify that the browser redirecs work as expected. Also, ran a docker container with the latest changes to verify that authentication is possible even when there is no access to a browser. Signed-off-by: Andon Andonov <[email protected]>
doks5
force-pushed
the
person/andonova/vdk-auth
branch
from
77f201d to
9858382
Compare
| click.echo("Login Successful") | ||
| except VDKAuthException as e: | ||
| if ( |
There was a problem hiding this comment.
Parsing error messages is not a good practice. You should not look at what we do in Impala as a good example. We have no control over what Expcetion Impala raises so we have to parse them there. We do within vdk-control-api-auth library. We should use error codes or enums or exception hierarchy to handle this .
| try: | ||
| apikey_auth.authenticate() | ||
| except VDKAuthException as e: | ||
| click.echo(f"Login failed. Error was: {e.message}") |
There was a problem hiding this comment.
That does not seem correct. If auth fails we should exit with non 0 exit code. This will only print the failure and will exit with 0 . Why not let the exception propagate or wrap it in UserError
There was a problem hiding this comment.
I agree, though I believe we didn't exit with non-0 status before either.
There was a problem hiding this comment.
I opted out of propagating the exception, as that would be displayed in the user's terminal, and I was not sure if this would be good UX. Also, we have never showed exception messages in terminal for failed login, so doing it now seems like a regression.
With the release of the vdk-control-api-auth library, we now have a
stand-alone authentication package that is to be used by all Versatile
Data Kit components that need to authenticate against a third-party
service.
This change adopts the new authentication package in vdk-control-cli,
and is now used when issuing
vdk logincommands.Additionally, authentication logic in vdk-control-cli which is overlapping
with such logic in vdk-control-api-auth is removed.
Testing Done: Installed the new changes locally and ran
vdk loginto verifythat the browser redirecs work as expected. Also, ran a docker container with
the latest changes to verify that authentication is possible even when there
is no access to a browser.
Signed-off-by: Andon Andonov [email protected]