feat(acl list): add flags to filter by resource by rkpattnaik780 · Pull Request #1299 · redhat-developer/app-services-cli

rkpattnaik780 · 2021-11-08T14:56:38Z

User should be able to list the ACLs per Cluster, Topic or Group.
A small bug fix to ACL list

Verification Steps

Run the command to list permissions for all accounts for topic topic_name

./rhoas kafka acl list --all-accounts --topic kakashi

Run the command to show all permssions pertaining to group "console-consumer"

./rhoas kafka acl list --group console-consumer

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Other (please specify)

Checklist

Documentation added for the feature
CI and all relevant tests are passing
Code Review completed
Verified independently by reviewer

wtrocki · 2021-11-08T15:18:17Z

Verified/Reviewed. Changes look good. Pinging @craicoverflow as another SME for correctness

craicoverflow

Some changes needed. Left a comment regarding whether --cluster is needed.

pkg/cmd/kafka/acl/list/list.go

pkg/localize/locales/en/cmd/acl.en.toml

pkg/cmd/kafka/acl/list/list.go

craicoverflow · 2021-11-08T16:59:55Z

pkg/cmd/kafka/acl/list/list.go

+	flags.AddTopic(&opts.topic)
+	flags.AddConsumerGroup(&opts.group)


We need to add dynamic completions for these.

Will it be good to add an additional field factory in the flagSet struct? That will make it easier to reuse completions for topic and group from cmdutils?

Yeah, that would be a good move. You can remove the localizer and conn fields so and just use this :)

I'd recommend refactoring these completions APIs also to look like this one:

app-services-cli/pkg/kafka/cmdutil/util.go

Line 11 in 8edfc44

func RegisterNameFlagCompletionFunc(cmd *cobra.Command, f *factory.Factory) error {

They also probably should not be in a generic cmdutil package since they are kafka specific (I did that 😂)

craicoverflow · 2021-11-11T10:03:22Z

This is ready for review @rkpattnaik780?

rkpattnaik780 · 2021-11-11T10:08:05Z

This is ready for review @rkpattnaik780?

Yes.

craicoverflow · 2021-11-11T10:10:20Z

This is ready for review @rkpattnaik780?

Yes.

Thanks.

wtrocki · 2021-11-11T10:39:49Z

Documentation needs little bit refinement:
I would use some common prefix to say what flag does - mainly filtering.

Set filter to all-accounts
Set filter to only cluster type

etc.

Current docs are less refined

Flags:
      --all-accounts             Set the ACL principal to match all principals (users and service accounts)
      --cluster                  Set the resource type to cluster
      --group string             Set the consumer group resource. When the --prefix option is also passed, this is used..
      --service-account string   Service account client ID used as principal for this operation

wtrocki · 2021-11-11T11:00:10Z

Documentation needs improvement. Otherwise ok.

rkpattnaik780 · 2021-11-11T12:00:51Z

Documentation needs little bit refinement: I would use some common prefix to say what flag does - mainly filtering.

Set filter to all-accounts Set filter to only cluster type

etc.

Current docs are less refined
Flags:
      --all-accounts             Set the ACL principal to match all principals (users and service accounts)
      --cluster                  Set the resource type to cluster
      --group string             Set the consumer group resource. When the --prefix option is also passed, this is used..
      --service-account string   Service account client ID used as principal for this operation

Falling back to not using flag abstraction here as the descriptions differ a bit owing to --prefix flag.

rkpattnaik780 · 2021-11-11T13:04:50Z

Having second thoughts adding a prefix regarding filter operation, wdyt @craicoverflow ?

craicoverflow

All looks good and works. However I noticed that the cluster flag still exists and I think we agreed it is not a requirement.

pkg/cmd/kafka/acl/flagutil/flagset.go

craicoverflow · 2021-11-11T10:26:25Z

pkg/cmd/kafka/acl/flagutil/flagset.go

 		flagName,
 		"",
-		fs.localizer.MustLocalize("kafka.acl.common.flag.transactionalID.description"),
+		fs.factory.Localizer.MustLocalize("kafka.acl.common.flag.transactionalID.description"),


nit: It may be good to create a local variable to save having to access this nested all the time

pkg/cmd/kafka/acl/list/list.go

craicoverflow · 2021-11-11T13:34:59Z

pkg/kafka/cmdutil/util.go

+
+// FilterValidTopicNameArgs filters topics from the API and returns the names
+// This is used in for dynamic completion of topic names
+func FilterValidTopicNameArgs(f *factory.Factory, toComplete string) (validNames []string, directive cobra.ShellCompDirective) {


nit: these could go in kafka flag util package?

pkg/localize/locales/en/cmd/acl.en.toml

pkg/cmd/kafka/acl/list/list.go

rkpattnaik780 added 3 commits November 8, 2021 20:04

feat(acl list): add flags to filter by resource

f075e62

docs: update acl list docs

d3561db

docs: add flag examples

4abc138

rkpattnaik780 requested a review from craicoverflow November 8, 2021 15:16

craicoverflow suggested changes Nov 8, 2021

View reviewed changes

craicoverflow reviewed Nov 8, 2021

View reviewed changes

rkpattnaik780 added 3 commits November 9, 2021 12:07

fix: address review comments

3afabd0

fix: address review comments

b3fcf0f

refactor: add autocompletions, change struct defs

f2c902f

wtrocki requested a review from craicoverflow November 11, 2021 10:02

wtrocki approved these changes Nov 11, 2021

View reviewed changes

fix: change help texts

e292865

rkpattnaik780 force-pushed the acl_list_filters branch from 7a212dd to e292865 Compare November 11, 2021 13:02

craicoverflow reviewed Nov 11, 2021

View reviewed changes

craicoverflow self-requested a review November 11, 2021 13:58

craicoverflow approved these changes Nov 11, 2021

View reviewed changes

rkpattnaik780 added 2 commits November 11, 2021 19:44

fix: remove localizer from flagset constructor

bdd57bf

fix: remove unused i18n key

e3313cf

rkpattnaik780 merged commit 5ea1f1b into main Nov 12, 2021

rkpattnaik780 deleted the acl_list_filters branch November 12, 2021 06:41

rkpattnaik780 mentioned this pull request Nov 12, 2021

Builds are failing on the main branch #1312

Closed

		flags.AddTopic(&opts.topic)
		flags.AddConsumerGroup(&opts.group)

Conversation

rkpattnaik780 commented Nov 8, 2021

Verification Steps

Type of change

Checklist

Uh oh!

wtrocki commented Nov 8, 2021

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craicoverflow Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

rkpattnaik780 Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

craicoverflow Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craicoverflow commented Nov 11, 2021

Uh oh!

rkpattnaik780 commented Nov 11, 2021

Uh oh!

craicoverflow commented Nov 11, 2021

Uh oh!

wtrocki commented Nov 11, 2021

Uh oh!

wtrocki commented Nov 11, 2021

Uh oh!

rkpattnaik780 commented Nov 11, 2021

Uh oh!

rkpattnaik780 commented Nov 11, 2021

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

craicoverflow Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

craicoverflow Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

craicoverflow Nov 10, 2021 •

edited

Loading