Add commands to manage roles #382
Conversation
mdellweg
force-pushed
the
roles
branch
7 times, most recently
from
de18611 to
b593e63
Compare
mdellweg
force-pushed
the
roles
branch
3 times, most recently
from
237cc46 to
1ecb6fd
Compare
mdellweg
force-pushed
the
roles
branch
4 times, most recently
from
d597f9a to
a5c8177
Compare
mdellweg
requested review from
gerrod3 and
ggainey
and removed request for
gerrod3
| if value == "": | ||
| value = "null" |
There was a problem hiding this comment.
Does this fix the issue where we can't set some fields back to None ("null") after setting it?
There was a problem hiding this comment.
This is related to the --object parameter below. Explanation there.
| CREATE_ID = "users_roles_create" | ||
| DELETE_ID = "users_roles_delete" | ||
| NULLABLES = {"content_object"} | ||
| user_ctx: PulpUserContext |
There was a problem hiding this comment.
No it it meant to hold a Context instance (with a specific user.pk)
| name="remove", | ||
| help=_("Revoke a permission from the group."), | ||
| decorators=[ | ||
| groupname_option, | ||
| group_option, | ||
| click.option( | ||
| "--permission", required=True, callback=_permission_callback, expose_value=False | ||
| "--permission", | ||
| required=True, | ||
| callback=lookup_callback("permission", PulpGroupPermissionContext), | ||
| expose_value=False, |
There was a problem hiding this comment.
Minor nitpick: should this permission.add_command call be moved to above the add_permission method so that it is grouped with the others?
There was a problem hiding this comment.
I see add_permission as one of "the others". They are all subcommands to the same group. And it is just a shame that add_permission cannot use a generic.
There was a problem hiding this comment.
Also in hopefully about 8 releases, this interface is gone anyway. ;)
| click.option("--role"), | ||
| click.option("--role-in", "role__in"), | ||
| click.option("--role-contains", "role__contains"), | ||
| click.option("--role-icontains", "role__icontains"), | ||
| click.option("--role-startswith", "role__startswith"), | ||
| click.option("--object", "content_object", callback=null_callback), |
There was a problem hiding this comment.
These are a lot of options. Can some of them be consolidated?
pulpcore/cli/core/group.py
Outdated
| click.option("--object", "content_object", required=True), | ||
| ] | ||
| ), | ||
| name="assign", |
There was a problem hiding this comment.
We should agree on a consistent name for these actions. Brian was suggesting add/remove for the roles rest mixin pr.
There was a problem hiding this comment.
Also, I see this and the remove commands both require --object to be specified. Are we not allowing users to add/remove model level roles?
There was a problem hiding this comment.
We do allow that by specifying --object "" explicitely. I thought it was too dangerous to assign model level roles whenever the admin just forgot to provide this opttion.
| click.option("--locked/--unlocked", default=None), | ||
| click.option("--name"), | ||
| click.option("--name-in", "name__in"), | ||
| click.option("--name-contains", "name__contains"), | ||
| click.option("--name-icontains", "name__icontains"), | ||
| click.option("--name-startswith", "name__startswith"), |
There was a problem hiding this comment.
Lots of options here as well. It's fine if you think we should have them all, I'm just unsure of how useful they all are.
There was a problem hiding this comment.
--name-startswith "core." is probably helpful, as is --name-contains "fileremote". About the icontains i'm not so sure.
| click.option( | ||
| "--no-permission", | ||
| is_eager=True, | ||
| is_flag=True, | ||
| expose_value=False, | ||
| callback=_no_permission_callback, | ||
| ), | ||
| click.option( | ||
| "--permission", | ||
| "permissions", | ||
| multiple=True, | ||
| help=_("Permission in the form '<app_label>.<codename>'. Can be used multiple times."), | ||
| callback=_permission_callback, | ||
| ), |
There was a problem hiding this comment.
I wonder if this could be implemented as a feature flag. Might remove the need for the _no_permission_callback. https://click.palletsprojects.com/en/8.0.x/options/#feature-switches
There was a problem hiding this comment.
Not sure this would work.
pulpcore/cli/core/role.py
Outdated
| @pass_pulp_context | ||
| @click.pass_context | ||
| def role(ctx: click.Context, pulp_ctx: PulpContext) -> None: | ||
| pulp_ctx.needs_plugin(PluginRequirement("core", min="3.16.dev")) |
| click.option( | ||
| "--password", | ||
| help=_( | ||
| "Password for the user. Provide an empty string to disable password authentication." | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Should we look into the password prompts for this field: https://click.palletsprojects.com/en/8.0.x/options/#password-prompts
There was a problem hiding this comment.
I guess, we should.
A change my password command would also be nice.
There was a problem hiding this comment.
Would you mind diverting that to a followup PR?
mdellweg
force-pushed
the
roles
branch
5 times, most recently
from
3959e76 to
3a0738d
Compare
[noissue]
|
This should be fixed by: pulp/pulpcore#1765 |
|
The CI Failure is a fluke. At some point we should really track down that file repo that keeps failing statistically. |
[noissue]