feat(kafka acl): add instance-id flag and use all alias#1217
feat(kafka acl): add instance-id flag and use all alias#1217rkpattnaik780 merged 2 commits intomainfrom
Conversation
wtrocki
left a comment
wtrocki
left a comment
There was a problem hiding this comment.
Looks and works good. Putting giberish into instance-id will return 404. Maybe we can have some message for it (but not required to merge)
rkpattnaik780
force-pushed
the
acl_instance_id
branch
from
d7fda37 to
f157644
Compare
We are getiing a |
craicoverflow
left a comment
craicoverflow
left a comment
There was a problem hiding this comment.
This is great. We are missing dynamic auto completions though for loading the instance IDs from the server. It would be great to add this either now or in a follow up PR.
|
|
||
| # Grant access to principal for producing messages to all topics | ||
| $ rhoas kafka acl grant-permissions --producer --user user_name --topic "*" | ||
| $ rhoas kafka acl grant-permissions --producer --user user_name --topic all |
There was a problem hiding this comment.
Should we have an example for both?
There was a problem hiding this comment.
Not sure myself. @rkpattnaik780 feel free to make call
There was a problem hiding this comment.
Won't prefer having asterisk as an example.
| cmd.Flags().BoolVar(&opts.producer, "producer", false, opts.localizer.MustLocalize("kafka.acl.grantPermissions.flag.producer.description")) | ||
| cmd.Flags().StringVar(&opts.topicPrefix, "topic-prefix", "", opts.localizer.MustLocalize("kafka.acl.common.flag.topicPrefix.description")) | ||
| cmd.Flags().StringVar(&opts.groupPrefix, "group-prefix", "", opts.localizer.MustLocalize("kafka.acl.common.flag.groupPrefix.description")) | ||
| cmd.Flags().StringVar(&opts.kafkaID, "instance-id", "", opts.localizer.MustLocalize("kafka.acl.common.flag.instance.id")) |
There was a problem hiding this comment.
What would your thoughts be if we make this a persistent flag at the acl command level? This would save having to add it to every acl subcommand.
There was a problem hiding this comment.
Can we create persistent flag without redefiniing it to set opts.kafkaID in the cmd?
That will probably require some packaged scoped variable that all commands will read?
Not sure if there is clever way that is as clean as defining that flag in every command
There was a problem hiding this comment.
I agree, we could have something similar for registry commands as well?
There was a problem hiding this comment.
Actually @wtrocki is probably correct, we would need to have the value set in memory via a pointer like the debug flag for this work.
Feel free to explore the idea, but it may not be worth it.
There was a problem hiding this comment.
Ideally we can apply instance-id to numerous commands as in #685 so we can think about this clever way and then change it across CLI. WDYT?
There was a problem hiding this comment.
skipping, to be addressed along with 685
There was a problem hiding this comment.
@craicoverflow , filtering by id column seems to be unsupported in Admin API. Should we raise it to admin api team, this would enable completions at multiple places.
There was a problem hiding this comment.
I'm not sure what you mean, why does the admin API require ID filtering?
There was a problem hiding this comment.
I meant for auto completions of --instance-id
There was a problem hiding this comment.
You mean the control plane API?
You could fetch all IDs and cache them. I don't think we need to have filtering in the API for it for this one use case.
3 participants
allalias forkafka acl grant-permissions.Closes #1203 , #1200
Verification Steps
Type of change
Checklist