[SC 38762] Adding collection role create #1182

jkube-globus · 2025-09-28T00:05:52Z

Adding in globus collection role create and corresponding test in support of SC-38762

Updated:

src/globus_cli/commands/collection/role/__init__.py
src/globus_cli/commands/collection/role/list.py
- Updated to leveraged _fields.py
src/globus_cli/commands/collection/role/show.py
- Updated to leveraged _fields.py
tests/files/api_fixtures/collection_operations.yaml
- Adding additional routes in support of role tests
tests/functional/collection/role/test_role_list.py
- Updating to pull additional values in from collection_operations.yaml
tests/functional/collection/role/test_role_show.py
- Updating to pull additional values in from collection_operations.yaml

Added:

changelog.d/20250927_194304_james_SC_38762_Collection_Role_Create.md
src/globus_cli/commands/collection/role/create.py
src/globus_cli/commands/collection/role/_fields.py
tests/functional/collection/role/test_role_create.py

derek-globus

Hey James, sorry about the really slow review here, we must've lost sight of this PR.

A couple requests for changes, but overall looks good!

src/globus_cli/commands/collection/role/create.py

tests/functional/collection/role/test_role_create.py

tests/functional/collection/role/test_role_delete.py

…obus/globus-cli into SC-38762_Collection_Role_Create

… users primary identity for role assignments target principal

…value]

…ponse for identity

…witching all tests over

sirosen

There are some minor tweaks around the command itself to discuss, but I think my main feedback is around the supporting tooling -- fields and test data.

Overall I think this is on-track, just needs a little bit more refinement and polish! 💎

sirosen · 2025-11-14T17:35:28Z

tests/functional/collection/role/test_role_list.py

-            "http_response_code": 200,
-        },
-    ).add()
-


I see that this moved from being a code construction here to being defined in the YAML fixture files.
Can we revert this change, and the one in the role show test, and leave these data alone?

I've been trying to migrate CLI and SDK off of the YAML fixtures, and onto the pattern used here of imperatively defined test data. It's a slow transition because we can't pull support all at once (or it would break a lot of tests).

Ideally we would not make any additions to the fixture files, but I won't insist on that for all new work at this stage. For now, I just don't want us to migrate things which are moving away from that paradigm "backwards", in the wrong direction.

Note

My rationale for getting away from the YAML is that it has in practice not worked out very well for extensibility and has created some brittle dependencies between test files. Most notably, when multiple distinct files are coordinated, this is done by carefully keeping IDs in sync between distinct files. The newer way to manage data dependencies I've worked up is to construct configurators like the get_identities_mocker (seen below this comment). It still doesn't cover all needs, but I'm interested in growing that suite of tools.

Absolutely and will do!

src/globus_cli/commands/collection/role/show.py

sirosen · 2025-11-14T17:52:02Z

src/globus_cli/commands/collection/role/_fields.py

+        super().__init__(auth_client)
+        self.add_items(collection_role.get("id"))
+        self.add_items(collection_role.get("role", ()))
+        self.add_items(collection_role.get("principal"))


It's not clear to me why this additional formatter exists. Could you help me understand why it's here?

By my understanding, all three of these calls pass strings, which are then iterated on with an attempt to parse each character in those strings:

globus-cli/src/globus_cli/termio/formatters/auth.py

Lines 61 to 68 in e7d2804

for value in values:

try:

principal, principal_type = self.parse(value)

except ValueError:

pass

else:

if principal_type == "identity":

self.resolved_ids.add(principal)

Since no single character will successfully parse as a principal URN, I'd expect the three calls to do nothing.

I see it used below in a field where it is formatting the principal field in a response. Would a PrincipalURNFormatter, which is what was in use in role list prior to this PR, not suffice?

sirosen · 2025-11-14T18:02:09Z

src/globus_cli/commands/collection/role/create.py

+@command("create")
+@collection_id_arg
+@click.argument("ROLE", type=click.Choice(t.get_args(_VALID_ROLES)), metavar="ROLE")
+@click.argument("PRINCIPAL", type=str, required=False)


I have two notes on this being an optional argument.

The first is a general "CLI feel" item: if it's optional, I think it should be an option, e.g. --principal.
It's good for any individual CLI tool to establish some sort of style guide to provide a uniform experience. In globus-cli, we try to keep things 100% mapped to "all required inputs are arguments, all optional inputs are options". I should probably add this to the contrib docs.

The second is... Should this be optional? I'm not convinced that's the correct way to represent the GCS API. I think defaulting to the current user is a very surprising behavior: since the API itself is for manipulating roles, the current user must have some role already. In all likelihood, a user running role create is creating roles for other users and groups.

In summary:

We should choose @click.option("--principal") or @click.argument("PRINCIPAL") (no required=False)

My current inclination is that @click.argument("PRINCIPAL") is most appropriate

Agreed on the first point, option = optional; argument = required.

The second point is a change that I suggested to James. I'll ask in the gcs devchat; seems like a pretty domain specific usage question.

seems like a pretty domain specific usage question.

Maybe this is where our perspectives diverge. I don't think it's purely domain specific.

I think we should consider this in the context of the following commands:

globus endpoint permission create

globus endpoint role create

globus flows update -- specifically the role options

globus group {join,leave,invite}

globus group member {add,approve,invite,reject,remove}

globus search index role create

Of these, globus group join, globus group leave, globus group invite accept, and globus group invite decline all lookup the current user, and the rest require a user or group as input. globus flows update is the only one I would consider inherently dissimilar from this API.

gcs weighed in, let's go back to your original behavior James (no defaulting to the current user). Sorry for the flip-flop.

From Jason:

Insist on an explicit identity_id or username
that makes it consistent with the gcs cli
admins omitting identity_id can (1) lead to confusing about which identity id will be used and (2) support tickets when things don't behave as expected
But also, in order to run collection role create you already need a role to allow the operation, so the operation is a bit redundant

sirosen · 2025-11-14T18:05:44Z

src/globus_cli/commands/collection/role/create.py

+    "--principal-type",
+    type=click.Choice(["identity", "group"]),
+    help="Qualifier to specify what type of principal (identity or group) is provided.",
+)


Note
For any other reviewers, this --principal-type phrasing is already in use elsewhere in the CLI, and I think we should apply it here (as currently in the PR) for uniformity.

That said, I'm very much interested in developing a better interface for passing these inputs. I think we could design something as a replacement which takes URNs, short-forms like group:<id>, and usernames.

Co-authored-by: Stephen Rosen <[email protected]>

jkube-globus added 10 commits September 23, 2025 19:00

Adding in collection role create

39495c3

Merge branch 'main' into SC-38762_Collection_Role_Create

9a5d427

Updated testing as well as formatting in create.py

cb68ac3

Correcting test_role_delete.py and testin test_role_create.py change

ee4569e

Switched to returning the role ID only

4c09912

Adding Changelog entry

524f90b

Removing unused variable 'identity_username'

cb39e0d

Correcting type hints

36d7287

Merge branch 'main' into SC-38762_Collection_Role_Create

1be8820

Adding in missing import of annotations

dbfb4cf

jkube-globus marked this pull request as ready for review October 16, 2025 15:45

jkube-globus requested review from MaxTueckeGlobus, aaschaer, ada-globus, derek-globus, kurtmckee, m1yag1 and sirosen as code owners October 16, 2025 15:45

derek-globus requested changes Oct 21, 2025

View reviewed changes

jkube-globus and others added 2 commits October 21, 2025 18:50

Merge branch 'globus:main' into SC-38762_Collection_Role_Create

1a97849

Switching display to a fieldset from text

a96a780

jkube-globus marked this pull request as draft October 21, 2025 23:06

jkube-globus added 8 commits October 21, 2025 19:09

Merge branch 'SC-38762_Collection_Role_Create' of github.com:jkube-gl…

8e97d2d

…obus/globus-cli into SC-38762_Collection_Role_Create

Correcting tests

b8e9b1c

Merge branch 'main' into SC-38762_Collection_Role_Create

3524f18

Correcting tests

2ff8829

Correcting tests

6f86379

Making principal argument optional and looking up currently logged in…

8daa6c5

… users primary identity for role assignments target principal

De-conflcting git pull

a479543

Adding verbiage for PRINCIPAL argument handling [default vs explicit …

8bd9bc7

…value]

jkube-globus added 3 commits November 12, 2025 08:40

Making collection roles classier with _fields.py

37e9570

Updating tests and collection_operations.yaml to include and auth res…

02d161e

…ponse for identity

Adding additional Collection roles routes in support of testing and s…

1dd8494

…witching all tests over

jkube-globus marked this pull request as ready for review November 13, 2025 15:45

sirosen requested changes Nov 14, 2025

View reviewed changes

Update src/globus_cli/commands/collection/role/show.py

ad29988

Co-authored-by: Stephen Rosen <[email protected]>

	for value in values:
	try:
	principal, principal_type = self.parse(value)
	except ValueError:
	pass
	else:
	if principal_type == "identity":
	self.resolved_ids.add(principal)

[SC 38762] Adding collection role create #1182

Are you sure you want to change the base?

[SC 38762] Adding collection role create #1182

Uh oh!

Conversation

jkube-globus commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derek-globus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sirosen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jkube-globus commented Sep 28, 2025 •

edited

Loading