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

feat: Use filtered resource pools when creating notebook #9045

Merged
merged 5 commits into from
Mar 26, 2024

Conversation

gt2345
Copy link
Contributor

@gt2345 gt2345 commented Mar 25, 2024

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

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

MD-284

@gt2345 gt2345 requested a review from a team as a code owner March 25, 2024 19:27
@gt2345 gt2345 requested a review from hkang1 March 25, 2024 19:27
@cla-bot cla-bot bot added the cla-signed label Mar 25, 2024
Copy link

netlify bot commented Mar 25, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 46f7b71
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/660348a68a350b000828b8af
😎 Deploy Preview https://deploy-preview-9045--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@gt2345 gt2345 requested review from keita-determined and removed request for hkang1 March 25, 2024 19:33
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 41.74%. Comparing base (f08b406) to head (46f7b71).
Report is 11 commits behind head on main.

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              
Flag Coverage Δ
harness ?
web 40.92% <84.61%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
webui/react/src/stores/workspaces.tsx 51.39% <100.00%> (+2.24%) ⬆️
webui/react/src/components/JupyterLabModal.tsx 84.16% <83.33%> (-0.17%) ⬇️

... and 320 files with indirect coverage changes

Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

added some comments, otherwise it works as expected. LGTM

@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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">
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

what does !boundResourcePoolsMap mean here? i dont think boundResourcePoolsMap can be undefined. and empty map still return true.

nit: selectedWorkspace === undefined

@keita-determined keita-determined changed the title feat: Use filtered resource pool when creating notebook feat: Use filtered resource pools when creating notebook Mar 26, 2024
@gt2345 gt2345 merged commit 1e6f0f7 into main Mar 26, 2024
69 of 81 checks passed
@gt2345 gt2345 deleted the gt/284-filtered-pool-for-notebook branch March 26, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants