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.

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

Enhancement / Print “accessing as <user>” in client #493

Merged
merged 11 commits into from
Jul 2, 2024

Conversation

EvgeniLeonti
Copy link
Contributor

Copy link

dagshub bot commented Jul 1, 2024

@EvgeniLeonti
Copy link
Contributor Author

@kbolashev i have implemented initial version which works fine, but isn't efficient:
it will make another http request to get the username from the token.

in cases where we call is_valid_token(), this new http request can be skipped.

so we have 2 options, leave it as is, or make required adjustments to skip the redundant http call.
in order to achieve the 2nd option, i thought about:

  1. adding username to __token_cache
  2. maintain existing caches to support backwards compatibility (adding username to already existing tokens)

this will require more complicated logic, and i'm not sure the scope of this task covers that.

WDYT?

@kbolashev
Copy link
Member

kbolashev commented Jul 1, 2024

I think you can change the signature and name of is_valid_token to return the username and get 99% of the way there.
You'll only have to call it for the case of the EnvVar token and that's it.

Turn it into get_username_of_token(token, host) -> Optional[str], return None if it fails, returns "login" otherwise.

Copy link
Member

@kbolashev kbolashev left a comment

Choose a reason for hiding this comment

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

Looks great!

I left a bunch of minor things that need to be changed, but the logic is great.

dagshub/auth/tokens.py Outdated Show resolved Hide resolved
dagshub/auth/tokens.py Outdated Show resolved Hide resolved
dagshub/auth/tokens.py Outdated Show resolved Hide resolved
dagshub/auth/tokens.py Outdated Show resolved Hide resolved
dagshub/auth/tokens.py Show resolved Hide resolved
dagshub/auth/tokens.py Outdated Show resolved Hide resolved
dagshub/auth/tokens.py Outdated Show resolved Hide resolved
tests/common/test_determine_repo.py Outdated Show resolved Hide resolved
dagshub/auth/tokens.py Outdated Show resolved Hide resolved
@kbolashev kbolashev added the enhancement New feature or request label Jul 1, 2024
Copy link
Member

@kbolashev kbolashev left a comment

Choose a reason for hiding this comment

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

Congrats on the first feature PR for the client!!! 🥳

@EvgeniLeonti EvgeniLeonti merged commit a20b330 into master Jul 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants