feat: CLI command to list the members of a Workspace [RM-388] #9686

ShreyaLnuHpe · 2024-07-19T01:32:52Z

Ticket

Description

We don't have a command that lists users/user-groups members of a Workspace and their permissions).
For details, visit original ask- DFR-462

Created a new CLI command to list user/group members of a Workspace

% det -u admin workspace --help
usage: det workspace [-h] subcommand ...

positional arguments:
  subcommand
    help         show help for this command
    archive      archive workspace
    create       create workspace
    delete       delete workspace
    describe     describe workspace
    edit         edit workspace
    list (ls)    list all workspaces
    list-members
                 list users/user-groups members of a workspace and their roles
    list-pools   list the resource pools available to a workspace
    list-projects
                 list the projects associated with a workspace
    unarchive    unarchive workspace

New CLI command:

% det -u admin workspace list-members <workspace_name> #default: tabular
% det -u admin workspace list-members <workspace_name> --json
% det -u admin workspace list-members <workspace_name> --csv
% det -u admin workspace list-members <workspace_name> --yaml

Test Plan

No more tests needed

Test Done

Manually Tested
E2E Test completed

Created Workspace, UserGroup, User and given Roles (though CLI/WebUI)

% det -u admin workspace list-members ws1
 User/Group Name   | User/Group   | Role Name
-------------------+-------+------------------
 admin             | U            | WorkspaceAdmin
 ug1               | G            | Editor
 u1                | U            | EditorRestricted

Similar to the Members tab in WebUI

Output in PGAdmin:

SELECT u.id AS user_id, u.username, r.role_name, u.active, u.admin
FROM users u
JOIN user_group_membership ugm ON u.id = ugm.user_id
JOIN role_assignments ra ON ugm.group_id = ra.group_id
JOIN roles r ON ra.role_id = r.id;

data-1721352684733.csv

Checklist

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

netlify · 2024-07-19T01:33:11Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`97062b6`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66a3f88373d5d70008f85b85

hamidzr

mostly lgtm thanks for working on this. i added some quick comments + a question in the slack thread.

harness/determined/cli/workspace.py

codecov · 2024-07-22T18:24:22Z

Codecov Report

Attention: Patch coverage is 7.14286% with 26 lines in your changes missing coverage. Please review.

Project coverage is 53.43%. Comparing base (7260f04) to head (97062b6).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9686      +/-   ##
==========================================
- Coverage   53.44%   53.43%   -0.01%     
==========================================
  Files        1257     1257              
  Lines      154350   154378      +28     
  Branches     3298     3298              
==========================================
+ Hits        82494    82495       +1     
- Misses      71706    71733      +27     
  Partials      150      150

Flag	Coverage Δ
backend	`44.86% <ø> (-0.01%)`	⬇️
harness	`72.62% <7.14%> (-0.07%)`	⬇️
web	`51.92% <ø> (ø)`

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

Files	Coverage Δ
harness/determined/cli/workspace.py	`15.41% <7.14%> (-1.10%)`	⬇️

... and 4 files with indirect coverage changes

harness/determined/cli/workspace.py

hamidzr

I'd check the way we mark users and groups with product in case they have a preference otherwise the functionality lgtm. maybe the header could be full User/Group to make it more clear.

Also, would we want to trim the user and permission objects from the json/yaml outputs? Is that something we do with any of our other CLIs? Let's skip this unless we get confirmation we want this.

.usersAssignedDirectly[]
.groups[].users[]
.assignments[].role.permissions[]


  role:
    name: WorkspaceAdmin
    permissions:
        # just keep the ids?
        - id: PERMISSION_TYPE_VIEW_TEMPLATES
          name: view templates
          scopeTypeMask:
            cluster: true
            workspace: true
    roleId: 2
    scopeTypeMask:
      cluster: true
      workspace: true



- active: true
  admin: false
  agentUserGroup: null
  displayName: ''
  id: 2
  lastAuthAt: null
  modifiedAt: '2024-07-22T16:02:07.363788Z'
  remote: false
  # just keep the username?
  username: determined

harness/determined/cli/workspace.py

ShreyaLnuHpe · 2024-07-25T02:38:15Z

e2e_tests/tests/cluster/test_workspace_org.py

+        detproc.check_call(admin_sess, create_workspace_cmd)
+
+        # Create test users (3) and groups (2) and assign to workspace.
+        roles = ["Editor", "Viewer", "WorkspaceAdmin", "EditorRestricted", "ModelRegistryViewer"]


Do we have some enum defined or a better way to access them? I couldn't find.

not that I know of. but another suggestion around here would be to use this utility instead which directly uses our apis to set up (i think) what you're tying to set up

determined/e2e_tests/tests/cluster/test_rbac.py

Line 25 in 769d600

