-
Notifications
You must be signed in to change notification settings - Fork 354
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
fix: Kill task permission on interactive page #8358
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
left a nit -- we should handle the task loading as a Loadable
.
const [task, setTask] = useState<CommandTask>(); | ||
|
||
const getTaskById = useCallback(async (taskType: CommandType, commandId: string) => { | ||
switch (taskType) { | ||
case 'command': | ||
setTask(await getCommand({ commandId })); | ||
break; | ||
case 'jupyter-lab': | ||
setTask(await getJupyterLab({ commandId })); | ||
break; | ||
case 'shell': | ||
setTask(await getShell({ commandId })); | ||
break; | ||
case 'tensor-board': | ||
setTask(await getTensorBoard({ commandId })); | ||
break; | ||
} | ||
}, []); | ||
|
||
useEffect(() => { | ||
getTaskById(type, id); | ||
}, [type, id, getTaskById]); |
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.
consider using useAsync
here -- it wraps the value in a Loadable
(which we should do here because now it's possible for the task to not load) and handles race conditions:
const [task, setTask] = useState<CommandTask>(); | |
const getTaskById = useCallback(async (taskType: CommandType, commandId: string) => { | |
switch (taskType) { | |
case 'command': | |
setTask(await getCommand({ commandId })); | |
break; | |
case 'jupyter-lab': | |
setTask(await getJupyterLab({ commandId })); | |
break; | |
case 'shell': | |
setTask(await getShell({ commandId })); | |
break; | |
case 'tensor-board': | |
setTask(await getTensorBoard({ commandId })); | |
break; | |
} | |
}, []); | |
useEffect(() => { | |
getTaskById(type, id); | |
}, [type, id, getTaskById]); | |
const task = useAsync(() => { | |
const call = { | |
command: getCommand, | |
'jupyter-lab': getJupyterLab | |
shell: getShell, | |
'tensor-board': getTensorBoard | |
}[type] | |
return call({ commandId: id }); | |
}, [type, id]) |
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.
had two small nits -- approved pending tests passing
Description
Interactive task used to not have access to the workspace id, so the permission check for killing task is always false for non-admin.
Test Plan
Log in as a user with workspace admin role
Create a notebook in that workspace
Notebook can be killed from
/det/tasks
page, as well as on/det/interactive
pageCommentary (optional)
Checklist
docs/release-notes/
.See Release Note for details.
Ticket
WEB-1268