-
Notifications
You must be signed in to change notification settings - Fork 359
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
chore: add SDK functionality for SSO Improvements #10096
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10096 +/- ##
==========================================
- Coverage 54.39% 49.33% -5.07%
==========================================
Files 1268 1227 -41
Lines 159317 157143 -2174
Branches 3631 3631
==========================================
- Hits 86668 77521 -9147
- Misses 72515 79488 +6973
Partials 134 134
Flags with carried forward coverage won't be shown. Click here to find out more.
|
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
||
|
||
class TokenType(enum.Enum): | ||
# UNSPECIFIED is internal to the bound API and is not be exposed to the front end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment seems to be leftover from copy/paste?
ACCESS_TOKEN = bindings.v1TokenType.ACCESS_TOKEN.name | ||
|
||
|
||
class AccessToken: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just call this Token
, that's what it is in the CLI after all
) | ||
return token.AccessToken._from_bindings(resp.tokenInfo, self._session) | ||
|
||
def list_tokens( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think all 3 of these new methods: describe_token
, describe_tokens
, and list_tokens
should be combined into a singular list_tokens
that takes in all the parameters.
the CLI and SDK roughly overlap in functionality, but the methods shouldn't be an exact replica. in the CLI, we have to be careful about which methods to expose and how because the UI is limited to the command line, so we generally value convenience and modularity. but the SDK has a bit of a different philosophy. it's in code and made for developers, so the methods we expose can be more powerful and more robust. (see list_experiments
here as an example)
the functionality in describe_token
, describe_tokens
, and list_tokens
can be captured with a single list_tokens
method, and the user can pass in exactly what they want, just like in code. i'm thinking list_tokens
would have a signature like:
def list_tokens(
self,
username: Optional[str],
token_ids: Optional[List[int]],
include_inactive: bool,
sort_by: token.TokenSortBy = token.TokenSortBy.NAME,
order_by: OrderBy = OrderBy.ASCENDING,
) -> List[token.Token]:
this is nice because it basically mirrors the actual bindings API call, the user isn't limited by separate methods, and the method is representative of the actual query we make in the system. we should also include sort/order as accepted parameters.
return token.AccessToken._from_bindings(resp.tokenInfo, self._session) | ||
|
||
def create_token( | ||
self, user_id: int, lifespan: Optional[str] = None, description: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make user_id
username here, since the other methods in the SDK use that as the standard identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, we've decided to accept days, so let's just make it expiration_days
here.
|
||
def reload(self) -> None: | ||
resp = bindings.get_GetAccessTokens( | ||
session=self._session, tokenIds=[self.id], showInactive=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does self.id
exist? either change this to self.token_id
or add a property to this class:
@property
def id(self) -> int:
return self._id
).tokenInfo | ||
self._hydrate(resp[0]) | ||
|
||
def edit_token(self, desc) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make this set_description
to follow standard for other SDK methods.
) | ||
self.reload() | ||
|
||
def revoke_token(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call this revoke
, we're already on the token
object, no need for the extra verbosity.
|
||
@classmethod | ||
def _from_bindings( | ||
cls, AccessToken_bindings: List[bindings.v1TokenInfo], session: api.Session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing is weird with AccessToken_bindings
. just call it token_bindings
?
@classmethod | ||
def _from_bindings( | ||
cls, AccessToken_bindings: List[bindings.v1TokenInfo], session: api.Session | ||
) -> "AccessToken | List[AccessToken]": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method should always just return a single Token
object, because it lives on the Token
class. it's up to the caller to create the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it'd be great if the Determined
SDK client class accepted a token: Optional[str]
parameter as well to be able to use the SDK with these access tokens.
Ticket
DET-10456
DET-10406
DET-10402
DET-10399
Description
Test Plan
Checklist
docs/release-notes/
See Release Note for details.