feat(kafka acl): add base and list command by rkpattnaik780 · Pull Request #1173 · redhat-developer/app-services-cli

rkpattnaik780 · 2021-10-04T12:30:54Z

This PR adds rhoas kafka acl list command to list all the Kafka ACL rules for a given instance.

Closes #1172

Verification Steps

rhoas kafka -h should display acl in list of sub commands
rhoas kafka acl list should display ACL rules for the selected Kafka instance.

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

craicoverflow · 2021-10-04T12:46:27Z

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

+
+[kafka.acl.cmd.example]
+one = '''
+


Remove this newline

craicoverflow · 2021-10-04T12:46:47Z

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

+
+[kafka.acl.cmd.longDescription]
+one = '''
+Set of commands that will let you manage resources


Suggested change

Set of commands that will let you manage resources

Set of commands that will let you manage resources.

Maybe "Kafka instance resources"?

typo, will make it Manage Kafka ACLs.

craicoverflow · 2021-10-04T14:04:05Z

I'm seeing an unexpected order of items. The UI shows a different order. The order from the CLI is the exact order that is returned from the API.

wtrocki · 2021-10-04T14:05:53Z

UI have numerous bugs logged in and still has some major development. I would not use it as a source of truth for the moment.

craicoverflow · 2021-10-04T14:05:55Z

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

+
+	cmd.Flags().Int32VarP(&opts.page, "page", "", cmdutil.ConvertPageValueToInt32(build.DefaultPageNumber), opts.localizer.MustLocalize("kafka.acl.list.flag.page.description"))
+	cmd.Flags().Int32VarP(&opts.size, "size", "", cmdutil.ConvertSizeValueToInt32(build.DefaultPageSize), opts.localizer.MustLocalize("kafka.acl.list.flag.size.description"))
+	cmd.Flags().StringVarP(&opts.output, "output", "o", "", opts.localizer.MustLocalize("kafka.acl.list.flag.output.description"))


Suggested change

cmd.Flags().StringVarP(&opts.output, "output", "o", "", opts.localizer.MustLocalize("kafka.acl.list.flag.output.description"))

cmd.Flags().StringVarP(&opts.output, "output", "o", dump.EmptyFormat, opts.localizer.MustLocalize("kafka.acl.list.flag.output.description"))

craicoverflow · 2021-10-04T14:06:39Z

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

+	req := api.AclsApi.GetAcls(opts.Context)
+
+	req.Page(float32(opts.page))
+
+	req.Size(float32(opts.size))
+
+	permissionsData, httpRes, err := req.Execute()


It's a matter of preference but this could be chained as it is a builder. Which do you prefer?

craicoverflow · 2021-10-04T14:08:14Z

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

+func formatPrincipal(principal string, localizer localize.Localizer) string {
+	s := strings.Split(principal, ":")[1]
+
+	if s == "*" {


Suggested change

if s == "*" {

if s == acl.Wildcard {

Something like this will be useful.

craicoverflow · 2021-10-04T14:09:43Z

UI have numerous bugs logged in and still has some major development. I would not use it as a source of truth for the moment.

Either way, I was expecting the last created ACL to appear at the bottom. How is it determined?

craicoverflow · 2021-10-04T14:12:19Z

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

+## Display Kafka ACL rules for the instance
+rhoas kafka acl list


Suggested change

## Display Kafka ACL rules for the instance

rhoas kafka acl list

# Display Kafka ACL rules for the instance

$ rhoas kafka acl list

craicoverflow · 2021-10-04T14:13:21Z

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

+
+
+[kafka.acl.list.flag.output.description]
+description = "Description for --output flag"


descriptions of flag i18n messages are redundant. Could be removed

craicoverflow · 2021-10-04T14:15:51Z

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

+			Permission:  fmt.Sprintf("%s | %s", p.GetPermission(), p.GetOperation()),
+			Description: fmt.Sprintf("%s %s \"%s\"", p.GetResourceType(), description, p.GetResourceName()),


I'm wondering if we should map raw ACL values to something more human readable. Example GROUP => Consumer Group, CLUSTER => Kafka instance.

I would keep them as in spec and aligned with the way our documentation will cover them

redhat-developer/app-services-guides#325

Yeah makes sense.

craicoverflow · 2021-10-04T14:20:40Z

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

+	req.Page(float32(opts.page))
+
+	req.Size(float32(opts.size))


These values are not being included because you need to use a builder or else reassign to reg like req = req.*

Let's address this if possible in this PR!

Done. Will merge it for now and raise a follow up.

craicoverflow

All good except pagination is not working.

craicoverflow

@rkpattnaik780 can you merge this and follow up on the comments in another PR (maybe create issues to track them). I need this closed as it included the base command.

wtrocki · 2021-10-04T14:37:43Z

Nice to have and great way to wrap your head around API.
Great work @rkpattnaik780

@rkpattnaik780 @craicoverflow let's release SDK as well - we basing on non recent changes which might cause some issues in some API's (incompatible changes were made)

feat(kafka acl): add base and list command

9746790

rkpattnaik780 requested a review from craicoverflow October 4, 2021 12:30

fix: build failing at docs/check

4a1f1b7

rkpattnaik780 removed the request for review from craicoverflow October 4, 2021 12:39

craicoverflow reviewed Oct 4, 2021

View reviewed changes

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

[kafka.acl.cmd.example]

one = '''

Copy link

Contributor

craicoverflow Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this newline

craicoverflow reviewed Oct 4, 2021

View reviewed changes

fix: address review comments

ef8cd27

rkpattnaik780 changed the base branch from main to kafka_acl October 4, 2021 13:41

rkpattnaik780 requested a review from craicoverflow October 4, 2021 13:56

craicoverflow reviewed Oct 4, 2021

View reviewed changes

craicoverflow self-requested a review October 4, 2021 14:23

craicoverflow suggested changes Oct 4, 2021

View reviewed changes

craicoverflow self-requested a review October 4, 2021 14:24

craicoverflow approved these changes Oct 4, 2021

View reviewed changes

fix: ineffective flag values

683db02

rkpattnaik780 merged commit 19db1da into kafka_acl Oct 4, 2021

rkpattnaik780 deleted the kafka_acl_cmd branch October 4, 2021 14:54

rkpattnaik780 mentioned this pull request Oct 5, 2021

fix(kafka acl): address review comments for list command #1180

Merged

9 tasks

craicoverflow pushed a commit to craicoverflow/app-services-cli that referenced this pull request Oct 7, 2021

feat(kafka acl): add base and list command (redhat-developer#1173)

c4b52f5

craicoverflow pushed a commit to craicoverflow/app-services-cli that referenced this pull request Oct 7, 2021

feat(kafka acl): add base and list command (redhat-developer#1173)

abe8e98

craicoverflow pushed a commit that referenced this pull request Oct 7, 2021

feat(kafka acl): add base and list command (#1173)

6676094

wtrocki pushed a commit that referenced this pull request Oct 12, 2021

feat(kafka acl): add base and list command (#1173)

0e467dc

craicoverflow mentioned this pull request Oct 12, 2021

feat(kafka acl): add acl delete command #1218

Merged

	Set of commands that will let you manage resources
	Set of commands that will let you manage resources.

	cmd.Flags().StringVarP(&opts.output, "output", "o", "", opts.localizer.MustLocalize("kafka.acl.list.flag.output.description"))
	cmd.Flags().StringVarP(&opts.output, "output", "o", dump.EmptyFormat, opts.localizer.MustLocalize("kafka.acl.list.flag.output.description"))

		## Display Kafka ACL rules for the instance
		rhoas kafka acl list



		[kafka.acl.list.flag.output.description]
		description = "Description for --output flag"

		Permission: fmt.Sprintf("%s \| %s", p.GetPermission(), p.GetOperation()),
		Description: fmt.Sprintf("%s %s \"%s\"", p.GetResourceType(), description, p.GetResourceName()),

Conversation

rkpattnaik780 commented Oct 4, 2021

Verification Steps

Type of change

Checklist

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!

craicoverflow commented Oct 4, 2021

Uh oh!

wtrocki commented Oct 4, 2021

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!

craicoverflow commented Oct 4, 2021

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!

Choose a reason for hiding this comment

Uh oh!

wtrocki Oct 4, 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!

Choose a reason for hiding this comment

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

wtrocki commented Oct 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wtrocki Oct 4, 2021 •

edited

Loading