-
Notifications
You must be signed in to change notification settings - Fork 70
Add tenants related commands #28
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
Conversation
| assert.False(t, strings.Contains(out.String(), "delete-tenant-test")) | ||
| } | ||
|
|
||
| func TestDeleteTenantArgsError(t *testing.T) { |
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.
please add test case to cover deleting a non-existent tenant
pkg/ctl/tenant/update.go
Outdated
| return vc.NameError | ||
| } | ||
|
|
||
| if data.AllowedClusters == nil && data.AdminRoles == nil { |
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.
Is this really needed?
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.
umm, I think if you don't want to change anything so why you want to execute update tenant.
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.
a user can remove all the allowed clusters and admin roles by specifying empty list of allowed clusters and roles, no?
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.
update Updates the configuration for a tenant
Usage: update [options] tenant-name
Options:
--admin-roles, -r
Comma separated list of auth principal allowed to administrate the
tenant. If empty the current set of roles won't be modified
--allowed-clusters, -c
Comma separated allowed clusters. If omitted, the current set of
clusters will be preserved
I used java admin to test but there is no changes with an empty list.
pkg/ctl/tenant/delete.go
Outdated
| func deleteTenantCmd(vc *cmdutils.VerbCmd) { | ||
| var desc pulsar.LongDescription | ||
|
|
||
| desc.CommandUsedFor = "This command is used for deleting a tenant and all namespaces and topics under it will be deleted." |
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.
two questions here:
- does pulsar allow deleting a non-empty tenant?
- do we have a test case to cover the case?
|
@zymap it seems that my comment regarding deleting a non-empty tenant is not addressed. PTAL and make sure you have responded to all my comments. |
Master issue: streamnative/pulsarctl#2 The pull requests add all tenant commands.
Master issue: streamnative/pulsarctl#2 The pull requests add all tenant commands.
Master issue: #2
The pull requests add all tenant commands.