feat: Use filtered resource pools when creating notebook #9045
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9045 +/- ##
==========================================
- Coverage 47.81% 41.74% -6.07%
==========================================
Files 1161 841 -320
Lines 143646 104196 -39450
Branches 2373 2378 +5
==========================================
- Hits 68678 43496 -25182
+ Misses 74815 60547 -14268
Partials 153 153
Flags with carried forward coverage won't be shown. Click here to find out more.
|
keita-determined
approved these changes
Mar 26, 2024
| @@ -129,6 +129,8 @@ class WorkspaceStore extends PollingStore { | |||
| public readonly boundResourcePools = (workspaceId: number) => | |||
| this.#boundResourcePools.select((map) => map.get(workspaceId)?.toJS()); | |||
|
|
|||
| public readonly boundResourcePoolsMap = () => this.#boundResourcePools.readOnly(); | |||
There was a problem hiding this comment.
Does that need to be a function?
why not like this?
Suggested change
| public readonly boundResourcePoolsMap = () => this.#boundResourcePools.readOnly(); | |
| public readonly boundResourcePoolsMap = this.#boundResourcePools.readOnly(); |
| @@ -349,12 +349,23 @@ const JupyterLabForm: React.FC<{ | |||
| }> = ({ form, formId, currentWorkspace, defaults, lockedWorkspace, setWorkspace, workspaces }) => { | |||
| const [templates, setTemplates] = useState<Template[]>([]); | |||
|
|
|||
| const resourcePools = Loadable.getOrElse([], useObservable(clusterStore.resourcePools)); | |||
| const selectedWorkspace = Form.useWatch('workspaceId', form); | |||
There was a problem hiding this comment.
nit:
Suggested change
| const selectedWorkspace = Form.useWatch('workspaceId', form); | |
| const selectedWorkspaceId = Form.useWatch('workspaceId', form); |
| <Select allowClear placeholder="Pick the best option"> | ||
| {resourcePools.map((pool) => ( | ||
| <Form.Item label="Resource Pool" name="pool"> | ||
| <Select allowClear disabled={!selectedWorkspace} placeholder="Pick the best option"> |
There was a problem hiding this comment.
nit: i dont think workspace id would take 0 in the future, but as a convention, selectedWorkspaceId === undefined can avoid a potential bug.
| const boundResourcePoolsMap = useObservable(workspaceStore.boundResourcePoolsMap()); | ||
|
|
||
| const boundResourcePools: ResourcePool[] = useMemo(() => { | ||
| if (!Loadable.isLoaded(resourcePools) || !boundResourcePoolsMap || !selectedWorkspace) |
There was a problem hiding this comment.
what does !boundResourcePoolsMap mean here? i dont think boundResourcePoolsMap can be undefined. and empty map still return true.
nit: selectedWorkspace === undefined
keita-determined
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Filter resource pools by selected workspace when creating notebook.
Test Plan
With and without rbac enabled, open the modal for creating Jupyter notebook.
Before selecting the workspace, the resource pool dropdown is disabled.
After selecting the workspace, verify that the resource pool dropdown only contains resource pools that are bound to that workspace (including shared resource pools)
Commentary (optional)
Checklist
docs/release-notes/.See Release Note for details.
Ticket
MD-284