def create_workspaces_with_users(

I would prefer keeping the code same to maintain the consistency while creating the group, their roles and assigning to the workspace.
Thanks for suggesting though.

hamidzr · 2024-07-25T16:02:52Z

e2e_tests/tests/cluster/test_workspace_org.py

+        detproc.check_call(admin_sess, create_workspace_cmd)
+
+        # Create test users (3) and groups (2) and assign to workspace.
+        roles = ["Editor", "Viewer", "WorkspaceAdmin", "EditorRestricted", "ModelRegistryViewer"]


not that I know of. but another suggestion around here would be to use this utility instead which directly uses our apis to set up (i think) what you're tying to set up

determined/e2e_tests/tests/cluster/test_rbac.py

Line 25 in 769d600

def create_workspaces_with_users(

azhou-determined · 2024-07-26T18:32:05Z

harness/determined/cli/workspace.py

+                    cli.Group(
+                        cli.output_format_args["json"],
+                        cli.output_format_args["yaml"],
+                        cli.output_format_args["table"],


let's not include "--table", it's the default and not a "real" format

azhou-determined

one comment that's not necessarily blocking, but otherwise LGTM!

CLI command to list the members of a Workspace

582e535

ShreyaLnuHpe requested a review from hamidzr July 19, 2024 01:32

ShreyaLnuHpe requested a review from a team as a code owner July 19, 2024 01:32

ShreyaLnuHpe requested a review from azhou-determined July 19, 2024 01:32

cla-bot bot added the cla-signed label Jul 19, 2024

hamidzr reviewed Jul 22, 2024

View reviewed changes

harness/determined/cli/workspace.py Outdated Show resolved Hide resolved

harness/determined/cli/workspace.py Outdated Show resolved Hide resolved

as per comments

e6efe69

ShreyaLnuHpe requested a review from hamidzr July 22, 2024 18:40

varlogtim reviewed Jul 22, 2024

View reviewed changes

harness/determined/cli/workspace.py Show resolved Hide resolved

varlogtim reviewed Jul 22, 2024

View reviewed changes

harness/determined/cli/workspace.py Outdated Show resolved Hide resolved

harness/determined/cli/workspace.py Outdated Show resolved Hide resolved

hamidzr approved these changes Jul 22, 2024

View reviewed changes

harness/determined/cli/workspace.py Outdated Show resolved Hide resolved

add tests

6d044b6

ShreyaLnuHpe requested a review from a team as a code owner July 25, 2024 02:36

ShreyaLnuHpe requested a review from NicholasBlaskey July 25, 2024 02:36

ShreyaLnuHpe commented Jul 25, 2024

View reviewed changes

ShreyaLnuHpe requested a review from hamidzr July 25, 2024 02:40

hamidzr approved these changes Jul 25, 2024

View reviewed changes

ShreyaLnuHpe added 2 commits July 25, 2024 10:57

Merge branch 'main' into shreya/makeCLICommand

6d56504

corrected make harness check

73f674e

ShreyaLnuHpe changed the title ~~feat: CLI command to list the members of a Workspace~~ feat: CLI command to list the members of a Workspace [RM-388] Jul 25, 2024

corrected make e2e_tests check

c581865

azhou-determined reviewed Jul 26, 2024

View reviewed changes

azhou-determined approved these changes Jul 26, 2024

View reviewed changes

removing --table option

97062b6

ShreyaLnuHpe enabled auto-merge (squash) July 26, 2024 19:54

ShreyaLnuHpe merged commit 9efb216 into main Jul 26, 2024
113 of 122 checks passed

ShreyaLnuHpe deleted the shreya/makeCLICommand branch July 26, 2024 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: CLI command to list the members of a Workspace [RM-388] #9686

feat: CLI command to list the members of a Workspace [RM-388] #9686

ShreyaLnuHpe commented Jul 19, 2024 •

edited

Loading

netlify bot commented Jul 19, 2024 •

edited

Loading

hamidzr left a comment

codecov bot commented Jul 22, 2024 •

edited

Loading

hamidzr left a comment •

edited

Loading

ShreyaLnuHpe Jul 25, 2024

hamidzr Jul 25, 2024

ShreyaLnuHpe Jul 25, 2024

hamidzr Jul 25, 2024

azhou-determined Jul 26, 2024

azhou-determined left a comment

feat: CLI command to list the members of a Workspace [RM-388] #9686

feat: CLI command to list the members of a Workspace [RM-388] #9686

Conversation

ShreyaLnuHpe commented Jul 19, 2024 • edited Loading

Ticket

Description

Test Plan

Test Done

Checklist

netlify bot commented Jul 19, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

hamidzr left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

hamidzr left a comment • edited Loading

Choose a reason for hiding this comment

ShreyaLnuHpe Jul 25, 2024

Choose a reason for hiding this comment

hamidzr Jul 25, 2024

Choose a reason for hiding this comment

ShreyaLnuHpe Jul 25, 2024

Choose a reason for hiding this comment

hamidzr Jul 25, 2024

Choose a reason for hiding this comment

azhou-determined Jul 26, 2024

Choose a reason for hiding this comment

azhou-determined left a comment

Choose a reason for hiding this comment

ShreyaLnuHpe commented Jul 19, 2024 •

edited

Loading

netlify bot commented Jul 19, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

hamidzr left a comment •

edited

Loading