chore: add SDK functionality for SSO Improvements#10096
chore: add SDK functionality for SSO Improvements#10096ShreyaLnuHpe wants to merge 1 commit intomainfrom
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
2 participants
Ticket
DET-10456
DET-10406
DET-10402
DET-10399
Description
Test Plan
Checklist
docs/release-notes/See Release Note for details.